From aa055469a8fb48d9eca21f77c066ba06829b1ad8 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Tue, 8 Sep 2026 17:13:53 -0500 Subject: [PATCH] fix(notifications): reload the inbox and its settings when the account changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defect Phase 14b found in `MyEventsViewModel` and flagged next door: the notifications surface has the identical shape, and it leaks the same way. A drawer route's view model outlives a sign-out. `navigateTopLevel` uses `popUpTo(HOME) { saveState = true }` with `restoreState = true`, so the `NavBackStackEntry` keeps its `ViewModelStore` and a view model that loaded only in `init` never runs again. Signing out and back in as somebody else showed the second account the FIRST account's inbox — titles and body text written for another person — with no request made at all, while the badge above the list showed the new account's real unread count, because the shell refreshes that on every session change. `InboxCache` was never the hole: it is keyed by (base URL, user id) and a snapshot has never crossed an account. The hole was the in-memory state, which nothing invalidated. Both view models now key on the signed-in account id, so a resume revalidation that returns the same user does not refetch. The inbox resets its state *before* loading rather than after, because `load()` paints the cache only when there is no `Success` on screen — otherwise the previous account's rows stay up for the whole round trip. The settings screen behind the inbox's gear is fixed with it, and there the stale render is worse than disclosure: those controls are written from, so a screen still showing the previous account's preferences would send this account's PUT built out of them. Walked on the emulator against a local website, before and after: two accounts with deliberately different inboxes, signed out and in within one process. Before, the second account saw the first's rows and the server logged no inbox fetch; after, it logs the fetch and shows its own. 572 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016wDDVXWMDz82WqE1i969r4 --- .../app/ui/notifications/InboxViewModel.kt | 35 +++++++++++++++- .../NotificationSettingsViewModel.kt | 17 +++++++- .../ui/notifications/InboxViewModelTest.kt | 40 +++++++++++++++++++ 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/runicgateway/app/ui/notifications/InboxViewModel.kt b/app/src/main/java/com/runicgateway/app/ui/notifications/InboxViewModel.kt index a8e7d49..6881f1f 100644 --- a/app/src/main/java/com/runicgateway/app/ui/notifications/InboxViewModel.kt +++ b/app/src/main/java/com/runicgateway/app/ui/notifications/InboxViewModel.kt @@ -20,6 +20,8 @@ import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import javax.inject.Inject @@ -39,6 +41,19 @@ import javax.inject.Inject * **Paging is keyset, not offset.** The next page is `before = the last id on * screen`, because the list gains rows at the top while it is being read and an * offset would show the same item twice or skip one. + * + * **It reloads when the ACCOUNT changes, not merely when it is created.** A + * drawer route's view model outlives a sign-out: `navigateTopLevel` saves and + * restores back-stack state, so the `NavBackStackEntry` keeps its + * `ViewModelStore` and a view model that loaded only in `init` never runs again. + * Signing out and back in as somebody else showed the second account the FIRST + * account's inbox — titles and body text written for another person — with no + * request made at all, while the badge beside it showed the new account's real + * count, because the shell refreshes that one on every session change. + * + * [InboxCache] was never the hole: it is keyed by `(base URL, user id)` and a + * snapshot has never crossed an account. The hole was the in-memory state, which + * nothing invalidated. */ @HiltViewModel class InboxViewModel @Inject constructor( @@ -68,7 +83,25 @@ class InboxViewModel @Inject constructor( val state: StateFlow = _state.asStateFlow() init { - load() + viewModelScope.launch { + sessionManager.state + .map { (it as? Session.SignedIn)?.user?.id } + .distinctUntilChanged() + .collect { userId -> + if (userId == null) { + // Signed out. The shell is already navigating away; drop the + // rows rather than leave them addressable behind it. + _state.value = State(items = UiState.Success(emptyList())) + } else { + // Reset BEFORE loading, not after: `load()` paints the cache + // only when there is no `Success` on screen, so the previous + // account's rows would otherwise stay up — and stay up for + // the whole round trip. + _state.value = State() + load() + } + } + } } /** diff --git a/app/src/main/java/com/runicgateway/app/ui/notifications/NotificationSettingsViewModel.kt b/app/src/main/java/com/runicgateway/app/ui/notifications/NotificationSettingsViewModel.kt index c522809..b4dc15a 100644 --- a/app/src/main/java/com/runicgateway/app/ui/notifications/NotificationSettingsViewModel.kt +++ b/app/src/main/java/com/runicgateway/app/ui/notifications/NotificationSettingsViewModel.kt @@ -7,6 +7,8 @@ import androidx.annotation.StringRes import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import com.runicgateway.app.R +import com.runicgateway.app.core.auth.Session +import com.runicgateway.app.core.auth.SessionManager import com.runicgateway.app.core.push.PushManager import com.runicgateway.app.core.result.ApiResult import com.runicgateway.app.data.api.dto.NotificationChannelItemDto @@ -19,6 +21,8 @@ import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import javax.inject.Inject @@ -51,6 +55,7 @@ class NotificationSettingsViewModel @Inject constructor( private val notifications: NotificationsRepository, private val playerShard: PlayerShardRepository, private val pushManager: PushManager, + sessionManager: SessionManager, ) : ViewModel() { data class Feedback(val ok: Boolean, @param:StringRes val messageRes: Int) @@ -72,7 +77,17 @@ class NotificationSettingsViewModel @Inject constructor( viewModelScope.launch { pushManager.supported.collect { supported -> _state.update { it.copy(supported = supported) } } } - load() + // Reloaded on an account change for the reason the inbox is, and one + // reason more: these controls are WRITTEN from. A screen still rendering + // the previous account's preferences would send this account's PUT built + // out of them, so a stale render here corrupts rather than merely + // discloses. + viewModelScope.launch { + sessionManager.state + .map { (it as? Session.SignedIn)?.user?.id } + .distinctUntilChanged() + .collect { userId -> if (userId != null) load() } + } } fun load() { diff --git a/app/src/test/java/com/runicgateway/app/ui/notifications/InboxViewModelTest.kt b/app/src/test/java/com/runicgateway/app/ui/notifications/InboxViewModelTest.kt index 2e6c461..2ec5b24 100644 --- a/app/src/test/java/com/runicgateway/app/ui/notifications/InboxViewModelTest.kt +++ b/app/src/test/java/com/runicgateway/app/ui/notifications/InboxViewModelTest.kt @@ -11,6 +11,7 @@ import com.runicgateway.app.core.net.BaseUrlHolder import com.runicgateway.app.data.api.dto.NotificationInboxDto import com.runicgateway.app.data.api.dto.NotificationItemDto import com.runicgateway.app.data.api.dto.NotificationReadResultDto +import com.runicgateway.app.data.api.dto.SafeUserDto import com.runicgateway.app.data.api.fake.FakeNotificationsApi import com.runicgateway.app.data.repository.NotificationsRepository import com.runicgateway.app.ui.UiState @@ -194,6 +195,45 @@ class InboxViewModelTest { assertNull(vm.linkFor(item(1).copy(url = "intent://evil#Intent;end"))) } + // ── The inbox belongs to ONE account ───────────────────────────── + + @Test fun switchingAccountDoesNotShowThePreviousOnesInbox() { + // **Found on the emulator, not by a test.** A drawer route's view model + // outlives a sign-out: `navigateTopLevel` saves and restores back-stack + // state, so the entry keeps its ViewModelStore and a view model that + // loaded only in `init` never runs again. Signing out and back in as + // somebody else showed the second account the FIRST account's inbox — + // titles and body text written for another person — with no request made + // at all, while the badge beside it showed the new account's real count. + val sessions = session(userId = 7) + api.pages = mapOf(null to NotificationInboxDto(items = listOf(item(1), item(2)), unread = 2)) + val vm = InboxViewModel(NotificationsRepository(api), cache, sessions, baseUrl) + assertEquals(listOf(1L, 2L), shown(vm)!!.map { it.id }) + + api.pages = mapOf(null to NotificationInboxDto(items = listOf(item(9)), unread = 1)) + sessions.onSignedOut() + // Signed out, the previous account's rows are gone rather than left + // addressable behind a shell that is navigating away. + assertEquals(emptyList(), shown(vm)!!.map { it.id }) + + sessions.onSignedIn("a", "r", SafeUserDto(id = 8, username = "bob", role = "player")) + assertEquals(listOf(9L), shown(vm)!!.map { it.id }) + } + + @Test fun aResumeRevalidationReturningTheSameUserDoesNotRefetch() { + // The gate is the account, not every session emission — the app + // re-validates its role on every resume. + val sessions = session(userId = 7) + api.pages = mapOf(null to NotificationInboxDto(items = listOf(item(1)), unread = 1)) + val vm = InboxViewModel(NotificationsRepository(api), cache, sessions, baseUrl) + val callsAfterFirstLoad = api.inboxCalls.size + + sessions.onUserRefreshed(SafeUserDto(id = 7, username = "alice", role = "admin")) + + assertEquals(callsAfterFirstLoad, api.inboxCalls.size) + assertEquals(listOf(1L), shown(vm)!!.map { it.id }) + } + // ── Opening an item in the app rather than a browser (M13) ───────── @Test fun anEventAnnouncementOpensNativelyAndKeepsItsRun() { -- 2.49.1