fix(notifications): reload the inbox and its settings when the account changes
All checks were successful
PR Checks / android-build (pull_request) Successful in 8m12s
All checks were successful
PR Checks / android-build (pull_request) Successful in 8m12s
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wDDVXWMDz82WqE1i969r4
This commit is contained in:
@@ -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> = _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()
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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<Long>(), 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() {
|
||||
|
||||
Reference in New Issue
Block a user