From b7772f9dd66288b0310fabf282c35e44303a4fef Mon Sep 17 00:00:00 2001 From: rapterjet2004 Date: Thu, 24 Sep 2026 14:04:17 -0500 Subject: [PATCH] fix(conversationlist): keep list bound to its own account on switch The conversation list observed a single account id held by the singleton OfflineFirstConversationsRepository and set asynchronously by whichever getRooms() call ran last. A newly created list briefly showed the previous account's rooms, and a stale screen could switch the list back to the old account. The view model also captured the user once at construction from CurrentUserProviderOld, which is updated asynchronously and could still return the previous account. - Replace roomListFlow with observeRooms(accountId); getRooms() only syncs. Tag sync errors with their account id. - Bind ConversationsListViewModel to an explicit user via setUser() and observe that account's rooms, clearing the list on account change. - Pass KEY_INTERNAL_USER_ID when switching accounts, resolve the user from UserManager.currentUserFlow otherwise, and recreate the list in onResume when the followed active account has changed. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: rapterjet2004 --- .../ChooseAccountDialogCompose.kt | 1 + .../ConversationsListActivity.kt | 36 ++++++++-- .../data/OfflineConversationsRepository.kt | 22 +++--- .../OfflineFirstConversationsRepository.kt | 31 +++----- .../viewmodels/ConversationsListViewModel.kt | 72 ++++++++++++++----- .../ChooseAccountShareToDialogFragment.kt | 3 + ...onversationListFreshnessIntegrationTest.kt | 2 +- 7 files changed, 108 insertions(+), 59 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chooseaccount/ChooseAccountDialogCompose.kt b/app/src/main/java/com/nextcloud/talk/chooseaccount/ChooseAccountDialogCompose.kt index a162e45b873..77acdbe8bd8 100644 --- a/app/src/main/java/com/nextcloud/talk/chooseaccount/ChooseAccountDialogCompose.kt +++ b/app/src/main/java/com/nextcloud/talk/chooseaccount/ChooseAccountDialogCompose.kt @@ -300,6 +300,7 @@ class ChooseAccountDialogCompose { if (userManager.setUserAsActive(userItem.user)) { cookieManager.cookieStore.removeAll() val intent = Intent(activity, ConversationsListActivity::class.java) + intent.putExtra(BundleKeys.KEY_INTERNAL_USER_ID, userItem.user.id) intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP) activity.startActivity(intent) onSelected() diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt index d27efe82093..52ee53e496a 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt @@ -198,14 +198,10 @@ class ConversationsListActivity : BaseActivity() { NextcloudTalkApplication.sharedApplication!!.componentApplication.inject(this) ecosystemManager = EcosystemManager(this@ConversationsListActivity) - val targetUserId = intent.getLongExtra(KEY_INTERNAL_USER_ID, 0L) - currentUser = if (targetUserId != 0L) { - runBlocking { userManager.getUserWithId(targetUserId) }!! - } else { - currentUserProviderOld.currentUser.blockingGet() - } + currentUser = resolveUser() conversationsListViewModel = ViewModelProvider(this, viewModelFactory)[ConversationsListViewModel::class.java] + currentUser?.let { conversationsListViewModel.setUser(it) } conversationTagsViewModel = ViewModelProvider(this, viewModelFactory)[ConversationTagsViewModel::class.java] setSupportActionBar(null) @@ -230,6 +226,26 @@ class ConversationsListActivity : BaseActivity() { initObservers() } + /** + * The account this screen shows: the one passed via [KEY_INTERNAL_USER_ID], otherwise the + * active one. [UserManager.currentUserFlow] is preferred over [currentUserProviderOld], since + * it is updated synchronously by [UserManager.setUserAsActive]. + */ + private fun resolveUser(): User? { + val targetUserId = intent.getLongExtra(KEY_INTERNAL_USER_ID, 0L) + return if (targetUserId != 0L) { + runBlocking { userManager.getUserWithId(targetUserId) }!! + } else { + userManager.currentUserFlow.value ?: currentUserProviderOld.currentUser.blockingGet() + } + } + + /** True when this screen follows the active account, and the active account has changed. */ + private fun isActiveUserChanged(): Boolean { + val activeUser = userManager.currentUserFlow.value + return !intent.hasExtra(KEY_INTERNAL_USER_ID) && activeUser != null && activeUser.id != currentUser?.id + } + override fun onSaveInstanceState(outState: Bundle) { super.onSaveInstanceState(outState) outState.putBoolean(KEY_ACCOUNT_DIALOG_VISIBLE, showAccountDialogState.value) @@ -353,6 +369,7 @@ class ConversationsListActivity : BaseActivity() { if (user != null) { userManager.setUserAsActive(user) val intent = Intent(context, ConversationsListActivity::class.java) + intent.putExtra(KEY_INTERNAL_USER_ID, user.id) startActivity(intent) } else { showSnackbar(getString(R.string.nc_no_account_found)) @@ -415,6 +432,11 @@ class ConversationsListActivity : BaseActivity() { eventBus.register(this) } + if (isActiveUserChanged()) { + recreate() + return + } + if (currentUser != null) { if (isServerEOL(currentUser!!.serverVersion?.major)) { showServerEOLDialog() @@ -671,7 +693,7 @@ class ConversationsListActivity : BaseActivity() { } fun fetchRooms() { - conversationsListViewModel.getRooms(currentUser!!) + conversationsListViewModel.getRooms() } private fun fetchPendingInvitations() { diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt index 72d9efe6131..15fd478e5ac 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.kt @@ -16,20 +16,21 @@ import kotlinx.coroutines.flow.Flow interface OfflineConversationsRepository { /** - * Live stream of the observed account's conversations, for use in the conversation list. - * Backed by the local database: it re-emits whenever conversation rows change (room list - * sync, background catch-up, optimistic updates), with unchanged lists deduplicated. + * Live stream of the conversations of the account [accountId], for use in the conversation + * list. Backed by the local database: it re-emits whenever conversation rows change (room + * list sync, background catch-up, optimistic updates), with unchanged lists deduplicated. */ - val roomListFlow: Flow> + fun observeRooms(accountId: Long): Flow> /** * Emits when [getRooms] fails to sync with the server (e.g. a dropped/reset connection on a * slow network) while there are no locally cached conversations to fall back on for that * account, so the UI can tell the user why the list is empty instead of failing silently. * A failed sync while conversations are already cached does not emit here, since - * [roomListFlow] already has data to show and the sync is a best-effort background refresh. + * [observeRooms] already has data to show and the sync is a best-effort background refresh. + * Collectors must ignore errors whose [SyncError.accountId] is not the account they show. */ - val syncErrorFlow: Flow + val syncErrorFlow: Flow /** * Stream of a single conversation, for use in each conversations settings. @@ -38,9 +39,8 @@ interface OfflineConversationsRepository { val conversationFlow: Flow /** - * Selects the account observed by [roomListFlow] and synchronizes its conversations with - * the server (when online). The synced changes surface through [roomListFlow], which - * observes the database. + * Synchronizes the conversations of [user] with the server (when online). The synced changes + * surface through [observeRooms], which observes the database. */ @Deprecated("use observeConversation") fun getRooms(user: User): Job @@ -53,7 +53,7 @@ interface OfflineConversationsRepository { fun getRoom(user: User, roomToken: String): Job /** - * Updates a single conversation in the local database. [roomListFlow] observes the database + * Updates a single conversation in the local database. [observeRooms] observes the database * and re-emits the updated list on its own. */ suspend fun updateConversation(conversationModel: ConversationModel) @@ -62,4 +62,6 @@ interface OfflineConversationsRepository { suspend fun getLocallyStoredConversation(user: User, roomToken: String): ConversationModel? fun observeConversation(accountId: Long, roomToken: String): Flow + + data class SyncError(val accountId: Long, val throwable: Throwable) } diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index 91738cb1a9b..f149d36e8b3 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -32,15 +32,11 @@ import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.schedulers.Schedulers import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow -import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.map import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.launch @@ -59,33 +55,24 @@ class OfflineFirstConversationsRepository @Inject constructor( private val context: Context, private val logger: Logger ) : OfflineConversationsRepository { - private val observedAccountId = MutableStateFlow(null) - /** * The conversation list as a live view of the local database — the single source of truth. * Every write to the conversations table (room list sync, background message catch-up, - * optimistic read state, drafts) reaches collectors reactively; [getRooms] only selects the - * account to observe and triggers the background sync, which stays in place as the authority - * and self-healing safeguard. + * optimistic read state, drafts) reaches collectors reactively; [getRooms] only triggers the + * background sync, which stays in place as the authority and self-healing safeguard. */ - @OptIn(ExperimentalCoroutinesApi::class) - override val roomListFlow: Flow> = - observedAccountId - .filterNotNull() - .distinctUntilChanged() - .flatMapLatest { accountId -> - dao.getConversationsForUser(accountId) - .map { entities -> entities.map(ConversationEntity::toDomainModel) } - } + override fun observeRooms(accountId: Long): Flow> = + dao.getConversationsForUser(accountId) + .map { entities -> entities.map(ConversationEntity::toDomainModel) } .distinctUntilChanged() override val conversationFlow: Flow get() = _conversationFlow private val _conversationFlow: MutableSharedFlow = MutableSharedFlow() - override val syncErrorFlow: Flow + override val syncErrorFlow: Flow get() = _syncErrorFlow - private val _syncErrorFlow: MutableSharedFlow = MutableSharedFlow() + private val _syncErrorFlow: MutableSharedFlow = MutableSharedFlow() private val scope = CoroutineScope(Dispatchers.IO) @@ -109,8 +96,6 @@ class OfflineFirstConversationsRepository @Inject constructor( override fun getRooms(user: User): Job = scope.launch { - observedAccountId.value = user.id!! - if (networkMonitor.isOnline.value) { getRoomsFromServer(user) } @@ -207,7 +192,7 @@ class OfflineFirstConversationsRepository @Inject constructor( Log.e(TAG, "Something went wrong when fetching conversations", e) val hasCachedConversations = dao.getConversationsForUser(user.id!!).first().isNotEmpty() if (!hasCachedConversations) { - _syncErrorFlow.emit(e) + _syncErrorFlow.emit(OfflineConversationsRepository.SyncError(user.id!!, e)) } } return conversationsFromSync diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt index ecb3ab613d3..c5a957953de 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt @@ -42,13 +42,13 @@ import com.nextcloud.talk.utils.ApiUtils import com.nextcloud.talk.utils.CapabilitiesUtil.hasSpreedFeatureCapability import com.nextcloud.talk.utils.SpreedFeatures import com.nextcloud.talk.utils.UserIdUtils -import com.nextcloud.talk.utils.database.user.CurrentUserProviderOld import com.nextcloud.talk.utils.withRetry import io.reactivex.Observer import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.disposables.Disposable import io.reactivex.schedulers.Schedulers import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -57,10 +57,16 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch @@ -72,7 +78,6 @@ import javax.inject.Inject class ConversationsListViewModel @Inject constructor( private val repository: OfflineConversationsRepository, private val threadsRepository: ThreadsRepository, - private val currentUserProvider: CurrentUserProviderOld, private val openConversationsRepository: OpenConversationsRepository, private val contactsRepository: ContactsRepository, private val unifiedSearchRepository: UnifiedSearchRepository, @@ -84,11 +89,37 @@ class ConversationsListViewModel @Inject constructor( private val logger: Logger ) : ViewModel() { - private val _currentUser = currentUserProvider.currentUser.blockingGet() - val currentUser: User = _currentUser - val credentials = ApiUtils.getCredentials(_currentUser.username, _currentUser.token) ?: "" + private val userFlow = MutableStateFlow(null) - private val searchHelper = MessageSearchHelper(unifiedSearchRepository, currentUser) + /** The account this list shows. Must be set via [setUser] before the view model is used. */ + val currentUser: User + get() = checkNotNull(userFlow.value) { "setUser must be called before using the view model" } + + val credentials: String + get() = ApiUtils.getCredentials(currentUser.username, currentUser.token) ?: "" + + private var searchHelper: MessageSearchHelper? = null + + private val accountIdFlow = userFlow + .map { it?.id } + .filterNotNull() + .distinctUntilChanged() + + /** + * Binds the view model to [user]. Switching to another account drops the previous account's + * search state; the room list follows via [accountIdFlow]. + */ + fun setUser(user: User) { + val previous = userFlow.value + userFlow.value = user + if (previous?.id != user.id) { + searchHelper?.cancelSearch() + searchHelper = MessageSearchHelper(unifiedSearchRepository, user) + if (previous != null) { + cancelSearch() + } + } + } sealed interface ViewState @@ -145,7 +176,9 @@ class ConversationsListViewModel @Inject constructor( val getRoomsViewState: LiveData get() = _getRoomsViewState - val getRoomsFlow = repository.roomListFlow + @OptIn(ExperimentalCoroutinesApi::class) + val getRoomsFlow = accountIdFlow + .flatMapLatest { accountId -> repository.observeRooms(accountId) } .onEach { list -> _getRoomsViewState.value = GetRoomsSuccessState(list.isNotEmpty()) }.catch { @@ -154,7 +187,8 @@ class ConversationsListViewModel @Inject constructor( init { repository.syncErrorFlow - .onEach { throwable -> _getRoomsViewState.value = GetRoomsErrorState(throwable) } + .filter { error -> error.accountId == userFlow.value?.id } + .onEach { error -> _getRoomsViewState.value = GetRoomsErrorState(error.throwable) } .launchIn(viewModelScope) } @@ -166,8 +200,12 @@ class ConversationsListViewModel @Inject constructor( */ val isLoadingRooms: StateFlow = _isLoadingRooms.asStateFlow() - val getRoomsStateFlow = repository - .roomListFlow + @OptIn(ExperimentalCoroutinesApi::class) + val getRoomsStateFlow = accountIdFlow + .flatMapLatest { accountId -> + // Clear the previous account's rooms until the new account's first emission. + repository.observeRooms(accountId).onStart { emit(emptyList()) } + } .catch { throwable -> Log.e(TAG, "Error observing the conversation list", throwable) _getRoomsViewState.value = GetRoomsErrorState(throwable) @@ -348,7 +386,7 @@ class ConversationsListViewModel @Inject constructor( _isSearchLoadingFlow.value = false searchJob?.cancel() searchJob = null - searchHelper.cancelSearch() + searchHelper?.cancelSearch() searchResultEntries.value = emptyList() _currentSearchQueryFlow.value = "" } @@ -536,13 +574,13 @@ class ConversationsListViewModel @Inject constructor( private fun getMessagesFlow(search: String): Flow = flow { - emit(searchHelper.startMessageSearch(search)) + emit(checkNotNull(searchHelper).startMessageSearch(search)) }.flowOn(Dispatchers.IO) fun loadMoreMessages(context: Context) { viewModelScope.launch { val result = withContext(Dispatchers.IO) { - searchHelper.loadMore() + searchHelper?.loadMore() } ?: return@launch val newEntries: List = @@ -563,11 +601,11 @@ class ConversationsListViewModel @Inject constructor( } } - fun getRooms(user: User) { + fun getRooms() { val startNanoTime = System.nanoTime() Log.d(TAG, "fetchData - getRooms - calling: $startNanoTime") _isLoadingRooms.value = true - val job = repository.getRooms(user) + val job = repository.getRooms(currentUser) viewModelScope.launch { job.join() _isLoadingRooms.value = false @@ -576,7 +614,7 @@ class ConversationsListViewModel @Inject constructor( fun checkIfThreadsExist() { val limitForFollowedThreadsExistenceCheck = 1 - val accountId = UserIdUtils.getIdForUser(currentUserProvider.currentUser.blockingGet()) + val accountId = UserIdUtils.getIdForUser(currentUser) fun isLastCheckTooOld(lastCheckDate: Long): Boolean { val currentTimeMillis = System.currentTimeMillis() @@ -944,8 +982,6 @@ class ConversationsListViewModel @Inject constructor( } override fun onNext(invitationsModel: InvitationsModel) { - val currentUser = currentUserProvider.currentUser.blockingGet() - if (invitationsModel.user.userId?.equals(currentUser.userId) == true && invitationsModel.user.baseUrl?.equals(currentUser.baseUrl) == true ) { diff --git a/app/src/main/java/com/nextcloud/talk/ui/chooseaccount/ChooseAccountShareToDialogFragment.kt b/app/src/main/java/com/nextcloud/talk/ui/chooseaccount/ChooseAccountShareToDialogFragment.kt index f2a36063e12..e8f4939e171 100644 --- a/app/src/main/java/com/nextcloud/talk/ui/chooseaccount/ChooseAccountShareToDialogFragment.kt +++ b/app/src/main/java/com/nextcloud/talk/ui/chooseaccount/ChooseAccountShareToDialogFragment.kt @@ -29,6 +29,7 @@ import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.ui.chooseaccount.model.LoadUsersSuccessStateChooseAccountShareTo import com.nextcloud.talk.ui.chooseaccount.model.SwitchUserSuccessStateChooseAccountShareTo import com.nextcloud.talk.ui.theme.ViewThemeUtils +import com.nextcloud.talk.utils.bundle.BundleKeys import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.launch import java.net.CookieManager @@ -75,6 +76,8 @@ class ChooseAccountShareToDialogFragment : DialogFragment() { } is SwitchUserSuccessStateChooseAccountShareTo -> { cookieManager.cookieStore.removeAll() + // Let the recreated list follow the newly active account. + activity?.intent?.removeExtra(BundleKeys.KEY_INTERNAL_USER_ID) activity?.recreate() dismiss() } diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt index 99fc651faef..d1568172b71 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt @@ -379,7 +379,7 @@ class ConversationListFreshnessIntegrationTest { runBlocking { val emissions = mutableListOf>() val collector = launch(Dispatchers.IO) { - repository.roomListFlow.collect { emissions.add(it) } + repository.observeRooms(ACCOUNT_ID).collect { emissions.add(it) } } repository.getRooms(user).join()