diff --git a/app/app/src/main/java/org/modg/bookshelf/data/prefs/SettingsStore.kt b/app/app/src/main/java/org/modg/bookshelf/data/prefs/SettingsStore.kt index 35ef865..9a04dfe 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/prefs/SettingsStore.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/prefs/SettingsStore.kt @@ -3,6 +3,7 @@ package org.modg.bookshelf.data.prefs import android.content.Context import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey @@ -27,6 +28,10 @@ class SettingsStore(private val context: Context) { val USER_EMAIL = stringPreferencesKey("user_email") val LAST_SYNC_TIME = longPreferencesKey("last_sync_time") val LAST_SHELF_ID = stringPreferencesKey("last_shelf_id") + // Named for the incident, not the feature: a generic "did the full repull + // run" flag would invite reuse for some future, unrelated repull with + // different safety requirements. See SyncEngine.sync(). + val FULL_REPULL_2026_09_17_DONE = booleanPreferencesKey("full_repull_2026_09_17_done") fun cursor(collection: String) = stringPreferencesKey("cursor_$collection") } @@ -36,6 +41,9 @@ class SettingsStore(private val context: Context) { val userEmail: Flow = context.dataStore.data.map { it[Keys.USER_EMAIL] } val lastSyncTime: Flow = context.dataStore.data.map { it[Keys.LAST_SYNC_TIME] } + /** See [Keys.FULL_REPULL_2026_09_17_DONE] and [setFullRepullDone]. */ + val fullRepullDone: Flow = context.dataStore.data.map { it[Keys.FULL_REPULL_2026_09_17_DONE] ?: false } + /** * The shelf most recently assigned to a book — SPEC "remember the most recently * used shelf" (shelving a box of books usually means one shelf, over and over). @@ -66,10 +74,30 @@ class SettingsStore(private val context: Context) { context.dataStore.edit { it[Keys.cursor(collection)] = cursor } } + /** Forces the next sync's pull to fetch that collection from scratch. */ + suspend fun clearCursor(collection: String) { + context.dataStore.edit { it.remove(Keys.cursor(collection)) } + } + suspend fun setLastSyncTime(epochMillis: Long) { context.dataStore.edit { it[Keys.LAST_SYNC_TIME] = epochMillis } } + /** One-way: production code never needs to un-set this once a full re-pull has landed. */ + suspend fun setFullRepullDone() { + context.dataStore.edit { it[Keys.FULL_REPULL_2026_09_17_DONE] = true } + } + + /** + * Test-only seam (internal, not private) — DataStore's on-disk state + * otherwise leaks across Robolectric test methods within a class; see + * [org.modg.bookshelf.data.prefs.SettingsStoreTest]'s [clearLastShelfId] + * call in its own setUp for the same reason. + */ + internal suspend fun resetFullRepullDoneForTesting() { + context.dataStore.edit { it.remove(Keys.FULL_REPULL_2026_09_17_DONE) } + } + suspend fun setLastShelfId(shelfId: String) { context.dataStore.edit { it[Keys.LAST_SHELF_ID] = shelfId } } @@ -92,6 +120,18 @@ class SettingsStore(private val context: Context) { } } + /** + * Auth EXPIRY, not sign-out: the 2026-09-17 incident (server logged + * `auth: ""` on every request — a 5-day-old token PocketBase silently + * treated as anonymous rather than rejecting) means the token can go bad + * on its own. Unlike [clearAuth], this keeps server URL, user id AND + * email so the setup screen can pre-fill everything but the password — + * re-signing-in after an expiry should be one field, not three. + */ + suspend fun clearAuthToken() { + context.dataStore.edit { it.remove(Keys.AUTH_TOKEN) } + } + companion object { const val COLLECTION_BOOKS = "books" const val COLLECTION_SHELVES = "shelves" diff --git a/app/app/src/main/java/org/modg/bookshelf/data/remote/PocketBaseApi.kt b/app/app/src/main/java/org/modg/bookshelf/data/remote/PocketBaseApi.kt index e2824d4..9ae3518 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/remote/PocketBaseApi.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/remote/PocketBaseApi.kt @@ -20,6 +20,19 @@ interface PocketBaseApi { @POST("api/collections/users/auth-with-password") suspend fun authWithPassword(@Body body: AuthWithPasswordRequest): AuthResponse + /** + * Re-issues a fresh token for the currently-attached one (see + * [PbAuthInterceptor]) — same response shape as [authWithPassword]. Called + * at the start of every [org.modg.bookshelf.data.repo.SyncEngine.sync]: a + * PocketBase user token that has quietly expired is NOT rejected by the + * server, it's just treated as anonymous, and an anonymous request 404s + * on records the rules hide instead of 401ing — see + * [org.modg.bookshelf.data.repo.SyncEngine]'s 404 hard-delete helpers for + * the 2026-09-17 incident that caused. + */ + @POST("api/collections/users/auth-refresh") + suspend fun authRefresh(): AuthResponse + // ---- books ---- @GET("api/collections/books/records") diff --git a/app/app/src/main/java/org/modg/bookshelf/data/repo/AuthRepository.kt b/app/app/src/main/java/org/modg/bookshelf/data/repo/AuthRepository.kt index 366c33c..aed0b63 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/repo/AuthRepository.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/repo/AuthRepository.kt @@ -18,6 +18,9 @@ class AuthRepository( val isLoggedIn: Flow = settingsStore.authToken.map { !it.isNullOrBlank() } val serverUrl: Flow = settingsStore.serverUrl + /** Read by the setup screen to pre-fill re-sign-in after an auth expiry (SettingsStore.clearAuthToken keeps this). */ + val userEmail: Flow = settingsStore.userEmail + /** Normalizes per SPEC: requires https, strips a trailing slash. */ suspend fun setServerUrl(rawUrl: String): Result { val trimmed = rawUrl.trim().trimEnd('/') @@ -36,6 +39,12 @@ class AuthRepository( settingsStore.setAuthToken(response.token) settingsStore.setUserId(response.record.id) settingsStore.setUserEmail(email) + // Every fresh sign-in reconciles fully, not just the one after the + // 2026-09-17 incident's stuck cursors — the same backstop for any + // future cause we haven't thought of yet. + settingsStore.clearCursor(SettingsStore.COLLECTION_BOOKCASES) + settingsStore.clearCursor(SettingsStore.COLLECTION_SHELVES) + settingsStore.clearCursor(SettingsStore.COLLECTION_BOOKS) Result.success(Unit) } catch (e: Exception) { Result.failure(e) diff --git a/app/app/src/main/java/org/modg/bookshelf/data/repo/SyncEngine.kt b/app/app/src/main/java/org/modg/bookshelf/data/repo/SyncEngine.kt index c71fa81..3c19d39 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/repo/SyncEngine.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/repo/SyncEngine.kt @@ -1,5 +1,6 @@ package org.modg.bookshelf.data.repo +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.first import okhttp3.MultipartBody import okhttp3.RequestBody.Companion.asRequestBody @@ -26,6 +27,13 @@ sealed class SyncResult { object Success : SyncResult() object Skipped : SyncResult() data class Failure(val error: Throwable) : SyncResult() + + /** + * The session is gone (auth-refresh, or any push/pull mid-sync, came back + * 401/403). Distinct from [Failure]: the fix isn't "retry later", it's + * "sign in again" — see the 2026-09-17 incident in SyncEngine's KDoc. + */ + object AuthExpired : SyncResult() } /** @@ -35,6 +43,18 @@ sealed class SyncResult { * is bookcases -> shelves -> books both ways, so relations resolve cleanly * even though Room does not enforce foreign keys between them (sync would * otherwise have to special-case out-of-order pages). + * + * 2026-09-17 incident: user auth tokens lasted PocketBase's default 5 days, + * the app logged in once on the setup screen and never refreshed, and + * PocketBase does NOT reject an expired/invalid `Authorization` header — it + * silently treats the request as anonymous. The `@request.auth.id != ""` + * collection rules then turned every write anonymous-404/400, and this + * class's own 404-means-deleted handling (see the update* helpers below) + * hard-deleted five books that were still intact on the server. Server + * tokens now last 180 days (commit ab1d294) as a backstop, but the real fix + * is here: refresh the token at the top of every sync, before any push/pull, + * so a stale token surfaces as [SyncResult.AuthExpired] instead of silently + * degrading into anonymous requests that 404 on records the rules are hiding. */ class SyncEngine( private val apiProvider: ApiProvider, @@ -46,6 +66,25 @@ class SyncEngine( suspend fun sync(): SyncResult { val api = apiProvider.api() ?: return SyncResult.Skipped return try { + val refreshed = api.authRefresh() + settingsStore.setAuthToken(refreshed.token) + settingsStore.setUserId(refreshed.record.id) + + // One-time recovery for the 2026-09-17 incident: rows hard-deleted + // locally by an anonymous-404 during the outage are still on the + // server, but the pull cursor has already moved past their + // `updated`, so an ordinary incremental pull will never see them + // again. Re-pulling everything once is safe — see + // applyIncomingBook/shouldApplyRemote below: a row missing + // locally is always inserted, and a locally-dirty row only ever + // loses to a strictly newer remote copy. + val recovering = !settingsStore.fullRepullDone.first() + if (recovering) { + settingsStore.clearCursor(SettingsStore.COLLECTION_BOOKCASES) + settingsStore.clearCursor(SettingsStore.COLLECTION_SHELVES) + settingsStore.clearCursor(SettingsStore.COLLECTION_BOOKS) + } + pushBookcases(api) pushShelves(api) pushBooks(api) @@ -53,7 +92,24 @@ class SyncEngine( pullShelves(api) pullBooks(api) settingsStore.setLastSyncTime(System.currentTimeMillis()) + // Only after the whole sync succeeds — a sync that fails partway + // through (e.g. pull throws) must retry the full re-pull next time. + if (recovering) settingsStore.setFullRepullDone() SyncResult.Success + } catch (e: CancellationException) { + throw e + } catch (e: HttpException) { + if (e.code() == 401 || e.code() == 403) { + // Belt and braces: this catches both authRefresh's own 401/403 + // and one from any push/pull call later in this same sync + // (e.g. the token was revoked mid-sync). The 404 handling in + // the update* helpers below only matches code 404, so it can + // never swallow this. + settingsStore.clearAuthToken() + SyncResult.AuthExpired + } else { + SyncResult.Failure(e) + } } catch (e: IOException) { SyncResult.Failure(e) } catch (e: Exception) { @@ -85,6 +141,15 @@ class SyncEngine( } } + /** + * A 404 here is trusted as "deleted on the server" ONLY because [sync] + * proved the attached token valid, moments earlier, by refreshing it. On + * 2026-09-17 a request that carried a quietly-expired token 404'd on a + * bookcase that still existed — PocketBase's create/update rules hide a + * record from an unauthenticated caller rather than 401ing it, so an + * anonymous PATCH to an existing id looks identical to one to a deleted + * id. Do not call this without a fresh refresh in the same [sync] run. + */ private suspend fun updateBookcase(api: PocketBaseApi, entity: BookcaseEntity) { try { val response = api.updateBookcase(entity.id, entity.toDto()) @@ -115,6 +180,7 @@ class SyncEngine( } } + /** Same trust argument as [updateBookcase]'s KDoc — see it for the 2026-09-17 incident. */ private suspend fun updateShelf(api: PocketBaseApi, entity: ShelfEntity) { try { val response = api.updateShelf(entity.id, entity.toDto()) @@ -145,6 +211,13 @@ class SyncEngine( } } + /** + * Same trust argument as [updateBookcase]'s KDoc — see it for the + * 2026-09-17 incident. This is the helper that actually lost the user's + * books that day: five records hard-deleted locally while fully intact + * on the server, because the request that 404'd on them carried no valid + * auth and PocketBase's rules hid rather than rejected them. + */ private suspend fun updateBook(api: PocketBaseApi, entity: BookEntity) { try { val response = api.updateBook(entity.id, entity.toDto()) diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/components/SyncStatusBar.kt b/app/app/src/main/java/org/modg/bookshelf/ui/components/SyncStatusBar.kt index e935b94..8060bb5 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/components/SyncStatusBar.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/components/SyncStatusBar.kt @@ -7,6 +7,7 @@ import androidx.compose.animation.core.infiniteRepeatable import androidx.compose.animation.core.rememberInfiniteTransition import androidx.compose.animation.core.tween import androidx.compose.foundation.background +import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Row @@ -36,6 +37,9 @@ enum class SyncStatus { Syncing, Offline, Error, + + /** The session expired (see SyncEngine's 2026-09-17 incident KDoc) — distinct from [Error] so the bar is tappable to re-sign-in. */ + AuthExpired, } /** @@ -52,11 +56,16 @@ fun SyncStatusBar( status: SyncStatus, label: String, modifier: Modifier = Modifier, + // Only [SyncStatus.AuthExpired] wires this up today (SPEC: sync failure is + // a quiet status line, never a dialog — but "sign in again" needs SOME + // affordance, and this bar is what the user is already looking at). + onClick: (() -> Unit)? = null, ) { Row( modifier = modifier .fillMaxWidth() .background(MaterialTheme.colorScheme.surfaceContainerLow) + .let { if (onClick != null) it.clickable(onClick = onClick) else it } .navigationBarsPadding() .padding(horizontal = 28.dp, vertical = 8.dp), verticalAlignment = Alignment.CenterVertically, @@ -77,7 +86,7 @@ private fun StatusDot(status: SyncStatus) { SyncStatus.Synced -> MaterialTheme.colorScheme.secondary SyncStatus.Syncing -> MaterialTheme.colorScheme.secondary SyncStatus.Offline -> MaterialTheme.colorScheme.onSurfaceVariant - SyncStatus.Error -> MaterialTheme.colorScheme.error + SyncStatus.Error, SyncStatus.AuthExpired -> MaterialTheme.colorScheme.error } if (status == SyncStatus.Syncing) { val transition = rememberInfiniteTransition(label = "sync-pulse") diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt index 2275236..c058589 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt @@ -105,6 +105,10 @@ fun LibraryScreen( container: AppContainer, onEnterByHandWithQuery: (String) -> Unit = { onAddByHandClick() }, onEditOnlineResult: (OnlineBook, BookMetadata?) -> Unit = { _, _ -> }, + // 2026-09-17 incident: the user is looking at THIS screen when the + // session expires, not the setup screen — MainActivity's start-destination + // check only runs on a fresh launch. Defaults to a no-op for previews/tests. + onAuthExpired: () -> Unit = {}, ) { val viewModel: LibraryViewModel = viewModel( // A fresh VM per distinct shelf filter, so tapping a different shelf @@ -161,7 +165,11 @@ fun LibraryScreen( } }, syncStatusBar = { - SyncStatusBar(status = uiState.syncBar.status, label = uiState.syncBar.label) + SyncStatusBar( + status = uiState.syncBar.status, + label = uiState.syncBar.label, + onClick = if (uiState.syncBar.status == SyncStatus.AuthExpired) onAuthExpired else null, + ) }, ) { innerPadding -> PullToRefreshBox( diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibrarySyncPresenter.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibrarySyncPresenter.kt index 532f06d..945e6b6 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibrarySyncPresenter.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibrarySyncPresenter.kt @@ -8,6 +8,9 @@ enum class SyncOutcome { SUCCESS, FAILURE, NO_SERVER, + + /** See [org.modg.bookshelf.data.repo.SyncResult.AuthExpired] — the fix is "sign in again", not "retry". */ + AUTH_EXPIRED, } data class SyncBarState(val status: SyncStatus, val label: String) @@ -29,6 +32,7 @@ object LibrarySyncPresenter { return when (lastOutcome) { SyncOutcome.FAILURE -> SyncBarState(SyncStatus.Error, "Sync failed — showing local library") SyncOutcome.NO_SERVER -> SyncBarState(SyncStatus.Offline, "No server configured") + SyncOutcome.AUTH_EXPIRED -> SyncBarState(SyncStatus.AuthExpired, "Signed out — sign in again to sync") SyncOutcome.SUCCESS, SyncOutcome.NONE -> if (lastSyncMillis != null) { SyncBarState(SyncStatus.Synced, "Synced ${elapsedPhrase(nowMillis - lastSyncMillis)}") } else { diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt index 91e1dad..988089f 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt @@ -137,7 +137,9 @@ class LibraryViewModel( ) private val _onlineSearchPhase = MutableStateFlow(OnlineSearchPhase.NotSearched) - private var onlineSearchJob: Job? = null + + /** Internal (not private) so tests can join it to deterministically wait out a cancelled search. */ + internal var onlineSearchJob: Job? = null /** * Recomputed from [allBooks] on every change, not frozen at search time — so a @@ -425,6 +427,7 @@ class LibraryViewModel( is SyncResult.Success -> _lastSyncOutcome.value = SyncOutcome.SUCCESS is SyncResult.Failure -> _lastSyncOutcome.value = SyncOutcome.FAILURE is SyncResult.Skipped -> _lastSyncOutcome.value = SyncOutcome.NO_SERVER + is SyncResult.AuthExpired -> _lastSyncOutcome.value = SyncOutcome.AUTH_EXPIRED } _isSyncing.value = false } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/nav/BookshelfNavHost.kt b/app/app/src/main/java/org/modg/bookshelf/ui/nav/BookshelfNavHost.kt index 6b64b56..b839a71 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/nav/BookshelfNavHost.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/nav/BookshelfNavHost.kt @@ -57,6 +57,11 @@ fun BookshelfNavHost( onAddByHandClick = { navController.navigate(Routes.addBook()) }, onLocationsClick = { navController.navigate(Routes.LOCATIONS) }, onSettingsClick = { navController.navigate(Routes.SETTINGS) }, + onAuthExpired = { + navController.navigate(Routes.SETUP) { + popUpTo(navController.graph.id) { inclusive = true } + } + }, container = container, // Wave 10: "Can't find it? Enter it by hand" under the online results — // pre-fills the add-by-hand form's title with the search query. diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsScreen.kt index 4330be9..5a7ba35 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsScreen.kt @@ -104,12 +104,30 @@ fun SettingsScreen( SectionHeading("Sync") InfoRow(label = "Last synced", value = formatLastSync(state.lastSyncTime)) - PrimaryButton( - text = if (state.isSyncing) "Syncing…" else "Sync now", - onClick = viewModel::syncNow, - enabled = !state.isSyncing, - modifier = Modifier.padding(top = 8.dp), - ) + if (state.syncAuthExpired) { + // Not a "Sync failed" dead end: the fix is signing in again, so the + // button goes straight to setup instead of retrying the same sync. + // Goes through onSignedOut directly, NOT viewModel.signOut(onSignedOut) + // — that also clears the stored email/user id, and setup pre-fills those. + Text( + text = "Your session expired. Sign in again.", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.padding(top = 8.dp), + ) + PrimaryButton( + text = "Sign in", + onClick = onSignedOut, + modifier = Modifier.padding(top = 8.dp), + ) + } else { + PrimaryButton( + text = if (state.isSyncing) "Syncing…" else "Sync now", + onClick = viewModel::syncNow, + enabled = !state.isSyncing, + modifier = Modifier.padding(top = 8.dp), + ) + } GoldDivider(modifier = Modifier.padding(vertical = 16.dp)) @@ -239,6 +257,7 @@ internal fun InfoRow(label: String, value: String) { private fun syncLabel(state: SettingsUiState): String = when { state.isSyncing -> "Syncing…" + state.syncAuthExpired -> "Signed out — sign in again to sync" state.syncError != null -> "Sync failed: ${state.syncError}" state.lastSyncTime != null -> "Synced • ${formatLastSync(state.lastSyncTime)}" else -> "Not synced yet" diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsViewModel.kt index 6c4edc7..177dbb0 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/settings/SettingsViewModel.kt @@ -26,11 +26,13 @@ data class SettingsUiState( val lastSyncTime: Long? = null, val isSyncing: Boolean = false, val syncError: String? = null, + val syncAuthExpired: Boolean = false, val latestCrash: CrashReport? = null, ) { val syncStatus: SyncStatus get() = when { isSyncing -> SyncStatus.Syncing + syncAuthExpired -> SyncStatus.AuthExpired syncError != null -> SyncStatus.Error lastSyncTime != null -> SyncStatus.Synced else -> SyncStatus.Offline @@ -56,6 +58,7 @@ class SettingsViewModel( private val isSyncing = MutableStateFlow(false) private val syncError = MutableStateFlow(null) + private val syncAuthExpired = MutableStateFlow(false) // Read only when this screen is opened (never at app startup) and off the // main thread — CrashReporter.list() reads files. See its KDoc. @@ -83,7 +86,7 @@ class SettingsViewModel( } val uiState: StateFlow = - combine(baseInfo, isSyncing, syncError, latestCrash) { base, syncing, error, crash -> + combine(baseInfo, isSyncing, syncError, syncAuthExpired, latestCrash) { base, syncing, error, authExpired, crash -> SettingsUiState( serverUrl = base.serverUrl, userEmail = base.userEmail, @@ -92,6 +95,7 @@ class SettingsViewModel( lastSyncTime = base.lastSyncTime, isSyncing = syncing, syncError = error, + syncAuthExpired = authExpired, latestCrash = crash, ) }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), SettingsUiState()) @@ -101,6 +105,7 @@ class SettingsViewModel( viewModelScope.launch { isSyncing.value = true syncError.value = null + syncAuthExpired.value = false // SyncEngine never throws (SPEC: sync failure is a quiet status // line, never a crash or a blocking dialog) — surface its result // as that quiet line, nothing more. @@ -108,6 +113,9 @@ class SettingsViewModel( is SyncResult.Success -> Unit is SyncResult.Skipped -> syncError.value = "No server configured yet." is SyncResult.Failure -> syncError.value = result.error.message ?: "Sync failed." + // Not a generic failure: "Sync failed: HTTP 401" would tell the user + // to retry, when what actually fixes this is signing in again. + is SyncResult.AuthExpired -> syncAuthExpired.value = true } isSyncing.value = false } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/setup/SetupViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/setup/SetupViewModel.kt index eed0e5b..78cc8e9 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/setup/SetupViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/setup/SetupViewModel.kt @@ -5,6 +5,7 @@ import androidx.lifecycle.viewModelScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -46,6 +47,16 @@ class SetupViewModel(private val authRepository: AuthRepository) : ViewModel() { .readTimeout(6, TimeUnit.SECONDS) .build() + init { + // Re-signing-in after an auth expiry (SettingsStore.clearAuthToken keeps + // server URL, user id and email) should be typing the password only. + viewModelScope.launch { + val url = authRepository.serverUrl.first() + val email = authRepository.userEmail.first() + _uiState.update { it.copy(serverUrl = url ?: it.serverUrl, email = email ?: it.email) } + } + } + fun onServerUrlChanged(value: String) { _uiState.update { it.copy(serverUrl = value, urlError = null) } } diff --git a/app/app/src/test/java/org/modg/bookshelf/data/repo/AuthRepositoryTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/repo/AuthRepositoryTest.kt index b16eebd..69ca183 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/repo/AuthRepositoryTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/repo/AuthRepositoryTest.kt @@ -49,4 +49,22 @@ class AuthRepositoryTest { assertNull(settingsStore.userEmail.first()) } + + /** + * Backstop for causes of stuck cursors we haven't thought of yet (see the + * 2026-09-17 incident's one-time re-pull in SyncEngine.sync()): every fresh + * sign-in reconciles fully, not just the specific recovery path. + */ + @Test + fun `login success clears all three pull cursors so a fresh sign-in reconciles fully`() = runTest { + settingsStore.setCursor(SettingsStore.COLLECTION_BOOKS, "2024-01-01 00:00:00.000Z") + settingsStore.setCursor(SettingsStore.COLLECTION_SHELVES, "2024-01-01 00:00:00.000Z") + settingsStore.setCursor(SettingsStore.COLLECTION_BOOKCASES, "2024-01-01 00:00:00.000Z") + + repository.login(email = "reader@example.com", password = "hunter2") + + assertNull(settingsStore.cursorFor(SettingsStore.COLLECTION_BOOKS).first()) + assertNull(settingsStore.cursorFor(SettingsStore.COLLECTION_SHELVES).first()) + assertNull(settingsStore.cursorFor(SettingsStore.COLLECTION_BOOKCASES).first()) + } } diff --git a/app/app/src/test/java/org/modg/bookshelf/data/repo/FakePocketBaseApi.kt b/app/app/src/test/java/org/modg/bookshelf/data/repo/FakePocketBaseApi.kt index 1cf297b..c40ab92 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/repo/FakePocketBaseApi.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/repo/FakePocketBaseApi.kt @@ -1,5 +1,6 @@ package org.modg.bookshelf.data.repo +import kotlinx.coroutines.CancellationException import okhttp3.MediaType.Companion.toMediaType import okhttp3.MultipartBody import okhttp3.ResponseBody.Companion.toResponseBody @@ -10,8 +11,10 @@ import org.modg.bookshelf.data.remote.BookcaseDto import org.modg.bookshelf.data.remote.PbListResponse import org.modg.bookshelf.data.remote.PocketBaseApi import org.modg.bookshelf.data.remote.ShelfDto +import org.modg.bookshelf.data.remote.UserRecordDto import retrofit2.HttpException import retrofit2.Response +import java.io.IOException fun httpError(code: Int): HttpException { val body = "{}".toResponseBody("application/json".toMediaType()) @@ -37,6 +40,16 @@ class FakePocketBaseApi : PocketBaseApi { val updateShelfErrors = mutableMapOf() val updateBookErrors = mutableMapOf() + /** Defaults to a normal, successful refresh — most tests aren't exercising the auth-expiry path. */ + var authRefreshHttpError: HttpException? = null + var authRefreshIOException: IOException? = null + var authRefreshCancellation: CancellationException? = null + var refreshedToken: String = "refreshed-fake-token" + var refreshedUserId: String = "fake-user-id" + + /** One-shot: exercises the "a sync whose pull fails leaves the one-time-repull flag unset" case. */ + var nextListBooksError: HttpException? = null + fun seedBook(dto: BookDto) { books[dto.id] = dto } @@ -44,9 +57,19 @@ class FakePocketBaseApi : PocketBaseApi { override suspend fun authWithPassword(body: AuthWithPasswordRequest): AuthResponse = AuthResponse(token = "fake-token") + override suspend fun authRefresh(): AuthResponse { + callLog += "authRefresh" + authRefreshCancellation?.let { throw it } + authRefreshIOException?.let { throw it } + authRefreshHttpError?.let { throw it } + return AuthResponse(token = refreshedToken, record = UserRecordDto(id = refreshedUserId)) + } + // ---- books ---- override suspend fun listBooks(filter: String?, sort: String, perPage: Int, page: Int): PbListResponse { + callLog += "listBooks" + nextListBooksError?.let { nextListBooksError = null; throw it } val cursor = filter?.substringAfter("'")?.substringBefore("'") val items = books.values.filter { cursor == null || it.updated > cursor }.sortedBy { it.updated } return PbListResponse(page = 1, totalPages = 1, totalItems = items.size, items = items) diff --git a/app/app/src/test/java/org/modg/bookshelf/data/repo/SyncEngineTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/repo/SyncEngineTest.kt index a048fc6..08ecd92 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/repo/SyncEngineTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/repo/SyncEngineTest.kt @@ -2,11 +2,16 @@ package org.modg.bookshelf.data.repo import androidx.room.Room import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.runTest import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue +import org.junit.Assert.fail import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -36,6 +41,11 @@ class SyncEngineTest { .build() settingsStore = SettingsStore(ApplicationProvider.getApplicationContext()) settingsStore.setServerUrl("https://example.test") + // DataStore's on-disk state leaks across Robolectric test methods in + // this class (see SettingsStoreTest's setUp for the same issue) — the + // one-time-repull flag being true from an earlier test would silently + // change the repull tests' behavior. + settingsStore.resetFullRepullDoneForTesting() api = FakePocketBaseApi() engine = SyncEngine( apiProvider = { api }, @@ -198,4 +208,124 @@ class SyncEngineTest { assertEquals("My local edit", db.bookDao().getByIdIncludingDeleted("book1")?.title) assertEquals(SyncState.PENDING_UPDATE, db.bookDao().getByIdIncludingDeleted("book1")?.syncState) } + + // ---------------------------------------------------------- auth refresh (2026-09-17) + + @Test + fun `sync refreshes the token before any push or pull, and persists the refreshed token`() = runTest { + db.bookcaseDao().upsert(bookcase("case1")) + api.refreshedToken = "brand-new-token" + api.refreshedUserId = "user-42" + + val result = engine.sync() + + assertTrue(result is SyncResult.Success) + val refreshIdx = api.callLog.indexOf("authRefresh") + val pushIdx = api.callLog.indexOf("createBookcase:case1") + assertTrue("expected authRefresh before any push, got ${api.callLog}", refreshIdx == 0 && refreshIdx < pushIdx) + assertEquals("brand-new-token", settingsStore.authToken.first()) + assertEquals("user-42", settingsStore.userId.first()) + } + + @Test + fun `a 401 on refresh is the 2026-09-17 incident's regression test - AuthExpired, token cleared, nothing else touched`() = runTest { + settingsStore.setAuthToken("stale-5-day-old-token") + settingsStore.setUserEmail("reader@example.com") + db.bookDao().upsert(book("book1", state = SyncState.PENDING_UPDATE).copy(title = "unsynced edit")) + api.authRefreshHttpError = httpError(401) + + val result = engine.sync() + + assertTrue(result is SyncResult.AuthExpired) + assertNull("token must be cleared so the app doesn't keep hammering the server with it", settingsStore.authToken.first()) + assertEquals("https://example.test", settingsStore.serverUrl.first()) + assertEquals("reader@example.com", settingsStore.userEmail.first()) + assertEquals("no push/pull call may have been made", listOf("authRefresh"), api.callLog) + val stillLocal = db.bookDao().getByIdIncludingDeleted("book1") + assertEquals("the PENDING_UPDATE book must survive an unauthenticated sync attempt intact", "unsynced edit", stillLocal?.title) + assertEquals(SyncState.PENDING_UPDATE, stillLocal?.syncState) + } + + @Test + fun `a 403 on refresh also reports AuthExpired`() = runTest { + api.authRefreshHttpError = httpError(403) + + assertTrue(engine.sync() is SyncResult.AuthExpired) + } + + @Test + fun `an IOException on refresh is a plain Failure and does not clear the token`() = runTest { + settingsStore.setAuthToken("still-good-token") + api.authRefreshIOException = java.io.IOException("no network") + + val result = engine.sync() + + assertTrue(result is SyncResult.Failure) + assertEquals("still-good-token", settingsStore.authToken.first()) + } + + @Test + fun `a 401 from a push call mid-sync reports AuthExpired and does not hard-delete the record`() = runTest { + db.bookDao().upsert(book("book1", state = SyncState.PENDING_UPDATE).copy(title = "keep me")) + api.updateBookErrors["book1"] = httpError(401) + + val result = engine.sync() + + assertTrue(result is SyncResult.AuthExpired) + val stillLocal = db.bookDao().getByIdIncludingDeleted("book1") + assertNotNull("a 401 must never be mistaken for the 404-means-deleted case", stillLocal) + assertEquals("keep me", stillLocal?.title) + assertEquals(SyncState.PENDING_UPDATE, stillLocal?.syncState) + } + + @Test + fun `CancellationException thrown during sync propagates instead of becoming a Failure`() = runTest { + api.authRefreshCancellation = CancellationException("scope cancelled") + + try { + engine.sync() + fail("expected CancellationException to propagate") + } catch (e: CancellationException) { + // expected + } + } + + // ---------------------------------------------------------- one-time full re-pull (2026-09-17) + + @Test + fun `first sync after the fix restores a book the incident hard-deleted, and marks the repull done`() = runTest { + // Mirrors the incident: the pull cursor is already past the lost row's + // `updated`, and the row is gone locally, but it is still intact on the server. + settingsStore.setCursor(SettingsStore.COLLECTION_BOOKS, "2024-06-01 00:00:00.000Z") + api.seedBook(BookDto(id = "lost-book", title = "Recovered", updated = "2024-01-01 00:00:00.000Z", created = "2024-01-01 00:00:00.000Z")) + assertNull(db.bookDao().getByIdIncludingDeleted("lost-book")) + + val result = engine.sync() + + assertTrue(result is SyncResult.Success) + assertEquals("Recovered", db.bookDao().getById("lost-book")?.title) + assertTrue(settingsStore.fullRepullDone.first()) + } + + @Test + fun `a sync whose pull fails leaves the one-time-repull flag unset so the next sync retries it`() = runTest { + api.nextListBooksError = httpError(500) + + val result = engine.sync() + + assertTrue(result is SyncResult.Failure) + assertFalse(settingsStore.fullRepullDone.first()) + } + + @Test + fun `once the repull flag is set, a normal sync no longer clears cursors`() = runTest { + settingsStore.setFullRepullDone() + settingsStore.setCursor(SettingsStore.COLLECTION_BOOKS, "2024-06-01 00:00:00.000Z") + // Older than the cursor: an incremental pull must NOT see this one. + api.seedBook(BookDto(id = "old-book", title = "Too old", updated = "2024-01-01 00:00:00.000Z", created = "2024-01-01 00:00:00.000Z")) + + engine.sync() + + assertNull(db.bookDao().getByIdIncludingDeleted("old-book")) + } } diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/library/LibrarySyncPresenterTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/library/LibrarySyncPresenterTest.kt index 8c1729b..dce48ba 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/library/LibrarySyncPresenterTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/library/LibrarySyncPresenterTest.kt @@ -20,6 +20,13 @@ class LibrarySyncPresenterTest { assertEquals("Sync failed — showing local library", state.label) } + @Test + fun `an expired session is a distinct status from a plain failure, not "Sync failed"`() { + val state = LibrarySyncPresenter.present(nowMillis = 1_000L, lastSyncMillis = null, isSyncing = false, lastOutcome = SyncOutcome.AUTH_EXPIRED) + assertEquals(SyncStatus.AuthExpired, state.status) + assertEquals("Signed out — sign in again to sync", state.label) + } + @Test fun `no server configured is offline, not an error`() { val state = LibrarySyncPresenter.present(nowMillis = 1_000L, lastSyncMillis = null, isSyncing = false, lastOutcome = SyncOutcome.NO_SERVER) diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt index c41df80..09bbb4a 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt @@ -97,6 +97,31 @@ class LibraryViewModelTest { initialShelfId = null, ) + // --- sync (2026-09-17 auth-expiry incident) --- + + @Test + fun `an expired session maps to the AuthExpired sync bar state, not a generic failure`() = runTest { + settingsStore.setServerUrl("https://example.test") + val api = org.modg.bookshelf.data.repo.FakePocketBaseApi() + api.authRefreshHttpError = org.modg.bookshelf.data.repo.httpError(401) + val vm = LibraryViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + computeDispatcher = Dispatchers.Main, + bookRepository = bookRepository, + locationRepository = locationRepository, + syncEngine = SyncEngine(ApiProvider { api }, db.bookDao(), db.bookcaseDao(), db.shelfDao(), settingsStore), + settingsStore = settingsStore, + bookSearchRepository = BookSearchRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), + metadataRepository = MetadataRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), + initialShelfId = null, + ) + + val bar = vm.uiState.first { it.syncBar.status == org.modg.bookshelf.ui.components.SyncStatus.AuthExpired }.syncBar + + assertEquals(org.modg.bookshelf.ui.components.SyncStatus.AuthExpired, bar.status) + assertEquals("Signed out — sign in again to sync", bar.label) + } + // --- typing vs submit --- @Test @@ -150,7 +175,12 @@ class LibraryViewModelTest { vm.onQueryChange("something else") assertEquals(OnlineSearchState.NotSearched, vm.onlineSearch.first()) - gate.countDown() // release the blocked background call so its thread isn't leaked + gate.countDown() // release the blocked background call + // Wait for the cancelled job to actually finish unwinding on its own thread before + // this test returns -- otherwise it can still be dispatching onto Dispatchers.Main + // after tearDown() (or even the next test's setUp()) touches it, which fails with + // "Dispatchers.Main is used concurrently with setting it" in whatever test runs next. + vm.onlineSearchJob?.join() } // --- partial and total source failure --- diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt index e52d144..680a7d2 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt @@ -71,6 +71,10 @@ class LibraryScreenPaparazziTest { @Test fun libraryEmptyLight() = snapshotBoth("library-empty") { Empty() } + /** 2026-09-17 incident: the sync bar's distinct "session expired" state — never "Sync failed". */ + @Test + fun libraryAuthExpiredLight() = snapshotBoth("library-auth-expired") { AuthExpired() } + @Test fun librarySearchBothSectionsLight() = snapshotBoth("library-search-both-sections") { SearchShell( @@ -170,6 +174,13 @@ class LibraryScreenPaparazziTest { @Composable private fun Empty() = Shell(books = emptyList(), syncStatus = SyncStatus.Offline, syncLabel = "Not synced yet") + @Composable + private fun AuthExpired() = Shell( + books = ScreenFixtures.books, + syncStatus = SyncStatus.AuthExpired, + syncLabel = "Signed out — sign in again to sync", + ) + /** Same chrome as [Shell] but with a non-blank query, so [LibraryScreen] shows the sectioned search-results view instead of the grid. */ @Composable private fun SearchShell(localMatches: List, onlineSearch: OnlineSearchState) { diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/screens/SettingsScreenPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/screens/SettingsScreenPaparazziTest.kt index 633c3f1..36b92ec 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/screens/SettingsScreenPaparazziTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/screens/SettingsScreenPaparazziTest.kt @@ -7,6 +7,8 @@ import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.ArrowBack import androidx.compose.material3.Icon import androidx.compose.material3.IconButton +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import androidx.compose.ui.unit.dp @@ -49,14 +51,21 @@ class SettingsScreenPaparazziTest { @Test fun settingsDiagnosticsEmptyLight() = snapshotBoth("settings-diagnostics-empty") { DiagnosticsEmpty() } - @Composable - private fun Populated() = Shell(crash = sampleCrash) + /** 2026-09-17 incident: "Your session expired. Sign in again." — never "Sync failed: HTTP 401". */ + @Test + fun settingsAuthExpiredLight() = snapshotBoth("settings-auth-expired") { AuthExpired() } @Composable - private fun DiagnosticsEmpty() = Shell(crash = null) + private fun Populated() = Shell(crash = sampleCrash, authExpired = false) @Composable - private fun Shell(crash: CrashReport?) { + private fun DiagnosticsEmpty() = Shell(crash = null, authExpired = false) + + @Composable + private fun AuthExpired() = Shell(crash = null, authExpired = true) + + @Composable + private fun Shell(crash: CrashReport?, authExpired: Boolean) { val lastSync = System.currentTimeMillis() - 2 * 60 * 1000L BookshelfScaffold( title = "Settings", @@ -64,7 +73,11 @@ class SettingsScreenPaparazziTest { IconButton(onClick = {}) { Icon(Icons.Filled.ArrowBack, contentDescription = "Back") } }, syncStatusBar = { - SyncStatusBar(status = SyncStatus.Synced, label = "Synced • ${formatLastSync(lastSync)}") + if (authExpired) { + SyncStatusBar(status = SyncStatus.AuthExpired, label = "Signed out — sign in again to sync") + } else { + SyncStatusBar(status = SyncStatus.Synced, label = "Synced • ${formatLastSync(lastSync)}") + } }, ) { innerPadding -> PaperSurface(modifier = Modifier.fillMaxWidth()) { @@ -82,7 +95,17 @@ class SettingsScreenPaparazziTest { SectionHeading("Sync") InfoRow(label = "Last synced", value = formatLastSync(lastSync)) - PrimaryButton(text = "Sync now", onClick = {}, modifier = Modifier.padding(top = 8.dp)) + if (authExpired) { + Text( + text = "Your session expired. Sign in again.", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.padding(top = 8.dp), + ) + PrimaryButton(text = "Sign in", onClick = {}, modifier = Modifier.padding(top = 8.dp)) + } else { + PrimaryButton(text = "Sync now", onClick = {}, modifier = Modifier.padding(top = 8.dp)) + } GoldDivider(modifier = Modifier.padding(vertical = 16.dp)) diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/settings/SettingsViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/settings/SettingsViewModelTest.kt new file mode 100644 index 0000000..a64c3de --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/ui/settings/SettingsViewModelTest.kt @@ -0,0 +1,116 @@ +package org.modg.bookshelf.ui.settings + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.modg.bookshelf.data.local.BookshelfDatabase +import org.modg.bookshelf.data.prefs.SettingsStore +import org.modg.bookshelf.data.remote.ApiProvider +import org.modg.bookshelf.data.repo.AuthRepository +import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.FakePocketBaseApi +import org.modg.bookshelf.data.repo.SyncEngine +import org.modg.bookshelf.data.repo.httpError +import org.modg.bookshelf.diagnostics.CrashReporter +import org.modg.bookshelf.ui.components.SyncStatus +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * 2026-09-17 incident: [SettingsViewModel.syncNow] used to surface an expired + * session as "Sync failed: HTTP 401", which reads as "try again later" rather + * than "you must sign in again". Covers the distinct [SyncResult.AuthExpired] + * mapping this class's SPEC section adds. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class SettingsViewModelTest { + + private lateinit var db: BookshelfDatabase + private lateinit var settingsStore: SettingsStore + private lateinit var api: FakePocketBaseApi + private lateinit var context: android.content.Context + + @Before + fun setUp() { + Dispatchers.setMain(UnconfinedTestDispatcher()) + context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, BookshelfDatabase::class.java) + .allowMainThreadQueries() + .build() + settingsStore = SettingsStore(context) + api = FakePocketBaseApi() + } + + @After + fun tearDown() { + db.close() + Dispatchers.resetMain() + } + + private fun viewModel(): SettingsViewModel = SettingsViewModel( + authRepository = AuthRepository(ApiProvider { api }, settingsStore), + settingsStore = settingsStore, + bookRepository = BookRepository(db.bookDao(), context), + syncEngine = SyncEngine(ApiProvider { api }, db.bookDao(), db.bookcaseDao(), db.shelfDao(), settingsStore), + crashReporter = CrashReporter(context), + ) + + @Test + fun `a plain sync failure keeps the generic error message`() = runTest { + settingsStore.setServerUrl("https://example.test") + api.authRefreshIOException = java.io.IOException("no network") + val vm = viewModel() + + vm.syncNow() + + // Not "!isSyncing" — that's also (trivially) true of the very first + // combined emission, before this sync's failure has landed. + val state = vm.uiState.first { it.syncError != null } + assertFalse(state.syncAuthExpired) + assertEquals(SyncStatus.Error, state.syncStatus) + } + + @Test + fun `an expired session maps to syncAuthExpired, not a generic sync error`() = runTest { + settingsStore.setServerUrl("https://example.test") + api.authRefreshHttpError = httpError(401) + val vm = viewModel() + + vm.syncNow() + + val state = vm.uiState.first { it.syncAuthExpired } + assertTrue(state.syncAuthExpired) + assertEquals(SyncStatus.AuthExpired, state.syncStatus) + assertEquals(null, state.syncError) + } + + @Test + fun `a later successful sync clears a previous auth-expired state`() = runTest { + settingsStore.setServerUrl("https://example.test") + api.authRefreshHttpError = httpError(401) + val vm = viewModel() + vm.syncNow() + vm.uiState.first { it.syncAuthExpired } + + api.authRefreshHttpError = null + vm.syncNow() + + val state = vm.uiState.first { !it.isSyncing && !it.syncAuthExpired } + assertFalse(state.syncAuthExpired) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/setup/SetupViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/setup/SetupViewModelTest.kt new file mode 100644 index 0000000..63cd4a3 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/ui/setup/SetupViewModelTest.kt @@ -0,0 +1,63 @@ +package org.modg.bookshelf.ui.setup + +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.modg.bookshelf.data.prefs.SettingsStore +import org.modg.bookshelf.data.remote.ApiProvider +import org.modg.bookshelf.data.repo.AuthRepository +import org.modg.bookshelf.data.repo.FakePocketBaseApi +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * 2026-09-17 incident: after an auth expiry clears only the token (see + * SettingsStore.clearAuthToken), re-signing-in should be typing the password + * only. Covers that the setup screen's ViewModel pre-fills what's already + * stored, whether that's from a first-run-in-progress or a post-expiry return trip. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class SetupViewModelTest { + + private lateinit var settingsStore: SettingsStore + private lateinit var authRepository: AuthRepository + + @Before + fun setUp() { + Dispatchers.setMain(UnconfinedTestDispatcher()) + settingsStore = SettingsStore(ApplicationProvider.getApplicationContext()) + authRepository = AuthRepository(ApiProvider { FakePocketBaseApi() }, settingsStore) + } + + @After + fun tearDown() { + Dispatchers.resetMain() + } + + @Test + fun `pre-fills the stored server URL and email when present`() = runTest { + settingsStore.setServerUrl("https://library.montanaro.home") + settingsStore.setUserEmail("reader@example.com") + + val viewModel = SetupViewModel(authRepository) + + val state = viewModel.uiState.first { + it.serverUrl == "https://library.montanaro.home" && it.email == "reader@example.com" + } + assertEquals("https://library.montanaro.home", state.serverUrl) + assertEquals("reader@example.com", state.email) + assertEquals("", state.password) // never pre-filled — SPEC "typing the password only" + } +} diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryAuthExpiredLight_library-auth-expired-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryAuthExpiredLight_library-auth-expired-dark.png new file mode 100644 index 0000000..eda2570 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryAuthExpiredLight_library-auth-expired-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryAuthExpiredLight_library-auth-expired-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryAuthExpiredLight_library-auth-expired-light.png new file mode 100644 index 0000000..606f8df Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryAuthExpiredLight_library-auth-expired-light.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_SettingsScreenPaparazziTest_settingsAuthExpiredLight_settings-auth-expired-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_SettingsScreenPaparazziTest_settingsAuthExpiredLight_settings-auth-expired-dark.png new file mode 100644 index 0000000..b6d90cb Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_SettingsScreenPaparazziTest_settingsAuthExpiredLight_settings-auth-expired-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_SettingsScreenPaparazziTest_settingsAuthExpiredLight_settings-auth-expired-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_SettingsScreenPaparazziTest_settingsAuthExpiredLight_settings-auth-expired-light.png new file mode 100644 index 0000000..2d70c38 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_SettingsScreenPaparazziTest_settingsAuthExpiredLight_settings-auth-expired-light.png differ diff --git a/logs/M-authexpiry-fix.sid b/logs/M-authexpiry-fix.sid new file mode 100644 index 0000000..5003bfb --- /dev/null +++ b/logs/M-authexpiry-fix.sid @@ -0,0 +1 @@ +d109abe5-9e54-4496-912d-b200c51d373e diff --git a/logs/M-authexpiry-fix.summary b/logs/M-authexpiry-fix.summary new file mode 100644 index 0000000..234eec6 --- /dev/null +++ b/logs/M-authexpiry-fix.summary @@ -0,0 +1,18 @@ +=== M-authexpiry-fix attempts=1 quota_waits=0 hard_fails=0 === +cost=$1.3351339999999998 turns=44 err=? +--- result (tail) --- +ar onlineSearchJob` to `internal var onlineSearchJob`, matching the file's existing convention (same reasoning already documented on `performOnlineSearch`) of loosening visibility so tests can synchronize on it directly. +- `app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt`: after `gate.countDown()`, added `vm.onlineSearchJob?.join()` so the test genuinely waits for the cancelled job to finish before returning, with a comment explaining why. + +No changes to the feature logic, SPEC, build files, or anything else — nothing else in the gate's failure pointed at the auth-expiry implementation itself. + +**Verification:** +- `assembleDebug` → exit 0. +- `testDebugUnitTest` → exit 0, reran 4 more times with `--rerun` after the fix, all clean (previously failed on run 2 of 2 before the fix). +- Test count: 355 passed, 2 skipped, 0 failures (baseline was 337) — via summed XML. +- `verifyPaparazziDebug` → exit 0. +- Recorded the 4 new Paparazzi snapshots (`library-auth-expired-{light,dark}`, `settings-auth-expired-{light,dark}`) and viewed all four PNGs: library grid shows the "Signed out — sign in again to sync" sync bar; settings shows "Your session expired. Sign in again." with a "Sign in" button. Both match spec's design language in light and dark, and `git status` confirmed no other snapshots were touched by the record run. +- Grepped a forced recompile for Kotlin warnings in touched files: none. +- Final full sequence run in order (assembleDebug → testDebugUnitTest → verifyPaparazziDebug): all exit 0. + +I did not re-review the original worker's auth-expiry design/logic (token refresh, full re-pull, AuthExpired propagation) since none of that was implicated by the gate failure — per instructions, I only touched what was making the gate red. diff --git a/logs/M-authexpiry.sid b/logs/M-authexpiry.sid new file mode 100644 index 0000000..c22419b --- /dev/null +++ b/logs/M-authexpiry.sid @@ -0,0 +1 @@ +6bcc6e7a-a2a5-449e-b75a-a62eea1cc657 diff --git a/logs/M-authexpiry.summary b/logs/M-authexpiry.summary new file mode 100644 index 0000000..97893ef --- /dev/null +++ b/logs/M-authexpiry.summary @@ -0,0 +1,13 @@ +=== M-authexpiry attempts=1 quota_waits=0 hard_fails=0 === +cost=$4.886013000000001 turns=123 err=? +--- result (tail) --- +ption` handler before it can reach a `code() == 404` branch, so it can never be misread as "deleted." A local dirty (`PENDING_*`) record is never touched by an `AuthExpired` result — the exception aborts the whole `sync()` before the offending DAO write and before any subsequent push/pull calls run. + +**Full re-pull safety:** relied on `SyncEngine.shouldApplyRemote` (SyncEngine.kt, in the `applyIncomingBook`/`Shelf`/`Bookcase` helpers) — no local record → always inserted; a `SYNCED` local record only loses to a remote that's `>=` its own `updated`; a dirty (`PENDING_*`) local record only loses to a remote strictly newer than its own `updated`. So a full re-pull can only ever add back rows or apply genuinely newer server state — it can't clobber an unsynced local edit. + +**Deviations:** +- I did not re-run baseline `testDebugUnitTest` before touching code — I trusted the prompt's stated 337/2-skipped baseline. I did do the required post-change runs. +- Added a small `internal` test-only method (`SettingsStore.resetFullRepullDoneForTesting`) purely to prevent DataStore's known cross-test-method state leak (the same issue `SettingsStoreTest` already documents) from corrupting the new repull tests. No production caller. +- Dropped one planned SetupViewModel test ("starts blank on first run") because it wasn't required and was fragile against the same cross-test DataStore leak; kept only the required pre-fill test, which is self-contained. + +**Out of scope but noticed:** several pre-existing Kotlin deprecation warnings (`Icons.Filled.ArrowBack`, `Icons.Outlined.Sort`, `centerAlignedTopAppBarColors`, `Modifier.menuAnchor()`, `LocalLifecycleOwner`) and a nullable-`ClassLoader` warning in some metadata client tests — none in files I touched, all pre-existing, left alone. diff --git a/tasks/M-authexpiry-fix.txt b/tasks/M-authexpiry-fix.txt new file mode 100644 index 0000000..659fe40 --- /dev/null +++ b/tasks/M-authexpiry-fix.txt @@ -0,0 +1,14 @@ +A previous worker implemented the task in ~/bookshelf/tasks/M-authexpiry.txt and left the +working tree FAILING its acceptance gate. Read, in order: ~/bookshelf/docs/SPEC.md, +~/bookshelf/tasks/M-authexpiry.txt (the original instructions — every constraint in it binds +you too), ~/bookshelf/logs/M-authexpiry.summary (that worker's report), then the gate output at +~/bookshelf/logs/gate-M-authexpiry.log. Lines starting "GATE:" and non-zero exits are the failures. + +Fix ONLY what makes the gate red. Do not redesign or re-implement the feature. The +test count must end ABOVE 337 with zero failures; do not delete or @Ignore tests to +get there. If a Paparazzi verify failed because snapshots were never recorded, record +them and LOOK at the PNGs before accepting them. + +Run in the FOREGROUND (never background a build): ./tasks/gw assembleDebug, +./tasks/gw testDebugUnitTest, ./tasks/gw verifyPaparazziDebug. Do not git commit/add/push. +Report what was wrong, what you changed, and the verbatim output tails.