diff --git a/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt b/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt index b8ea20a..0f8517f 100644 --- a/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt +++ b/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt @@ -93,11 +93,24 @@ class AppContainer(private val context: Context) { // server. A call timeout is load-bearing here: with none, a stalled // connection hangs on OkHttp's default (unbounded) socket timeouts, and the // user is standing at a bookshelf waiting on it. + // + // These were 12s/10s/10s and were MANUFACTURING failures. Measuring the exact + // Open Library call 30 times on 2026-09-09 (docs/METADATA-SOURCES.md) found + // successful requests with a median of 4.3s but a long tail — p90 9.2s, max + // 22.0s, and one connect phase alone of 19.6s. Two of 26 successes exceeded the + // old 12s call timeout, so ~8% of lookups that were about to work were being + // cancelled and reported to the user as "couldn't be reached". + // + // Raising these does NOT slow the failure path: every observed failure was a + // TLS-stage reset returning in under 2.5s, and a socket that is going to break + // breaks long before any of these limits. The timeouts only ever bound the + // slow-success tail, which is precisely what we want to stop truncating. + // 25s > the 22.0s worst observed success, with room to spare. private val metadataHttpClient: OkHttpClient by lazy { OkHttpClient.Builder() - .callTimeout(12, TimeUnit.SECONDS) - .connectTimeout(10, TimeUnit.SECONDS) - .readTimeout(10, TimeUnit.SECONDS) + .callTimeout(25, TimeUnit.SECONDS) + .connectTimeout(20, TimeUnit.SECONDS) + .readTimeout(20, TimeUnit.SECONDS) .build() } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt index ff61fc3..50182b3 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/GoogleBooksClient.kt @@ -1,7 +1,6 @@ package org.modg.bookshelf.data.metadata import java.io.IOException -import java.net.SocketTimeoutException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.serialization.SerializationException @@ -20,7 +19,18 @@ class GoogleBooksClient( ) { private val json = Json(from = json) { ignoreUnknownKeys = true } - suspend fun lookup(isbn13: String): SourceResult = withContext(Dispatchers.IO) { fetch(isbn13) } + /** + * Retries transient failures per [RetryPolicy]. Note this source's standing + * failure — keyless requests share one exhausted global quota and answer 429, + * which [RetryPolicy] deliberately does NOT retry, so today this costs nothing + * and changes nothing here. See docs/METADATA-SOURCES.md. + */ + suspend fun lookup(isbn13: String): SourceResult = + withContext(Dispatchers.IO) { withRetry { fetch(isbn13) } } + + /** Single un-retried attempt, for tests that need to count calls. */ + internal suspend fun lookupOnce(isbn13: String): SourceResult = + withContext(Dispatchers.IO) { fetch(isbn13) } private fun fetch(isbn13: String): SourceResult = try { val request = Request.Builder() @@ -29,10 +39,8 @@ class GoogleBooksClient( httpClient.newCall(request).execute().use { response -> classify(response.code, response.body.string()) } - } catch (e: SocketTimeoutException) { - SourceResult.Failed("timeout") } catch (e: IOException) { - SourceResult.Failed("network error") + SourceResult.fromException(e) } /** @@ -42,16 +50,16 @@ class GoogleBooksClient( * can never disagree about what a body means. */ internal fun classify(httpCode: Int, body: String?): SourceResult { - if (httpCode !in 200..299) return SourceResult.Failed("http $httpCode") - if (body.isNullOrBlank()) return SourceResult.Failed("empty body") + if (httpCode !in 200..299) return SourceResult.fromHttpCode(httpCode) + if (body.isNullOrBlank()) return SourceResult.Failed("empty body", FailureKind.MALFORMED) return try { val dto = json.decodeFromString(GoogleBooksResponseDto.serializer(), body) val metadata = dto.items.firstOrNull()?.volumeInfo?.toBookMetadata() if (metadata != null) SourceResult.Found(metadata) else SourceResult.NotFound } catch (e: SerializationException) { - SourceResult.Failed("malformed json") + SourceResult.Failed("malformed json", FailureKind.MALFORMED) } catch (e: IllegalArgumentException) { - SourceResult.Failed("malformed json") + SourceResult.Failed("malformed json", FailureKind.MALFORMED) } } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/OpenLibraryClient.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/OpenLibraryClient.kt index fefe696..51c0018 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/OpenLibraryClient.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/OpenLibraryClient.kt @@ -1,7 +1,6 @@ package org.modg.bookshelf.data.metadata import java.io.IOException -import java.net.SocketTimeoutException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.serialization.SerializationException @@ -23,7 +22,17 @@ class OpenLibraryClient( // Real responses carry fields this DTO doesn't model; never let an unknown key throw. private val json = Json(from = json) { ignoreUnknownKeys = true } - suspend fun lookup(isbn13: String): SourceResult = withContext(Dispatchers.IO) { fetch(isbn13) } + /** + * Retries transient failures per [RetryPolicy]. Measured against the live API, + * 13% of requests fail at the TLS stage in well under a second while successful + * ones take seconds — so a retry is nearly free and removes most of that 13%. + */ + suspend fun lookup(isbn13: String): SourceResult = + withContext(Dispatchers.IO) { withRetry { fetch(isbn13) } } + + /** Single un-retried attempt, for tests that need to count calls. */ + internal suspend fun lookupOnce(isbn13: String): SourceResult = + withContext(Dispatchers.IO) { fetch(isbn13) } private fun fetch(isbn13: String): SourceResult = try { val request = Request.Builder() @@ -32,10 +41,8 @@ class OpenLibraryClient( httpClient.newCall(request).execute().use { response -> classify(response.code, response.body.string(), isbn13) } - } catch (e: SocketTimeoutException) { - SourceResult.Failed("timeout") } catch (e: IOException) { - SourceResult.Failed("network error") + SourceResult.fromException(e) } /** @@ -45,16 +52,16 @@ class OpenLibraryClient( * can never disagree about what a body means. */ internal fun classify(httpCode: Int, body: String?, isbn13: String): SourceResult { - if (httpCode !in 200..299) return SourceResult.Failed("http $httpCode") - if (body.isNullOrBlank()) return SourceResult.Failed("empty body") + if (httpCode !in 200..299) return SourceResult.fromHttpCode(httpCode) + if (body.isNullOrBlank()) return SourceResult.Failed("empty body", FailureKind.MALFORMED) return try { val root = json.parseToJsonElement(body).jsonObject val entry = root["ISBN:$isbn13"]?.jsonObject ?: return SourceResult.NotFound SourceResult.Found(json.decodeFromJsonElement(entry).toBookMetadata(isbn13)) } catch (e: SerializationException) { - SourceResult.Failed("malformed json") + SourceResult.Failed("malformed json", FailureKind.MALFORMED) } catch (e: IllegalArgumentException) { - SourceResult.Failed("malformed json") + SourceResult.Failed("malformed json", FailureKind.MALFORMED) } } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt new file mode 100644 index 0000000..372daf1 --- /dev/null +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/RetryPolicy.kt @@ -0,0 +1,110 @@ +package org.modg.bookshelf.data.metadata + +import kotlin.random.Random +import kotlinx.coroutines.delay + +/** + * When a failed lookup attempt is worth repeating, and how long to wait first. + * + * Grounded in a measurement of the live Open Library API from 2026-09-09 — 30 + * requests, the exact call [OpenLibraryClient] makes (see docs/METADATA-SOURCES.md): + * + * - 13% of requests failed, every one of them a TLS-stage connection reset. + * - Failures were FAST: 0.23s, 0.31s, 0.59s, 2.46s. + * - Successes were SLOW and long-tailed: median 4.3s, p90 9.2s, max 22.0s. + * + * That asymmetry drives every constant here. A retry costs roughly a fifth of a + * second of the user's time and turns a 13% failure rate into ~1.7% at two + * attempts and ~0.2% at three, which is why the backoff is short rather than the + * conventional exponential-with-seconds. It is also why [isRetryable] refuses to + * repeat a [FailureKind.TIMEOUT]: a timeout means we already spent the full budget + * on that attempt, so repeating it risks tripling the wait for a user standing at + * a bookshelf, and the measurement says slow requests usually eventually succeed + * rather than fail — the fix for those is a generous timeout, not another attempt. + */ +object RetryPolicy { + + /** One original attempt plus two retries. Beyond this the marginal gain is noise. */ + const val MAX_ATTEMPTS = 3 + + /** + * Stop starting NEW attempts once this much time has gone into a single source. + * A backstop against pathological cases (every attempt hitting the slow tail), + * not a normal-path limit. It is deliberately checked only BETWEEN attempts — + * an in-flight request is never cancelled, because the 22-second request in the + * sample was a successful one and killing it would manufacture exactly the + * failure this whole change exists to remove. + */ + const val TOTAL_BUDGET_MILLIS = 30_000L + + /** + * [FailureKind.TRANSPORT] and [FailureKind.SERVER_ERROR] are transient and + * cheap to re-ask. The rest are not: + * - TIMEOUT — the budget is already spent; see the class KDoc. + * - RATE_LIMITED — the source is explicitly asking us to stop. Hammering a + * quota is how an intermittent block becomes a permanent one, + * and METADATA-SOURCES.md records that happening to this + * project's IP during research. When the Google Books API key + * lands, revisit this: a keyed 429 is a per-second rate limit + * and IS worth one Retry-After-respecting retry, unlike + * today's keyless daily-quota 429, which never clears. + * - CLIENT_ERROR — an identical request gets an identical answer. + * - MALFORMED — same bytes, same parse failure. + */ + fun isRetryable(kind: FailureKind): Boolean = + kind == FailureKind.TRANSPORT || kind == FailureKind.SERVER_ERROR + + /** + * Backoff before attempt number [nextAttempt] (2-based: the wait before the + * first retry is `backoffMillis(2)`). 250ms then 750ms, plus up to 40% jitter + * so that two sources — or two phones in the same house — cannot fall into + * lockstep and hammer a recovering server in unison. + */ + fun backoffMillis(nextAttempt: Int, random: Random = Random.Default): Long { + val base = when (nextAttempt) { + 2 -> 250L + else -> 750L + } + return base + random.nextLong(0, (base * 0.4).toLong().coerceAtLeast(1)) + } +} + +/** + * Runs [attempt] until it succeeds, fails un-retryably, or runs out of attempts or + * budget. Returns the LAST result, so the caller always sees a real outcome rather + * than a synthesised one. + * + * A [SourceResult.Failed] that survives retrying has its attempt count appended to + * [SourceResult.Failed.reason] ("tls connection reset, 3 attempts"). That string is + * shown to the user on the scan sheet and is the only diagnostic we get back from a + * real phone — "failed once" and "failed three times in a row" are very different + * stories about the network, and without this they are indistinguishable. + * + * [sleep] and [nowMillis] are injectable purely so tests can run the real policy + * with no wall-clock delay; production callers use the defaults. + */ +suspend fun withRetry( + maxAttempts: Int = RetryPolicy.MAX_ATTEMPTS, + budgetMillis: Long = RetryPolicy.TOTAL_BUDGET_MILLIS, + random: Random = Random.Default, + nowMillis: () -> Long = { System.currentTimeMillis() }, + sleep: suspend (Long) -> Unit = { delay(it) }, + attempt: suspend () -> SourceResult, +): SourceResult { + val started = nowMillis() + var last: SourceResult = attempt() + var attemptsMade = 1 + + while (attemptsMade < maxAttempts) { + val failure = last as? SourceResult.Failed ?: return last + if (!RetryPolicy.isRetryable(failure.kind)) break + if (nowMillis() - started >= budgetMillis) break + + sleep(RetryPolicy.backoffMillis(attemptsMade + 1, random)) + last = attempt() + attemptsMade++ + } + + val failure = last as? SourceResult.Failed ?: return last + return if (attemptsMade > 1) failure.copy(reason = "${failure.reason}, $attemptsMade attempts") else failure +} diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt index bd4ed25..88bb71c 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/SourceResult.kt @@ -1,5 +1,39 @@ package org.modg.bookshelf.data.metadata +import java.io.IOException +import java.io.InterruptedIOException +import java.net.ConnectException +import java.net.SocketException +import java.net.SocketTimeoutException +import java.net.UnknownHostException +import javax.net.ssl.SSLException +import javax.net.ssl.SSLHandshakeException + +/** + * Why a source failed, in the only terms that matter to a caller: is it worth + * asking again? [RetryPolicy] is the single place that decides, so the answer + * cannot drift between the two clients. + */ +enum class FailureKind { + /** Never got an answer: DNS, refused connection, reset socket, failed TLS handshake. */ + TRANSPORT, + + /** We gave up waiting. Distinct from [TRANSPORT] because the budget is already spent. */ + TIMEOUT, + + /** HTTP 429. The source is telling us to stop asking; asking harder is the wrong move. */ + RATE_LIMITED, + + /** HTTP 5xx — the source's problem, and usually a passing one. */ + SERVER_ERROR, + + /** HTTP 4xx other than 429. Repeating an identical request cannot change the answer. */ + CLIENT_ERROR, + + /** 2xx whose body we could not parse. Deterministic: the same bytes will fail again. */ + MALFORMED, +} + /** * Per-source lookup outcome (SPEC.md "Book metadata lookup": "Lookup outcome is * THREE-WAY, never a bare null"). [OpenLibraryClient] and [GoogleBooksClient] each @@ -14,10 +48,54 @@ sealed interface SourceResult { data object NotFound : SourceResult /** - * The source could not be asked, or its answer couldn't be trusted: non-2xx, - * timeout, transport error, or a body that didn't parse. [reason] is a short - * diagnostic ("http 429", "timeout", "malformed json") for logs — never shown - * to the user verbatim. + * The source could not be asked, or its answer couldn't be trusted. [reason] is + * a short diagnostic ("http 429", "tls reset", "malformed json") that is shown + * to the user as supplementary detail on the scan sheet and is our ONLY + * diagnostic channel from a real phone — so it names the specific failure, not + * a generic one. [kind] is the same fact in a form [RetryPolicy] can act on; + * nothing should ever parse [reason] to recover it. */ - data class Failed(val reason: String) : SourceResult + data class Failed(val reason: String, val kind: FailureKind = FailureKind.TRANSPORT) : SourceResult + + companion object { + /** + * Maps a thrown [IOException] to a specific failure. Both clients call this + * so they can never disagree, and so a new exception type gets classified + * once rather than twice. + * + * Measured against the live Open Library API on 2026-09-09 (30 requests): + * every observed failure was a TLS-stage `Connection reset by peer`, and all + * four came back in under 2.5s while successful requests took a median of + * 4.3s. Failures are cheap and fast; that asymmetry is the whole reason + * retrying is worth doing, and why the timeouts are set as generously as + * they are in `AppContainer.metadataHttpClient`. + * + * Order matters: [SocketTimeoutException] is an [InterruptedIOException] and + * [SSLHandshakeException] is an [SSLException], so the specific arms come + * first. OkHttp reports a blown `callTimeout` as a bare + * [InterruptedIOException], which is why that arm exists at all. + */ + fun fromException(e: IOException): Failed = when (e) { + is SocketTimeoutException -> Failed("timeout", FailureKind.TIMEOUT) + is InterruptedIOException -> Failed("timeout", FailureKind.TIMEOUT) + is UnknownHostException -> Failed("dns lookup failed", FailureKind.TRANSPORT) + is SSLHandshakeException -> Failed("tls handshake failed", FailureKind.TRANSPORT) + is SSLException -> Failed("tls connection reset", FailureKind.TRANSPORT) + is ConnectException -> Failed("connection refused", FailureKind.TRANSPORT) + is SocketException -> Failed("connection reset", FailureKind.TRANSPORT) + else -> Failed("network error (${e.javaClass.simpleName})", FailureKind.TRANSPORT) + } + + /** + * Maps a non-2xx HTTP status to a specific failure. 429 is called out + * separately from the rest of 4xx because it is the one client error that is + * about us rather than about the request, and because it is currently + * Google Books' permanent state — see docs/METADATA-SOURCES.md. + */ + fun fromHttpCode(code: Int): Failed = when { + code == 429 -> Failed("http 429 (rate limited)", FailureKind.RATE_LIMITED) + code in 500..599 -> Failed("http $code (server error)", FailureKind.SERVER_ERROR) + else -> Failed("http $code", FailureKind.CLIENT_ERROR) + } + } } 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 b7a2817..35ef865 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 @@ -26,6 +26,7 @@ class SettingsStore(private val context: Context) { val USER_ID = stringPreferencesKey("user_id") val USER_EMAIL = stringPreferencesKey("user_email") val LAST_SYNC_TIME = longPreferencesKey("last_sync_time") + val LAST_SHELF_ID = stringPreferencesKey("last_shelf_id") fun cursor(collection: String) = stringPreferencesKey("cursor_$collection") } @@ -35,6 +36,13 @@ 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] } + /** + * 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). + * Never written for "Not shelved"; see [SettingsStore] callers. + */ + val lastShelfId: Flow = context.dataStore.data.map { it[Keys.LAST_SHELF_ID] } + fun cursorFor(collection: String): Flow = context.dataStore.data.map { it[Keys.cursor(collection)] } @@ -62,12 +70,25 @@ class SettingsStore(private val context: Context) { context.dataStore.edit { it[Keys.LAST_SYNC_TIME] = epochMillis } } - /** Sign out: drop the token/user identity but keep the server URL — no need to re-enter it. */ + suspend fun setLastShelfId(shelfId: String) { + context.dataStore.edit { it[Keys.LAST_SHELF_ID] = shelfId } + } + + suspend fun clearLastShelfId() { + context.dataStore.edit { it.remove(Keys.LAST_SHELF_ID) } + } + + /** + * Sign out: drop the token/user identity but keep the server URL — no need to + * re-enter it. Also drops the remembered shelf: a shared library's other + * account shouldn't have its shelving habit leak into this one's session. + */ suspend fun clearAuth() { context.dataStore.edit { it.remove(Keys.AUTH_TOKEN) it.remove(Keys.USER_ID) it.remove(Keys.USER_EMAIL) + it.remove(Keys.LAST_SHELF_ID) } } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/components/ShelfPickerSheet.kt b/app/app/src/main/java/org/modg/bookshelf/ui/components/ShelfPickerSheet.kt new file mode 100644 index 0000000..067ce86 --- /dev/null +++ b/app/app/src/main/java/org/modg/bookshelf/ui/components/ShelfPickerSheet.kt @@ -0,0 +1,180 @@ +package org.modg.bookshelf.ui.components + +import androidx.compose.foundation.clickable +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.verticalScroll +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.outlined.Check +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.ModalBottomSheet +import androidx.compose.material3.Text +import androidx.compose.material3.rememberModalBottomSheetState +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.font.FontStyle +import androidx.compose.ui.text.font.FontWeight +import androidx.compose.ui.unit.dp +import org.modg.bookshelf.data.local.BookcaseEntity +import org.modg.bookshelf.data.local.ShelfEntity + +/** + * The grouped shelf picker shared by the scan screen's save sheet and the + * detail screen's location picker. Replaces the old flat "Bookcase • Shelf" + * [androidx.compose.material3.DropdownMenu] — which stopped being usable once + * a library had more than a couple of bookcases — with a scrolling + * [ModalBottomSheet] grouped one section per bookcase. A bookcase is not + * itself a place a book can sit, so its header is a non-selectable label; + * only the shelves listed under it are choices. + */ +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun ShelfPickerSheet( + bookcases: List, + shelves: List, + selectedShelfId: String?, + recentShelfId: String?, + onShelfSelected: (String?) -> Unit, + onDismissRequest: () -> Unit, +) { + ModalBottomSheet(onDismissRequest = onDismissRequest, sheetState = rememberModalBottomSheetState()) { + ShelfPickerContent( + bookcases = bookcases, + shelves = shelves, + selectedShelfId = selectedShelfId, + recentShelfId = recentShelfId, + onShelfSelected = { shelfId -> + onShelfSelected(shelfId) + onDismissRequest() + }, + ) + } +} + +/** + * The picker's contents, split out from [ShelfPickerSheet] so it can be rendered + * directly (Paparazzi has no real Window/scrim behind a headless [ModalBottomSheet], + * same problem [org.modg.bookshelf.ui.screens.ScanScreenPaparazziTest]'s class doc + * describes for the camera preview). Internal rather than private so that test can + * reach it. + */ +@Composable +internal fun ShelfPickerContent( + bookcases: List, + shelves: List, + selectedShelfId: String?, + recentShelfId: String?, + onShelfSelected: (String?) -> Unit, +) { + val recent = resolveRecentShelf(recentShelfId, shelves, bookcases) + val recentShelf = recent?.first + val recentBookcase = recent?.second + + Column( + modifier = Modifier + .fillMaxWidth() + .verticalScroll(rememberScrollState()) + .padding(horizontal = 16.dp, vertical = 8.dp), + ) { + // Omitted entirely when there is no remembered shelf, or it no longer exists + // (deleted since it was last used) — a dangling "Recent" entry would be worse + // than no shortcut at all. + if (recentShelf != null && recentBookcase != null) { + SectionHeader(text = "Recent") + ShelfRow( + label = "${recentBookcase.name} • ${recentShelf.label}", + selected = selectedShelfId == recentShelf.id, + onClick = { onShelfSelected(recentShelf.id) }, + ) + GoldDivider(modifier = Modifier.padding(vertical = 12.dp)) + } + + ShelfRow( + label = "Not shelved", + selected = selectedShelfId == null, + onClick = { onShelfSelected(null) }, + ) + + bookcases.sortedBy { it.position }.forEach { bookcase -> + GoldDivider(modifier = Modifier.padding(vertical = 12.dp)) + SectionHeader(text = bookcase.name) + val bookcaseShelves = shelves.filter { it.bookcaseId == bookcase.id }.sortedBy { it.position } + if (bookcaseShelves.isEmpty()) { + // So an empty bookcase reads as "no shelves yet", not a rendering bug. + Text( + text = "No shelves yet", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + fontStyle = FontStyle.Italic, + modifier = Modifier.padding(vertical = 8.dp), + ) + } else { + bookcaseShelves.forEach { shelf -> + ShelfRow( + label = shelf.label, + selected = selectedShelfId == shelf.id, + onClick = { onShelfSelected(shelf.id) }, + ) + } + } + } + } +} + +@Composable +private fun SectionHeader(text: String) { + Text( + text = text, + style = MaterialTheme.typography.titleSmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(top = 8.dp, bottom = 4.dp), + ) +} + +@Composable +private fun ShelfRow(label: String, selected: Boolean, onClick: () -> Unit) { + Row( + modifier = Modifier + .fillMaxWidth() + .clickable(onClick = onClick) + .padding(vertical = 12.dp), + verticalAlignment = Alignment.CenterVertically, + ) { + Text( + text = label, + style = MaterialTheme.typography.bodyLarge, + fontWeight = if (selected) FontWeight.Bold else FontWeight.Normal, + color = if (selected) MaterialTheme.colorScheme.primary else MaterialTheme.colorScheme.onSurface, + modifier = Modifier.weight(1f), + ) + if (selected) { + Icon( + imageVector = Icons.Outlined.Check, + contentDescription = null, + tint = MaterialTheme.colorScheme.primary, + ) + } + } +} + +/** + * Resolves [recentShelfId] to the shelf + bookcase it names, or null when there is + * nothing remembered, or the remembered shelf was deleted since — the two cases the + * "Recent" section must be omitted for. Pure so it's directly unit-testable without + * standing up Compose. + */ +internal fun resolveRecentShelf( + recentShelfId: String?, + shelves: List, + bookcases: List, +): Pair? { + val shelf = recentShelfId?.let { id -> shelves.find { it.id == id } } ?: return null + val bookcase = bookcases.find { it.id == shelf.bookcaseId } ?: return null + return shelf to bookcase +} diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailScreen.kt index 99f5bc6..6cced92 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailScreen.kt @@ -19,8 +19,6 @@ import androidx.compose.material.icons.outlined.ExpandLess import androidx.compose.material.icons.outlined.ExpandMore import androidx.compose.material.icons.outlined.LocationOn import androidx.compose.material.icons.outlined.Save -import androidx.compose.material3.DropdownMenu -import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme @@ -56,6 +54,7 @@ import org.modg.bookshelf.ui.components.BookCover import org.modg.bookshelf.ui.components.BookshelfScaffold import org.modg.bookshelf.ui.components.GoldDivider import org.modg.bookshelf.ui.components.PaperSurface +import org.modg.bookshelf.ui.components.ShelfPickerSheet /** * SPEC.md "detail" screen. [book] filters `deleted = 0`, so once the user @@ -76,6 +75,7 @@ fun DetailScreen( DetailViewModel( bookRepository = container.bookRepository, locationRepository = container.locationRepository, + settingsStore = container.settingsStore, bookId = bookId, ) } @@ -85,6 +85,7 @@ fun DetailScreen( val book by viewModel.book.collectAsState() val bookcases by viewModel.bookcases.collectAsState() val shelves by viewModel.shelves.collectAsState() + val recentShelfId by viewModel.recentShelfId.collectAsState() var retainedBook by remember { mutableStateOf(null) } LaunchedEffect(book) { book?.let { retainedBook = it } } @@ -163,6 +164,7 @@ fun DetailScreen( book = display, bookcases = bookcases, shelves = shelves, + recentShelfId = recentShelfId, onShelfSelected = viewModel::saveLocation, ) } @@ -271,9 +273,10 @@ internal fun LocationSection( book: BookEntity, bookcases: List, shelves: List, + recentShelfId: String?, onShelfSelected: (String?) -> Unit, ) { - var menuExpanded by remember { mutableStateOf(false) } + var pickerOpen by remember { mutableStateOf(false) } val currentShelf = shelves.find { it.id == book.shelfId } val currentBookcase = currentShelf?.let { shelf -> bookcases.find { it.id == shelf.bookcaseId } } val label = if (currentShelf != null && currentBookcase != null) { @@ -291,25 +294,20 @@ internal fun LocationSection( ) { Icon(Icons.Outlined.LocationOn, contentDescription = null, tint = MaterialTheme.colorScheme.secondary) Text(text = label, style = MaterialTheme.typography.bodyLarge, modifier = Modifier.weight(1f)) - Box { - TextButton(onClick = { menuExpanded = true }) { Text("Change") } - DropdownMenu(expanded = menuExpanded, onDismissRequest = { menuExpanded = false }) { - DropdownMenuItem( - text = { Text("Not shelved") }, - onClick = { onShelfSelected(null); menuExpanded = false }, - ) - bookcases.forEach { bookcase -> - shelves.filter { it.bookcaseId == bookcase.id }.forEach { shelf -> - DropdownMenuItem( - text = { Text("${bookcase.name} • ${shelf.label}") }, - onClick = { onShelfSelected(shelf.id); menuExpanded = false }, - ) - } - } - } - } + TextButton(onClick = { pickerOpen = true }) { Text("Change") } } } + + if (pickerOpen) { + ShelfPickerSheet( + bookcases = bookcases, + shelves = shelves, + selectedShelfId = book.shelfId, + recentShelfId = recentShelfId, + onShelfSelected = onShelfSelected, + onDismissRequest = { pickerOpen = false }, + ) + } } @Composable diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailViewModel.kt index 850a012..9ce59f6 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/detail/DetailViewModel.kt @@ -9,6 +9,7 @@ import kotlinx.coroutines.launch import org.modg.bookshelf.data.local.BookEntity import org.modg.bookshelf.data.local.BookcaseEntity import org.modg.bookshelf.data.local.ShelfEntity +import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository import org.modg.bookshelf.data.repo.LocationRepository import org.modg.bookshelf.data.repo.decodeAuthors @@ -44,6 +45,7 @@ data class BookEditForm( class DetailViewModel( private val bookRepository: BookRepository, private val locationRepository: LocationRepository, + private val settingsStore: SettingsStore, private val bookId: String, ) : ViewModel() { @@ -61,6 +63,10 @@ class DetailViewModel( val shelves: StateFlow> = locationRepository.observeShelves() .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + /** The shelf most recently assigned to any book, across sessions — surfaced as the picker's "Recent" shortcut. */ + val recentShelfId: StateFlow = settingsStore.lastShelfId + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), null) + fun saveNotes(notes: String) { val current = book.value ?: return viewModelScope.launch { bookRepository.save(current.copy(notes = notes.ifBlank { null })) } @@ -68,7 +74,17 @@ class DetailViewModel( fun saveLocation(shelfId: String?) { val current = book.value ?: return - viewModelScope.launch { bookRepository.save(current.copy(shelfId = shelfId)) } + viewModelScope.launch { performSaveLocation(current, shelfId) } + } + + /** + * The actual save-location logic, split out from [saveLocation] so tests can await it + * directly (as a plain suspend call) instead of racing [viewModelScope]'s launch. + */ + internal suspend fun performSaveLocation(current: BookEntity, shelfId: String?) { + bookRepository.save(current.copy(shelfId = shelfId)) + // "Not shelved" (null) must never overwrite the memory — it isn't a shelf. + if (shelfId != null) settingsStore.setLastShelfId(shelfId) } fun saveEdit(form: BookEditForm) { 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 6941187..cb6c2b9 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 @@ -18,7 +18,6 @@ import androidx.compose.material.icons.outlined.QrCodeScanner import androidx.compose.material.icons.outlined.Search import androidx.compose.material.icons.outlined.Settings import androidx.compose.material.icons.outlined.Sort -import androidx.compose.material.icons.outlined.Warehouse import androidx.compose.material3.DropdownMenu import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.ExperimentalMaterial3Api @@ -37,6 +36,7 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.res.painterResource import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.sp @@ -44,6 +44,7 @@ import androidx.lifecycle.viewmodel.compose.viewModel import androidx.lifecycle.viewmodel.initializer import androidx.lifecycle.viewmodel.viewModelFactory import org.modg.bookshelf.AppContainer +import org.modg.bookshelf.R import org.modg.bookshelf.data.local.BookEntity import org.modg.bookshelf.data.local.BookcaseEntity import org.modg.bookshelf.data.local.ShelfEntity @@ -97,7 +98,7 @@ fun LibraryScreen( title = "Bookshelf", actions = { IconButton(onClick = onLocationsClick) { - Icon(Icons.Outlined.Warehouse, contentDescription = "Bookcases & shelves") + Icon(painterResource(R.drawable.ic_shelves), contentDescription = "Bookcases & shelves") } IconButton(onClick = onSettingsClick) { Icon(Icons.Outlined.Settings, contentDescription = "Settings") @@ -202,20 +203,28 @@ internal fun LibraryToolbar( Icon(Icons.Outlined.FilterList, contentDescription = "Filter by bookcase or shelf") } DropdownMenu(expanded = filterMenuExpanded, onDismissRequest = { filterMenuExpanded = false }) { - DropdownMenuItem( - text = { Text("All books") }, - onClick = { onFilterChange(LibraryFilter.All); filterMenuExpanded = false }, - ) - bookcases.forEach { bookcase -> + if (bookcases.isEmpty() && shelves.isEmpty()) { DropdownMenuItem( - text = { Text(bookcase.name, style = MaterialTheme.typography.titleSmall) }, - onClick = { onFilterChange(LibraryFilter.Bookcase(bookcase.id)); filterMenuExpanded = false }, + text = { Text("Add a bookcase to enable filtering") }, + enabled = false, + onClick = {}, ) - shelves.filter { it.bookcaseId == bookcase.id }.forEach { shelf -> + } else { + DropdownMenuItem( + text = { Text("All books") }, + onClick = { onFilterChange(LibraryFilter.All); filterMenuExpanded = false }, + ) + bookcases.forEach { bookcase -> DropdownMenuItem( - text = { Text(" ${shelf.label}") }, - onClick = { onFilterChange(LibraryFilter.Shelf(shelf.id)); filterMenuExpanded = false }, + text = { Text(bookcase.name, style = MaterialTheme.typography.titleSmall) }, + onClick = { onFilterChange(LibraryFilter.Bookcase(bookcase.id)); filterMenuExpanded = false }, ) + shelves.filter { it.bookcaseId == bookcase.id }.forEach { shelf -> + DropdownMenuItem( + text = { Text(" ${shelf.label}") }, + onClick = { onFilterChange(LibraryFilter.Shelf(shelf.id)); filterMenuExpanded = false }, + ) + } } } } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/locations/LocationsScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/locations/LocationsScreen.kt index a1f2e17..4b85eb2 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/locations/LocationsScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/locations/LocationsScreen.kt @@ -32,12 +32,15 @@ import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier +import androidx.compose.ui.focus.FocusRequester +import androidx.compose.ui.focus.focusRequester import androidx.compose.ui.unit.dp import androidx.lifecycle.viewmodel.compose.viewModel import androidx.lifecycle.viewmodel.initializer @@ -94,7 +97,12 @@ fun LocationsScreen( action = { PrimaryButton(text = "Add a bookcase", onClick = viewModel::openAddBookcase) }, ) } else { - LazyColumn(contentPadding = PaddingValues(bottom = 96.dp)) { + LazyColumn( + contentPadding = PaddingValues( + top = innerPadding.calculateTopPadding(), + bottom = innerPadding.calculateBottomPadding() + 96.dp, + ), + ) { items(state.bookcases, key = { it.bookcase.id }) { bookcaseUi -> BookcaseRow( bookcaseUi = bookcaseUi, @@ -268,12 +276,22 @@ private fun BookcaseEditDialog( ) { var name by remember { mutableStateOf(editing?.name.orEmpty()) } var note by remember { mutableStateOf(editing?.note.orEmpty()) } + val nameFocusRequester = remember { FocusRequester() } + LaunchedEffect(Unit) { + runCatching { nameFocusRequester.requestFocus() } + } AlertDialog( onDismissRequest = onDismiss, title = { Text(text = title, style = MaterialTheme.typography.titleLarge) }, text = { Column(verticalArrangement = Arrangement.spacedBy(8.dp)) { - OutlinedTextField(value = name, onValueChange = { name = it }, label = { Text("Name") }, singleLine = true) + OutlinedTextField( + value = name, + onValueChange = { name = it }, + label = { Text("Name") }, + singleLine = true, + modifier = Modifier.focusRequester(nameFocusRequester), + ) OutlinedTextField(value = note, onValueChange = { note = it }, label = { Text("Note (optional)") }, singleLine = true) } }, @@ -292,11 +310,21 @@ private fun ShelfEditDialog( onSubmit: (label: String) -> Unit, ) { var label by remember { mutableStateOf(editing?.label.orEmpty()) } + val labelFocusRequester = remember { FocusRequester() } + LaunchedEffect(Unit) { + runCatching { labelFocusRequester.requestFocus() } + } AlertDialog( onDismissRequest = onDismiss, title = { Text(text = title, style = MaterialTheme.typography.titleLarge) }, text = { - OutlinedTextField(value = label, onValueChange = { label = it }, label = { Text("Label") }, singleLine = true) + OutlinedTextField( + value = label, + onValueChange = { label = it }, + label = { Text("Label") }, + singleLine = true, + modifier = Modifier.focusRequester(labelFocusRequester), + ) }, confirmButton = { TextButton(onClick = { onSubmit(label) }, enabled = label.isNotBlank()) { Text("Save") } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt index bcb8dd3..bd299f0 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt @@ -44,7 +44,10 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.focus.FocusRequester +import androidx.compose.ui.focus.focusRequester import androidx.compose.ui.platform.LocalContext +import androidx.compose.ui.platform.LocalSoftwareKeyboardController import androidx.compose.ui.text.input.KeyboardType import androidx.compose.ui.platform.LocalLifecycleOwner import androidx.compose.ui.unit.dp @@ -66,6 +69,7 @@ import org.modg.bookshelf.ui.components.BookshelfScaffold import org.modg.bookshelf.ui.components.EmptyState import org.modg.bookshelf.ui.components.PrimaryButton import org.modg.bookshelf.ui.components.SecondaryButton +import org.modg.bookshelf.ui.components.ShelfPickerSheet /** * SPEC.md "scan" screen: camera + reticle, on-hit bottom sheet, continuous @@ -85,6 +89,7 @@ fun ScanScreen( bookRepository = container.bookRepository, locationRepository = container.locationRepository, metadataRepository = container.metadataRepository, + settingsStore = container.settingsStore, ) } }, @@ -97,6 +102,7 @@ fun ScanScreen( val bookcases by viewModel.bookcases.collectAsState() val shelves by viewModel.shelves.collectAsState() val selectedShelfId by viewModel.selectedShelfId.collectAsState() + val recentShelfId by viewModel.recentShelfId.collectAsState() val permissionState = rememberPermissionState(Manifest.permission.CAMERA) LaunchedEffect(Unit) { @@ -159,6 +165,7 @@ fun ScanScreen( bookcases = bookcases, shelves = shelves, selectedShelfId = selectedShelfId, + recentShelfId = recentShelfId, onShelfSelected = viewModel::selectShelf, onSave = { viewModel.save(state.isbn13, state.metadata) }, onSkip = { viewModel.skip() }, @@ -174,6 +181,7 @@ fun ScanScreen( bookcases = bookcases, shelves = shelves, selectedShelfId = selectedShelfId, + recentShelfId = recentShelfId, onShelfSelected = viewModel::selectShelf, onSave = { title, authors -> viewModel.saveManualEntry(state.isbn13, title, authors) }, onSkip = { viewModel.skip() }, @@ -185,6 +193,7 @@ fun ScanScreen( ) { LookupFailedSheet( isbn13 = state.isbn13, + reason = state.reason, onRetry = { viewModel.retryLookup(state.isbn13) }, onEnterByHand = { viewModel.enterByHand(state.isbn13) }, onSkip = { viewModel.skip() }, @@ -260,9 +269,8 @@ internal fun ScanReticle(modifier: Modifier = Modifier) { /** * Shown the moment a barcode is decoded, while the metadata lookup runs. It names - * the ISBN it read and says so in words, because a lone spinner reads as "still - * working on it" — the user goes on holding the book up to the camera when the - * camera is already done with it. + * the ISBN it read, because a lone spinner reads as "still working on it" — echoing + * the number back is what tells the user the barcode was actually recognised. */ @Composable internal fun SearchingSheet(isbn13: String) { @@ -283,12 +291,6 @@ internal fun SearchingSheet(isbn13: String) { color = MaterialTheme.colorScheme.onSurfaceVariant, modifier = Modifier.padding(top = 4.dp), ) - Text( - text = "Barcode read — you can lower the book.", - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(top = 12.dp), - ) } } @@ -321,6 +323,7 @@ internal fun FoundBookSheet( bookcases: List, shelves: List, selectedShelfId: String?, + recentShelfId: String?, onShelfSelected: (String?) -> Unit, onSave: () -> Unit, onSkip: () -> Unit, @@ -353,6 +356,7 @@ internal fun FoundBookSheet( bookcases = bookcases, shelves = shelves, selectedShelfId = selectedShelfId, + recentShelfId = recentShelfId, onShelfSelected = onShelfSelected, ) Row(modifier = Modifier.fillMaxWidth().padding(top = 16.dp), horizontalArrangement = Arrangement.spacedBy(12.dp)) { @@ -375,6 +379,7 @@ internal fun ManualEntrySheet( bookcases: List, shelves: List, selectedShelfId: String?, + recentShelfId: String?, onShelfSelected: (String?) -> Unit, onSave: (title: String, authors: List) -> Unit, onSkip: () -> Unit, @@ -409,6 +414,7 @@ internal fun ManualEntrySheet( bookcases = bookcases, shelves = shelves, selectedShelfId = selectedShelfId, + recentShelfId = recentShelfId, onShelfSelected = onShelfSelected, ) Row(modifier = Modifier.fillMaxWidth().padding(top = 16.dp), horizontalArrangement = Arrangement.spacedBy(12.dp)) { @@ -429,11 +435,15 @@ internal fun ManualEntrySheet( * Shown when a lookup couldn't be completed — SPEC: "a retry affordance, with * manual entry as the escape hatch; it must NOT claim the book is unknown." No * shelf picker here: saving isn't offered from this sheet, only a path onward to - * one that does (Retry, or the manual-entry form via "Enter by hand"). + * one that does (Retry, or the manual-entry form via "Enter by hand"). [reason] + * is [MetadataRepository]'s diagnostic string (e.g. "open library: network error; + * google books: http 429") — the only channel we have back from a lookup failure + * on a real phone, so it must actually reach the screen instead of being dropped. */ @Composable internal fun LookupFailedSheet( isbn13: String, + reason: String, onRetry: () -> Unit, onEnterByHand: () -> Unit, onSkip: () -> Unit, @@ -445,7 +455,13 @@ internal fun LookupFailedSheet( "This doesn't mean the book is unknown.", style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(top = 8.dp, bottom = 16.dp), + modifier = Modifier.padding(top = 8.dp), + ) + Text( + text = reason, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(top = 4.dp, bottom = 16.dp), ) PrimaryButton(text = "Retry", onClick = onRetry, modifier = Modifier.fillMaxWidth()) Row(modifier = Modifier.fillMaxWidth().padding(top = 12.dp), horizontalArrangement = Arrangement.spacedBy(12.dp)) { @@ -478,9 +494,10 @@ internal fun ShelfPicker( bookcases: List, shelves: List, selectedShelfId: String?, + recentShelfId: String?, onShelfSelected: (String?) -> Unit, ) { - var expanded by remember { mutableStateOf(false) } + var pickerOpen by remember { mutableStateOf(false) } val currentShelf = shelves.find { it.id == selectedShelfId } val currentBookcase = currentShelf?.let { shelf -> bookcases.find { it.id == shelf.bookcaseId } } val label = if (currentShelf != null && currentBookcase != null) { @@ -490,21 +507,18 @@ internal fun ShelfPicker( } Box(modifier = Modifier.padding(top = 12.dp)) { - SecondaryButton(text = label, onClick = { expanded = true }) - androidx.compose.material3.DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) { - androidx.compose.material3.DropdownMenuItem( - text = { Text("Not shelved") }, - onClick = { onShelfSelected(null); expanded = false }, - ) - bookcases.forEach { bookcase -> - shelves.filter { it.bookcaseId == bookcase.id }.forEach { shelf -> - androidx.compose.material3.DropdownMenuItem( - text = { Text("${bookcase.name} • ${shelf.label}") }, - onClick = { onShelfSelected(shelf.id); expanded = false }, - ) - } - } - } + SecondaryButton(text = label, onClick = { pickerOpen = true }) + } + + if (pickerOpen) { + ShelfPickerSheet( + bookcases = bookcases, + shelves = shelves, + selectedShelfId = selectedShelfId, + recentShelfId = recentShelfId, + onShelfSelected = onShelfSelected, + onDismissRequest = { pickerOpen = false }, + ) } } @@ -517,6 +531,14 @@ private fun ManualIsbnDialog(onDismiss: () -> Unit, onSubmit: (String) -> Unit) // screen was just rebuilt to eliminate. val isbn13 = remember(text) { IsbnUtils.toIsbn13(text) } val malformed = text.isNotBlank() && isbn13 == null + val focusRequester = remember { FocusRequester() } + val keyboardController = LocalSoftwareKeyboardController.current + // The dialog opens with the field unfocused otherwise — this is the only entry + // point into the field, so make it ready to type into immediately. + LaunchedEffect(Unit) { + runCatching { focusRequester.requestFocus() } + keyboardController?.show() + } AlertDialog( onDismissRequest = onDismiss, title = { Text("Enter ISBN") }, @@ -529,6 +551,7 @@ private fun ManualIsbnDialog(onDismiss: () -> Unit, onSubmit: (String) -> Unit) singleLine = true, isError = malformed, keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number), + modifier = Modifier.focusRequester(focusRequester), ) if (malformed) { Text( diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt index 7bd7194..59a91a5 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt @@ -15,6 +15,7 @@ import org.modg.bookshelf.data.local.ShelfEntity import org.modg.bookshelf.data.metadata.BookMetadata import org.modg.bookshelf.data.metadata.IsbnUtils import org.modg.bookshelf.data.metadata.MetadataRepository +import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository import org.modg.bookshelf.data.repo.LocationRepository @@ -28,6 +29,7 @@ class ScanViewModel( private val bookRepository: BookRepository, locationRepository: LocationRepository, private val metadataRepository: MetadataRepository, + private val settingsStore: SettingsStore, val scannerController: ScannerController = ScannerController(), ) : ViewModel() { @@ -51,6 +53,10 @@ class ScanViewModel( private val _selectedShelfId = MutableStateFlow(null) val selectedShelfId: StateFlow = _selectedShelfId.asStateFlow() + /** The shelf most recently assigned to any book, across sessions — surfaced as the picker's "Recent" shortcut. */ + val recentShelfId: StateFlow = settingsStore.lastShelfId + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), null) + init { viewModelScope.launch { scannerController.scanResults.collect { isbn13 -> onScanned(isbn13) } @@ -104,35 +110,53 @@ class ScanViewModel( /** Save from a successful metadata lookup. */ fun save(isbn13: String, metadata: BookMetadata) { - viewModelScope.launch { - bookRepository.createBook( - title = metadata.title ?: "Untitled", - subtitle = metadata.subtitle, - authors = metadata.authors, - isbn13 = metadata.isbn13 ?: isbn13, - isbn10 = metadata.isbn10, - publisher = metadata.publisher, - publishedDate = metadata.publishedDate, - pageCount = metadata.pageCount, - description = metadata.description, - coverSourceUrl = metadata.coverUrl, - shelfId = _selectedShelfId.value, - ) - recordSave() - } + viewModelScope.launch { performSave(isbn13, metadata) } + } + + /** + * The actual save-from-metadata logic, split out from [save] so tests can await it + * directly (as a plain suspend call) instead of racing [viewModelScope]'s launch. + */ + internal suspend fun performSave(isbn13: String, metadata: BookMetadata) { + val shelfId = _selectedShelfId.value + bookRepository.createBook( + title = metadata.title ?: "Untitled", + subtitle = metadata.subtitle, + authors = metadata.authors, + isbn13 = metadata.isbn13 ?: isbn13, + isbn10 = metadata.isbn10, + publisher = metadata.publisher, + publishedDate = metadata.publishedDate, + pageCount = metadata.pageCount, + description = metadata.description, + coverSourceUrl = metadata.coverUrl, + shelfId = shelfId, + ) + rememberShelf(shelfId) + recordSave() } /** Save from the manual-entry form shown when metadata lookup misses (SPEC: pre-filled with the scanned ISBN). */ fun saveManualEntry(isbn13: String, title: String, authors: List) { - viewModelScope.launch { - bookRepository.createBook( - title = title.ifBlank { "Untitled" }, - authors = authors, - isbn13 = isbn13, - shelfId = _selectedShelfId.value, - ) - recordSave() - } + viewModelScope.launch { performSaveManualEntry(isbn13, title, authors) } + } + + /** Same split as [performSave], for [saveManualEntry]. */ + internal suspend fun performSaveManualEntry(isbn13: String, title: String, authors: List) { + val shelfId = _selectedShelfId.value + bookRepository.createBook( + title = title.ifBlank { "Untitled" }, + authors = authors, + isbn13 = isbn13, + shelfId = shelfId, + ) + rememberShelf(shelfId) + recordSave() + } + + /** "Not shelved" (null) must never overwrite the memory — it isn't a shelf. */ + private suspend fun rememberShelf(shelfId: String?) { + if (shelfId != null) settingsStore.setLastShelfId(shelfId) } private fun recordSave() { diff --git a/app/app/src/main/res/drawable/ic_shelves.xml b/app/app/src/main/res/drawable/ic_shelves.xml new file mode 100644 index 0000000..2080c6e --- /dev/null +++ b/app/app/src/main/res/drawable/ic_shelves.xml @@ -0,0 +1,15 @@ + + + + + + + diff --git a/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt index 26611a3..77a6b0d 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/metadata/GoogleBooksClientTest.kt @@ -66,25 +66,25 @@ class GoogleBooksClientTest { @Test fun `classify reports Failed for a 404`() { val result = client.classify(404, null) - assertEquals(SourceResult.Failed("http 404"), result) + assertEquals(SourceResult.Failed("http 404", FailureKind.CLIENT_ERROR), result) } @Test fun `classify reports Failed for a 429 keyless-quota response, distinctly from NotFound`() { val result = client.classify(429, null) - assertEquals(SourceResult.Failed("http 429"), result) + assertEquals(SourceResult.Failed("http 429 (rate limited)", FailureKind.RATE_LIMITED), result) } @Test fun `classify reports Failed for a 500`() { val result = client.classify(500, "Internal Server Error") - assertEquals(SourceResult.Failed("http 500"), result) + assertEquals(SourceResult.Failed("http 500 (server error)", FailureKind.SERVER_ERROR), result) } @Test fun `classify reports Failed for a malformed body even on a 2xx status`() { val result = client.classify(200, fixture("malformed.json")) - assertEquals(SourceResult.Failed("malformed json"), result) + assertEquals(SourceResult.Failed("malformed json", FailureKind.MALFORMED), result) } private fun fixture(name: String): String = diff --git a/app/app/src/test/java/org/modg/bookshelf/data/metadata/OpenLibraryClientTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/metadata/OpenLibraryClientTest.kt index cc73301..6e653f3 100644 --- a/app/app/src/test/java/org/modg/bookshelf/data/metadata/OpenLibraryClientTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/data/metadata/OpenLibraryClientTest.kt @@ -125,25 +125,25 @@ class OpenLibraryClientTest { @Test fun `classify reports Failed for a 404`() { val result = client.classify(404, null, "9780201558029") - assertEquals(SourceResult.Failed("http 404"), result) + assertEquals(SourceResult.Failed("http 404", FailureKind.CLIENT_ERROR), result) } @Test fun `classify reports Failed for a 429, distinctly from NotFound`() { val result = client.classify(429, null, "9780201558029") - assertEquals(SourceResult.Failed("http 429"), result) + assertEquals(SourceResult.Failed("http 429 (rate limited)", FailureKind.RATE_LIMITED), result) } @Test fun `classify reports Failed for a 500`() { val result = client.classify(500, "Internal Server Error", "9780201558029") - assertEquals(SourceResult.Failed("http 500"), result) + assertEquals(SourceResult.Failed("http 500 (server error)", FailureKind.SERVER_ERROR), result) } @Test fun `classify reports Failed for a malformed body even on a 2xx status`() { val result = client.classify(200, fixture("malformed.json"), "9780201558029") - assertEquals(SourceResult.Failed("malformed json"), result) + assertEquals(SourceResult.Failed("malformed json", FailureKind.MALFORMED), result) } private fun fixture(name: String): String = diff --git a/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt new file mode 100644 index 0000000..996a4d2 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/data/metadata/RetryPolicyTest.kt @@ -0,0 +1,191 @@ +package org.modg.bookshelf.data.metadata + +import java.io.IOException +import java.io.InterruptedIOException +import java.net.ConnectException +import java.net.SocketException +import java.net.SocketTimeoutException +import java.net.UnknownHostException +import javax.net.ssl.SSLException +import javax.net.ssl.SSLHandshakeException +import kotlin.random.Random +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Covers the retry decision and the loop that acts on it. Every case is driven by + * the 2026-09-09 measurement of the live Open Library API recorded in + * docs/METADATA-SOURCES.md: fast transient TLS resets (worth repeating) against a + * slow but usually-successful long tail (not worth repeating). + * + * The loop is exercised with an injected clock and sleep, so these assert the real + * policy constants with no wall-clock time and no flakiness. + */ +class RetryPolicyTest { + + private val transport = SourceResult.Failed("tls connection reset", FailureKind.TRANSPORT) + private val found = SourceResult.Found(BookMetadata(title = "Dune")) + + // --- which failures are worth repeating --- + + @Test + fun `transport and server errors are retryable`() { + assertTrue(RetryPolicy.isRetryable(FailureKind.TRANSPORT)) + assertTrue(RetryPolicy.isRetryable(FailureKind.SERVER_ERROR)) + } + + @Test + fun `a timeout is not retried -- its budget is already spent`() { + assertFalse(RetryPolicy.isRetryable(FailureKind.TIMEOUT)) + } + + @Test + fun `a rate limit is not retried -- hammering a quota is how a block becomes permanent`() { + assertFalse(RetryPolicy.isRetryable(FailureKind.RATE_LIMITED)) + } + + @Test + fun `deterministic failures are not retried`() { + assertFalse(RetryPolicy.isRetryable(FailureKind.CLIENT_ERROR)) + assertFalse(RetryPolicy.isRetryable(FailureKind.MALFORMED)) + } + + // --- the loop --- + + @Test + fun `a first-attempt success is returned without retrying`() = runTest { + var calls = 0 + val result = withRetry(sleep = {}) { calls++; found } + assertEquals(found, result) + assertEquals(1, calls) + } + + @Test + fun `a transient failure followed by success returns the success`() = runTest { + var calls = 0 + val result = withRetry(sleep = {}) { + calls++ + if (calls == 1) transport else found + } + assertEquals(found, result) + assertEquals(2, calls) + } + + @Test + fun `retrying stops at MAX_ATTEMPTS and reports how many were made`() = runTest { + var calls = 0 + val result = withRetry(sleep = {}) { calls++; transport } + assertEquals(RetryPolicy.MAX_ATTEMPTS, calls) + // The attempt count is the whole diagnostic value of the string on a real + // phone: one reset and three in a row are different network stories. + assertEquals( + SourceResult.Failed("tls connection reset, 3 attempts", FailureKind.TRANSPORT), + result, + ) + } + + @Test + fun `a single failure is reported without an attempt count`() = runTest { + val timeout = SourceResult.Failed("timeout", FailureKind.TIMEOUT) + var calls = 0 + val result = withRetry(sleep = {}) { calls++; timeout } + assertEquals(1, calls) + assertEquals(timeout, result) + } + + @Test + fun `NotFound is authoritative and is never retried`() = runTest { + var calls = 0 + val result = withRetry(sleep = {}) { calls++; SourceResult.NotFound } + assertEquals(SourceResult.NotFound, result) + assertEquals(1, calls) + } + + @Test + fun `an un-retryable failure short-circuits after one attempt`() = runTest { + var calls = 0 + val result = withRetry(sleep = {}) { calls++; SourceResult.fromHttpCode(429) } + assertEquals(1, calls) + assertEquals(FailureKind.RATE_LIMITED, (result as SourceResult.Failed).kind) + } + + @Test + fun `the budget stops a new attempt but never cancels one in flight`() = runTest { + // Clock jumps past the budget during the first attempt. The result of that + // attempt must still be honoured, and no second attempt may start. + var now = 0L + var calls = 0 + val result = withRetry( + budgetMillis = 1_000L, + nowMillis = { now }, + sleep = {}, + ) { + calls++ + now += 5_000L + transport + } + assertEquals(1, calls) + assertEquals(transport, result) + } + + @Test + fun `backoff is short and jittered, never zero and never seconds long`() { + val r = Random(1234) + repeat(200) { + val first = RetryPolicy.backoffMillis(2, r) + val second = RetryPolicy.backoffMillis(3, r) + assertTrue("first retry backoff was $first", first in 250L..349L) + assertTrue("second retry backoff was $second", second in 750L..1049L) + } + } + + // --- exception and status classification --- + + @Test + fun `socket timeout and a blown call timeout both classify as TIMEOUT`() { + assertEquals(FailureKind.TIMEOUT, SourceResult.fromException(SocketTimeoutException()).kind) + // OkHttp reports an exceeded callTimeout as a bare InterruptedIOException. + assertEquals(FailureKind.TIMEOUT, SourceResult.fromException(InterruptedIOException()).kind) + } + + @Test + fun `the observed live failure -- a TLS-stage reset -- classifies as retryable transport`() { + val failed = SourceResult.fromException(SSLException("Connection reset by peer")) + assertEquals(FailureKind.TRANSPORT, failed.kind) + assertTrue(RetryPolicy.isRetryable(failed.kind)) + } + + @Test + fun `each transport exception gets its own reason, not a generic one`() { + // The reason string is the only diagnostic we get back from a real phone, + // so these must stay distinguishable from each other. + val reasons = listOf( + SourceResult.fromException(UnknownHostException()).reason, + SourceResult.fromException(SSLHandshakeException("h")).reason, + SourceResult.fromException(SSLException("r")).reason, + SourceResult.fromException(ConnectException()).reason, + SourceResult.fromException(SocketException()).reason, + ) + assertEquals(reasons.size, reasons.toSet().size) + assertTrue(reasons.none { it.isBlank() }) + } + + @Test + fun `an unrecognised IOException names its own type rather than saying network error`() { + val failed = SourceResult.fromException(IOException("boom")) + assertEquals(FailureKind.TRANSPORT, failed.kind) + assertTrue(failed.reason, failed.reason.contains("IOException")) + } + + @Test + fun `http statuses map to the kinds that drive retrying`() { + assertEquals(FailureKind.RATE_LIMITED, SourceResult.fromHttpCode(429).kind) + assertEquals(FailureKind.SERVER_ERROR, SourceResult.fromHttpCode(500).kind) + assertEquals(FailureKind.SERVER_ERROR, SourceResult.fromHttpCode(503).kind) + assertEquals(FailureKind.CLIENT_ERROR, SourceResult.fromHttpCode(404).kind) + assertEquals(FailureKind.CLIENT_ERROR, SourceResult.fromHttpCode(400).kind) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/data/prefs/SettingsStoreTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/prefs/SettingsStoreTest.kt new file mode 100644 index 0000000..9190cf9 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/data/prefs/SettingsStoreTest.kt @@ -0,0 +1,73 @@ +package org.modg.bookshelf.data.prefs + +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Covers the "remember the most recently used shelf" feature's persistence layer — + * see [org.modg.bookshelf.ui.scan.ScanViewModel] and + * [org.modg.bookshelf.ui.detail.DetailViewModel] for the callers that decide *when* + * to write, and [org.modg.bookshelf.ui.components.resolveRecentShelf] for how the + * picker decides whether the remembered shelf is still offerable. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class SettingsStoreTest { + + private lateinit var settingsStore: SettingsStore + + @Before + fun setUp() = runTest { + settingsStore = SettingsStore(ApplicationProvider.getApplicationContext()) + // DataStore's backing file lives in the app's real files dir, which Robolectric + // does not reset between test methods in this class — start every test from a + // known-clean slate instead of depending on method execution order. + settingsStore.clearLastShelfId() + } + + @Test + fun `last shelf id is null before anything is remembered`() = runTest { + assertNull(settingsStore.lastShelfId.first()) + } + + @Test + fun `last shelf id round-trips through the store`() = runTest { + settingsStore.setLastShelfId("sh-top") + + assertEquals("sh-top", settingsStore.lastShelfId.first()) + } + + @Test + fun `setting a new last shelf id overwrites the previous one`() = runTest { + settingsStore.setLastShelfId("sh-top") + settingsStore.setLastShelfId("sh-desk") + + assertEquals("sh-desk", settingsStore.lastShelfId.first()) + } + + @Test + fun `clearLastShelfId drops the remembered shelf`() = runTest { + settingsStore.setLastShelfId("sh-top") + + settingsStore.clearLastShelfId() + + assertNull(settingsStore.lastShelfId.first()) + } + + @Test + fun `sign out clears the remembered shelf so it can't leak to the other account`() = runTest { + settingsStore.setLastShelfId("sh-top") + + settingsStore.clearAuth() + + assertNull(settingsStore.lastShelfId.first()) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/components/ShelfPickerSheetPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/components/ShelfPickerSheetPaparazziTest.kt new file mode 100644 index 0000000..86d4598 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/ui/components/ShelfPickerSheetPaparazziTest.kt @@ -0,0 +1,78 @@ +package org.modg.bookshelf.ui.components + +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Surface +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import app.cash.paparazzi.DeviceConfig +import app.cash.paparazzi.Paparazzi +import org.junit.Rule +import org.junit.Test +import org.modg.bookshelf.data.local.BookcaseEntity +import org.modg.bookshelf.data.local.ShelfEntity +import org.modg.bookshelf.data.local.SyncState +import org.modg.bookshelf.ui.theme.BookshelfTheme + +/** + * [ShelfPickerSheet]'s grouped picker, replacing the old flat dropdown (SPEC + * task: "the state the whole change exists for"). Renders [ShelfPickerContent] + * directly rather than the real [androidx.compose.material3.ModalBottomSheet] — + * Paparazzi has no real Window/scrim behind a headless sheet, same problem + * [org.modg.bookshelf.ui.screens.ScanScreenPaparazziTest]'s class doc describes. + * Two bookcases, one of them empty, plus a remembered "Recent" shelf — the + * state this whole component exists for. + */ +class ShelfPickerSheetPaparazziTest { + + @get:Rule + val paparazzi = Paparazzi(deviceConfig = DeviceConfig.PIXEL_6) + + private val livingRoom = BookcaseEntity( + id = "bc-living-room", name = "Living Room", note = null, position = 0, + createdAt = 1, updatedAt = 1, syncState = SyncState.SYNCED, + ) + private val study = BookcaseEntity( + id = "bc-study", name = "Study", note = null, position = 1, + createdAt = 1, updatedAt = 1, syncState = SyncState.SYNCED, + ) + private val topShelf = ShelfEntity( + id = "sh-top", bookcaseId = livingRoom.id, label = "Top shelf — fiction", position = 0, + createdAt = 1, updatedAt = 1, syncState = SyncState.SYNCED, + ) + private val bottomShelf = ShelfEntity( + id = "sh-bottom", bookcaseId = livingRoom.id, label = "Bottom shelf — reference", position = 1, + createdAt = 1, updatedAt = 1, syncState = SyncState.SYNCED, + ) + + @Test + fun shelfPickerGroupedLight() = snapshotBoth("shelf-picker-grouped") { + Content() + } + + @Composable + private fun Content() = Shell { + ShelfPickerContent( + bookcases = listOf(livingRoom, study), // "study" has no shelves yet — empty-bookcase state + shelves = listOf(topShelf, bottomShelf), + selectedShelfId = bottomShelf.id, + recentShelfId = topShelf.id, // recent shelf, distinct from the current selection + onShelfSelected = {}, + ) + } + + @Composable + private fun Shell(content: @Composable () -> Unit) { + Box(modifier = Modifier.fillMaxWidth()) { + Surface(color = MaterialTheme.colorScheme.surface) { + content() + } + } + } + + private fun snapshotBoth(name: String, content: @Composable () -> Unit) { + paparazzi.snapshot(name = "$name-light") { BookshelfTheme(darkTheme = false) { content() } } + paparazzi.snapshot(name = "$name-dark") { BookshelfTheme(darkTheme = true) { content() } } + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/components/ShelfPickerSheetTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/components/ShelfPickerSheetTest.kt new file mode 100644 index 0000000..789441b --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/ui/components/ShelfPickerSheetTest.kt @@ -0,0 +1,53 @@ +package org.modg.bookshelf.ui.components + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import org.modg.bookshelf.data.local.BookcaseEntity +import org.modg.bookshelf.data.local.ShelfEntity +import org.modg.bookshelf.data.local.SyncState + +/** + * [resolveRecentShelf] backs [ShelfPickerSheet]'s "Recent" section: it must be + * omitted, not shown dangling, when there is nothing remembered or the + * remembered shelf no longer exists (deleted since it was last used). + */ +class ShelfPickerSheetTest { + + private val livingRoom = BookcaseEntity( + id = "bc-living-room", name = "Living Room", note = null, position = 0, + createdAt = 1, updatedAt = 1, syncState = SyncState.SYNCED, + ) + private val topShelf = ShelfEntity( + id = "sh-top", bookcaseId = livingRoom.id, label = "Top shelf", position = 0, + createdAt = 1, updatedAt = 1, syncState = SyncState.SYNCED, + ) + + @Test + fun `no remembered shelf resolves to null`() { + val result = resolveRecentShelf(recentShelfId = null, shelves = listOf(topShelf), bookcases = listOf(livingRoom)) + + assertNull(result) + } + + @Test + fun `a remembered shelf that no longer exists resolves to null`() { + val result = resolveRecentShelf(recentShelfId = "sh-deleted", shelves = listOf(topShelf), bookcases = listOf(livingRoom)) + + assertNull(result) + } + + @Test + fun `a remembered shelf whose bookcase was deleted resolves to null`() { + val result = resolveRecentShelf(recentShelfId = topShelf.id, shelves = listOf(topShelf), bookcases = emptyList()) + + assertNull(result) + } + + @Test + fun `a remembered shelf that still exists resolves to its shelf and bookcase`() { + val result = resolveRecentShelf(recentShelfId = topShelf.id, shelves = listOf(topShelf), bookcases = listOf(livingRoom)) + + assertEquals(topShelf to livingRoom, result) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/detail/DetailViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/detail/DetailViewModelTest.kt new file mode 100644 index 0000000..0872e5a --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/ui/detail/DetailViewModelTest.kt @@ -0,0 +1,77 @@ +package org.modg.bookshelf.ui.detail + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +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.local.BookEntity +import org.modg.bookshelf.data.local.BookshelfDatabase +import org.modg.bookshelf.data.prefs.SettingsStore +import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.LocationRepository +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * SPEC's "remember the most recently used shelf" from the detail screen's side: + * [DetailViewModel.saveLocation] must write a non-null shelf to [SettingsStore], + * but "Not shelved" (null) must never overwrite what's already remembered. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class DetailViewModelTest { + + private lateinit var db: BookshelfDatabase + private lateinit var settingsStore: SettingsStore + private lateinit var viewModel: DetailViewModel + private lateinit var book: BookEntity + private lateinit var shelfId: String + + @Before + fun setUp() = runTest { + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, BookshelfDatabase::class.java) + .allowMainThreadQueries() + .build() + settingsStore = SettingsStore(context) + val bookRepository = BookRepository(db.bookDao(), context) + val locationRepository = LocationRepository(db.bookcaseDao(), db.shelfDao(), db.bookDao()) + val bookcaseId = locationRepository.createBookcase(name = "Living Room") + shelfId = locationRepository.createShelf(bookcaseId, label = "Top shelf") + val bookId = bookRepository.createBook(title = "Piranesi") + book = checkNotNull(bookRepository.getById(bookId)) + + viewModel = DetailViewModel( + bookRepository = bookRepository, + locationRepository = locationRepository, + settingsStore = settingsStore, + bookId = bookId, + ) + } + + @After + fun tearDown() { + db.close() + } + + @Test + fun `moving a book to a shelf remembers that shelf`() = runTest { + viewModel.performSaveLocation(book, shelfId) + + assertEquals(shelfId, settingsStore.lastShelfId.first()) + } + + @Test + fun `marking a book Not shelved does not overwrite the remembered shelf`() = runTest { + settingsStore.setLastShelfId(shelfId) + + viewModel.performSaveLocation(book, null) + + assertEquals(shelfId, settingsStore.lastShelfId.first()) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt new file mode 100644 index 0000000..423e12d --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt @@ -0,0 +1,100 @@ +package org.modg.bookshelf.ui.scan + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json +import okhttp3.OkHttpClient +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.local.BookshelfDatabase +import org.modg.bookshelf.data.metadata.BookMetadata +import org.modg.bookshelf.data.metadata.MetadataRepository +import org.modg.bookshelf.data.prefs.SettingsStore +import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.LocationRepository +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * SPEC's "remember the most recently used shelf": [ScanViewModel.save] and + * [ScanViewModel.saveManualEntry] must write the chosen shelf to [SettingsStore] + * so it survives to the next scanning session — except "Not shelved" (null), + * which must never overwrite what's already remembered. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class ScanViewModelTest { + + private lateinit var db: BookshelfDatabase + private lateinit var settingsStore: SettingsStore + private lateinit var viewModel: ScanViewModel + private lateinit var locationRepository: LocationRepository + private lateinit var shelfId: String + + @Before + fun setUp() = runTest { + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, BookshelfDatabase::class.java) + .allowMainThreadQueries() + .build() + settingsStore = SettingsStore(context) + locationRepository = LocationRepository(db.bookcaseDao(), db.shelfDao(), db.bookDao()) + val bookcaseId = locationRepository.createBookcase(name = "Living Room") + shelfId = locationRepository.createShelf(bookcaseId, label = "Top shelf") + + viewModel = ScanViewModel( + bookRepository = BookRepository(db.bookDao(), context), + locationRepository = locationRepository, + metadataRepository = MetadataRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), + settingsStore = settingsStore, + ) + } + + @After + fun tearDown() { + db.close() + } + + @Test + fun `saving a metadata hit to a shelf remembers that shelf`() = runTest { + viewModel.selectShelf(shelfId) + + viewModel.performSave("9780765326355", BookMetadata(title = "The Way of Kings")) + + assertEquals(shelfId, settingsStore.lastShelfId.first()) + } + + @Test + fun `saving a manual entry to a shelf remembers that shelf`() = runTest { + viewModel.selectShelf(shelfId) + + viewModel.performSaveManualEntry("9780765326355", "The Way of Kings", listOf("Brandon Sanderson")) + + assertEquals(shelfId, settingsStore.lastShelfId.first()) + } + + @Test + fun `saving as Not shelved does not overwrite the remembered shelf`() = runTest { + settingsStore.setLastShelfId(shelfId) + viewModel.selectShelf(null) + + viewModel.performSave("9780765326355", BookMetadata(title = "The Way of Kings")) + + assertEquals(shelfId, settingsStore.lastShelfId.first()) + } + + @Test + fun `saving a manual entry as Not shelved does not overwrite the remembered shelf`() = runTest { + settingsStore.setLastShelfId(shelfId) + viewModel.selectShelf(null) + + viewModel.performSaveManualEntry("9780765326355", "The Way of Kings", emptyList()) + + assertEquals(shelfId, settingsStore.lastShelfId.first()) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/screens/DetailScreenPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/screens/DetailScreenPaparazziTest.kt index 500d674..a8c7058 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/screens/DetailScreenPaparazziTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/screens/DetailScreenPaparazziTest.kt @@ -78,6 +78,7 @@ class DetailScreenPaparazziTest { book = book, bookcases = ScreenFixtures.bookcases, shelves = ScreenFixtures.shelves, + recentShelfId = null, onShelfSelected = {}, ) } diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/screens/LocationsScreenPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/screens/LocationsScreenPaparazziTest.kt index b5ec1b0..cebd330 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/screens/LocationsScreenPaparazziTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/screens/LocationsScreenPaparazziTest.kt @@ -18,6 +18,7 @@ import app.cash.paparazzi.DeviceConfig import app.cash.paparazzi.Paparazzi import org.junit.Rule import org.junit.Test +import org.modg.bookshelf.data.local.BookcaseEntity import org.modg.bookshelf.ui.components.BookshelfScaffold import org.modg.bookshelf.ui.components.GoldDivider import org.modg.bookshelf.ui.components.PaperSurface @@ -40,12 +41,25 @@ class LocationsScreenPaparazziTest { @Test fun locationsPopulatedLight() = snapshotBoth("locations-populated") { Populated() } - @Composable - private fun Populated() = Shell() + /** + * Regression coverage for the ghost-bookcase bug: with the list branch not + * folding in the Scaffold's top app bar inset, a single bookcase's row + * rendered underneath the app bar and was invisible. This renders exactly + * that one-bookcase state; check the recorded PNG shows the bookcase row + * fully below the app bar, not clipped/hidden behind it. + */ + @Test + fun locationsSingleBookcaseLight() = snapshotBoth("locations-single-bookcase") { SingleBookcase() } @Composable - private fun Shell() { - val bookcaseUis = ScreenFixtures.bookcases.map { bookcase -> + private fun Populated() = Shell(ScreenFixtures.bookcases) + + @Composable + private fun SingleBookcase() = Shell(listOf(ScreenFixtures.livingRoom)) + + @Composable + private fun Shell(bookcases: List) { + val bookcaseUis = bookcases.map { bookcase -> BookcaseUi( bookcase = bookcase, shelves = ScreenFixtures.shelves diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/screens/ScanScreenPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/screens/ScanScreenPaparazziTest.kt index eaa8f0f..d0d2dfa 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/screens/ScanScreenPaparazziTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/screens/ScanScreenPaparazziTest.kt @@ -96,6 +96,7 @@ class ScanScreenPaparazziTest { bookcases = ScreenFixtures.bookcases, shelves = ScreenFixtures.shelves, selectedShelfId = ScreenFixtures.deskShelf.id, + recentShelfId = null, onShelfSelected = {}, onSave = {}, onSkip = {}, @@ -127,6 +128,7 @@ class ScanScreenPaparazziTest { ) { LookupFailedSheet( isbn13 = "9780765326355", + reason = "open library: network error; google books: http 429", onRetry = {}, onEnterByHand = {}, onSkip = {}, diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.components_ShelfPickerSheetPaparazziTest_shelfPickerGroupedLight_shelf-picker-grouped-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.components_ShelfPickerSheetPaparazziTest_shelfPickerGroupedLight_shelf-picker-grouped-dark.png new file mode 100644 index 0000000..1758921 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.components_ShelfPickerSheetPaparazziTest_shelfPickerGroupedLight_shelf-picker-grouped-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.components_ShelfPickerSheetPaparazziTest_shelfPickerGroupedLight_shelf-picker-grouped-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.components_ShelfPickerSheetPaparazziTest_shelfPickerGroupedLight_shelf-picker-grouped-light.png new file mode 100644 index 0000000..827adb1 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.components_ShelfPickerSheetPaparazziTest_shelfPickerGroupedLight_shelf-picker-grouped-light.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LocationsScreenPaparazziTest_locationsSingleBookcaseLight_locations-single-bookcase-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LocationsScreenPaparazziTest_locationsSingleBookcaseLight_locations-single-bookcase-dark.png new file mode 100644 index 0000000..96529f5 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LocationsScreenPaparazziTest_locationsSingleBookcaseLight_locations-single-bookcase-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LocationsScreenPaparazziTest_locationsSingleBookcaseLight_locations-single-bookcase-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LocationsScreenPaparazziTest_locationsSingleBookcaseLight_locations-single-bookcase-light.png new file mode 100644 index 0000000..a2ecd32 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LocationsScreenPaparazziTest_locationsSingleBookcaseLight_locations-single-bookcase-light.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-dark.png index a1dd69e..fa683a2 100644 Binary files a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-dark.png and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-light.png index a6e6d04..af6b04e 100644 Binary files a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-light.png and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanLookupFailedSheetLight_scan-lookup-failed-sheet-light.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-dark.png index ef8f8f0..5f586d7 100644 Binary files a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-dark.png and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-light.png index 888e600..3267c83 100644 Binary files a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-light.png and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_ScanScreenPaparazziTest_scanSearchingSheetLight_scan-searching-sheet-light.png differ diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index ecec457..da1eab4 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -411,3 +411,90 @@ successfully. Use these instead, in this order: 2. `tail logs/.state` — says SUCCESS / GIVING UP / WALL CLOCK explicitly. 3. `git status --porcelain` — is there actually work in the tree? The sentinel is a convenience, not the record of truth. `logs/.state` is. + +## Wave 6 — second on-device feedback round: COMPLETE, verified 2026-09-09 +The user tested the phone build again and sent eight items. All eight are done. +Two Sonnet workers (`tasks/H1-screens.txt`, `tasks/H2-picker.txt`) took the UI +work; the ORCHESTRATOR did the retry/backoff work itself in `data/metadata` and +`AppContainer`, because it needed the live measurement below to design it. + +| Check | Result | +|---|---| +| `./tasks/gw assembleDebug` | exit 0 | +| `./tasks/gw testDebugUnitTest` | exit 0 — **172 tests**, 1 skipped, 0 failures (was 138) | +| `./tasks/gw verifyPaparazziDebug` | exit 0 | +| `grep "always 'false'"` on a `--rerun-tasks` rebuild | **0 hits** | +| `./tasks/gw assembleRelease` | exit 0 — 41,810,740 bytes, V2 signer `CN=Bookshelf, O=Montanaro` | +| boundary check | clean — neither worker touched a build file or the other's packages | + +### The ghost bookcase was an inset bug, not a data bug +`LocationsScreen`'s list branch dropped the Scaffold's `innerPadding` while its +empty-state branch applied it, so the FIRST bookcase row rendered underneath the +top app bar and was invisible. Every symptom the user described follows from that: +invisible first bookcase, no empty state on re-entry (the list was genuinely +non-empty), a second bookcase created, both showing in the filter menu. **Both +records were always real and healthy** — the user should delete the spare. +Every other screen was checked for the same class of bug; Locations was the only +one. Fix: fold `innerPadding` into the LazyColumn's `contentPadding` (NOT +`Modifier.padding`, which would clip the scroll area instead of insetting it). + +### Metadata: measured, not guessed +See `docs/METADATA-SOURCES.md` § "Measured again 2026-09-09" for the full data. +Two things that change how you should think about this app: + +1. **Google Books keyless is dead for everyone, permanently.** The user's + residential-IP test returned a quota error naming `project_number:624717413613` + — a shared anonymous *project*, not an IP. The old note in METADATA-SOURCES.md + guessing that a residential IP "may well get answers" is now marked CORRECTED + in place. Because `combine()` turns any Failed-with-no-Found into `Unavailable`, + this standing failure meant **every** Open Library hiccup surfaced as + "one or more sources couldn't be reached". The app has been single-sourced all + along. **The user has deliberately deferred the API key — do not add it unasked.** +2. **Our own timeouts were manufacturing failures.** 30 live requests: 13% failed, + all fast TLS resets (<2.5s); successes had a median of 4.3s but a max of 22.0s, + and **2 of 26 successes exceeded the old 12s `callTimeout`**. Timeouts are now + 25s/20s/20s. Failures are fast and successes are slow, so a short timeout buys + nothing on the failure path and costs real successes on the slow path. + +`RetryPolicy` + `withRetry` (new, `data/metadata/`) retry TRANSPORT and +SERVER_ERROR only. It deliberately does NOT retry: +- **TIMEOUT** — the budget is already spent; retrying could triple the wait. +- **RATE_LIMITED** — hammering a quota is how an intermittent block becomes a + permanent one, and METADATA-SOURCES.md records that happening to this project's + IP. Revisit when the Google Books key lands: a *keyed* 429 is a per-second limit + and does deserve one Retry-After-respecting retry. +`SourceResult.Failed` now carries a `FailureKind` alongside its human `reason`, and +`reason` names the specific exception ("tls connection reset, 3 attempts") instead +of a generic "network error". **That string is now rendered on the scan sheet and +is our ONLY diagnostic channel from a real phone.** Nothing may parse it. + +### Known-soft spots in wave 6 — do not mistake these for verified +1. **The Paparazzi "regression" snapshot for the ghost bookcase is a lookalike, + not the real screen.** `LocationsScreenPaparazziTest` hand-rolls its own + Scaffold+LazyColumn copy because the real `LocationsScreen` needs an + `AppContainer` (Room + DataStore). The orchestrator verified the REAL fix by + reading the diff; the PNG only proves the test's copy is right, and the two can + drift — the copy already omits the bottom inset the real screen adds. Splitting + a stateless `LocationsContent(state, callbacks)` out of the screen would make + this snapshot genuine. Worth doing before anyone trusts it as regression cover. +2. **The auto-focus calls are unverified.** Three dialogs now do + `LaunchedEffect(Unit) { runCatching { focusRequester.requestFocus() } }`. That + is the idiomatic form, but there is no emulator here and `runCatching` means a + too-early call fails SILENTLY rather than crashing. If a dialog opens unfocused + on the phone, that is why; the fix is to await a frame before requesting. +3. **The shelf picker opens as a bottom sheet stacked on top of the save sheet** + (H2's own flagged judgement call). It renders correctly in Paparazzi but + sheet-over-sheet is awkward on real Android. Watch it on the device. + +### Worker lessons (both are repeats — the prompts already forbade them) +- **H1 backgrounded a Gradle build and ended its turn**, exactly the wave-4 + failure, despite an explicit foreground-only instruction AND + `CLAUDE_CODE_PRINT_BG_WAIT_CEILING_MS=0` being set in `run-task.sh`. Its final + message was "I'll wait for this background build to complete." `run-task.sh` + still recorded SUCCESS because the process exited 0. **`.state` saying SUCCESS + means the process exited cleanly, NOT that the worker finished its task** — read + `logs/.summary` and check that the result is an actual report. H1's work + was fine, but nobody verified it except the orchestrator. +- H2 (108 turns, $4.00) followed the brief closely, ran builds in the foreground, + and reported honestly, including flagging its own stacked-sheet judgement call. + Cost ratio to H1 ($0.66, 5 turns) is roughly the ratio of work actually done. diff --git a/docs/METADATA-SOURCES.md b/docs/METADATA-SOURCES.md index 5042f85..a97e1d1 100644 --- a/docs/METADATA-SOURCES.md +++ b/docs/METADATA-SOURCES.md @@ -97,6 +97,10 @@ Worth saying plainly, because it bounds how much weight the numbers carry: this host sharing a quota pool with other tenants; **your phone, on a residential or mobile IP, may well get answers.** That's precisely why the app needs to be able to tell us which it got. + > **CORRECTED 2026-09-09 — this guess was WRONG.** The user tested from a + > residential IP and got the same refusal, naming a shared *project* quota + > rather than an IP one. See "Measured again 2026-09-09" at the end of this + > file. Do not act on the sentence above. - I don't know which three ISBNs you scanned. If you still have the books to hand, those three numbers are worth more than another 60 sampled ones. @@ -215,3 +219,76 @@ One thing worth deciding separately: 17% of books legitimately have no cover art anywhere. The placeholder now looks deliberate rather than broken, but if you want covers on everything, that's a different feature — photograph the book, store it as the cover — and not a metadata-source problem at all. + +## Measured again 2026-09-09, after the user tested from a residential IP + +Two things were settled that the first round could only guess at. + +### Google Books keyless is dead everywhere, not just from this box + +The user ran the app's exact Google Books call from their home connection and got: + + Quota exceeded for quota metric 'Queries' and limit 'Queries per day' + of service 'books.googleapis.com' for consumer 'project_number:624717413613' + +That names a **Google Cloud project, not an IP**. Every keyless caller on the +internet is billed to that one shared anonymous project and its daily quota is +exhausted. So: + +- The earlier caveat — "your phone, on a residential or mobile IP, may well get + answers" — is **WRONG**. Delete it from your mental model. It was tested and it + is not true. +- Backoff cannot help. This is a daily quota, not a per-second rate limit. +- A free API key is the only fix, and it is a complete one: it moves the app into + its own project with its own quota (free tier 1,000 req/day). + +The knock-on is the part that actually hurt the user. `MetadataRepository.combine` +turns "any source Failed, none Found" into `Unavailable`. Google Books is a +PERMANENT standing failure, so **every** Open Library hiccup became `Unavailable`. +The app has effectively been single-sourced this whole time while reporting +failures as though two sources had been consulted. + +**The user has deliberately deferred the API key.** Do not implement it unasked. + +### Open Library: 13% failure, and our own timeout was manufacturing more + +30 requests, the exact call `OpenLibraryClient` makes, 1.5s apart, from the sprite: + +| | | +|---|---| +| failure rate | **13%** (4 of 30) | +| every failure | curl exit 35 — TLS-stage `Connection reset by peer` | +| failure latency | 0.23s, 0.31s, 0.59s, 2.46s — **all fast** | +| success latency | median **4.3s**, p75 6.0s, p90 **9.2s**, max **22.0s** | +| successes over the old 12s callTimeout | **2 of 26 (8%)** | + +Two conclusions, and they point in opposite directions: + +1. **Failures are cheap and transient**, so retrying is nearly free. 13% → ~1.7% + at two attempts → ~0.2% at three. This is why `RetryPolicy` exists and why its + backoff is milliseconds rather than the conventional seconds. +2. **Successes are slow and long-tailed**, and the app's own + `callTimeout(12s)`/`connectTimeout(10s)` were cutting off roughly 8% of + lookups that were about to succeed — then reporting them to the user as + "couldn't be reached". The app was generating a meaningful share of its own + failures. Timeouts are now 25s/20s/20s, above the 22.0s worst observed success. + +The asymmetry is the whole design: a short timeout buys nothing on the failure +path (failures return in under 2.5s regardless) and costs real successes on the +slow path. That is also why `RetryPolicy.isRetryable` refuses to repeat a +TIMEOUT — a timeout means the budget was already spent, and the evidence says +slow requests mostly succeed if you let them finish. + +Connection reuse is visible in the data and matters in real use: cold connects +ran 2-19s while warm ones ran 0.07s. OkHttp pools connections for 5 minutes, so +scanning a box of books in sequence stays on the fast path after the first book. + +### What still is not known + +- All 30 requests came from this datacenter IP. The user's phone may see a + different failure rate. The reason string now shown on the scan sheet + (`SourceResult.Failed.reason`, e.g. "tls connection reset, 3 attempts") is how + we find out — it is the only diagnostic channel we have from a real device. +- The user's own 3-request sample showed 2 failures. That is consistent with 13% + (p ~ 5%) but does not confirm it. If their phone reports "3 attempts" often, + their network is worse than this one and the retry count deserves revisiting. diff --git a/logs/G-diagnostics.sid b/logs/G-diagnostics.sid new file mode 100644 index 0000000..6a15241 --- /dev/null +++ b/logs/G-diagnostics.sid @@ -0,0 +1 @@ +9b656dc8-a225-47ec-b6ce-549241854de6 diff --git a/logs/G-diagnostics.summary b/logs/G-diagnostics.summary new file mode 100644 index 0000000..e6a5c5b --- /dev/null +++ b/logs/G-diagnostics.summary @@ -0,0 +1,19 @@ +=== G-diagnostics attempts=2 quota_waits=1 hard_fails=0 === +cost=$0.28007899999999997 turns=6 err=? +--- result (tail) --- +` case. `byIsbnCoverUrl` untouched. + +**AppContainer.kt** — `metadataHttpClient` now has `callTimeout(12s)/connectTimeout(10s)/readTimeout(10s)`. PocketBase `okHttpClient` untouched. + +**ui.scan:** +- `ScanCodeFilter.kt` — `accept()` returns `ScanOutcome { Isbn, NotAnIsbn, Ignored }`; one shared debounce mechanism throttles repeats of valid *and* rejected codes alike. +- `ScannerController.kt` — new `rejectedCodes: SharedFlow` routed alongside `scanResults`. +- `ScanModels.kt` — `ScanSheetState.NotFound` gained `viaLookupFailure: Boolean = false`; added `ScanSheetState.LookupFailed(isbn13, reason)`; `ScanMetadataOutcome.from` maps all three `LookupResult` cases. +- `ScanViewModel.kt` — `retryLookup`/`enterByHand` added; lookup logic factored so Retry bypasses the "sheet already showing" guard; `rejectedMessage: StateFlow` with a 3s auto-clear timer. +- `ScanScreen.kt` — new `LookupFailedSheet` (Retry/Enter by hand/Skip, dismissible, never claims the book unknown); `ManualEntrySheet` reworded via `authoritative: Boolean`; new `RejectedBarcodeBanner` shown over the camera. + +**Tests:** classify() cases for both clients, the full 3×3 `combine` matrix, `ScanCodeFilter` throttle tests with an injected clock, `ScannerController` rejected-code routing, updated `ScanModelsTest`, and two new Paparazzi snapshots (`LookupFailedSheet`, rejected-barcode banner) recorded in both light and dark — all visually verified. + +No warnings (Kotlin or otherwise) were introduced in any file I touched; a clean `--rerun` compile shows only pre-existing, unrelated deprecation warnings elsewhere in the codebase. + +Nothing was left undone relative to the original instructions, and no constraints (forbidden files, no commits, foreground builds, no MockWebServer) were violated. diff --git a/logs/H1-screens.sid b/logs/H1-screens.sid new file mode 100644 index 0000000..8f8ec99 --- /dev/null +++ b/logs/H1-screens.sid @@ -0,0 +1 @@ +430e1016-0c8c-47a5-905b-ad106cbb6b19 diff --git a/logs/H1-screens.summary b/logs/H1-screens.summary new file mode 100644 index 0000000..75e2110 --- /dev/null +++ b/logs/H1-screens.summary @@ -0,0 +1,4 @@ +=== H1-screens attempts=1 quota_waits=0 hard_fails=0 === +cost=$0.6623552 turns=5 err=? +--- result (tail) --- +I'll wait for this background build to complete before continuing. diff --git a/logs/H2-picker.sid b/logs/H2-picker.sid new file mode 100644 index 0000000..fee804e --- /dev/null +++ b/logs/H2-picker.sid @@ -0,0 +1 @@ +67bf128a-9c53-4fd2-b9ce-10b99f4a385d diff --git a/logs/H2-picker.summary b/logs/H2-picker.summary new file mode 100644 index 0000000..bd3912c --- /dev/null +++ b/logs/H2-picker.summary @@ -0,0 +1,10 @@ +=== H2-picker attempts=1 quota_waits=0 hard_fails=0 === +cost=$3.9990499999999995 turns=108 err=? +--- result (tail) --- + succeeded. New `shelf-picker-grouped-{light,dark}` snapshot shows two bookcases (Living Room with two shelves, one selected/bold/checked; Study empty with "No shelves yet"), plus the "Recent" section and "Not shelved" — verified visually. Existing `scan-searching-sheet` and `scan-lookup-failed-sheet` snapshots regenerated and confirmed correct (no stray "lower the book" line; reason string now visible). +- Grepped all Kotlin compiler warnings from a forced rebuild: zero `always 'false'` hits anywhere, including my files. +- `git status --porcelain`: only my owned files changed by me (`data/prefs/SettingsStore.kt`, `ui/scan/ScanScreen.kt`, `ui/scan/ScanViewModel.kt`, `ui/detail/DetailScreen.kt`, `ui/detail/DetailViewModel.kt`, new `ui/components/ShelfPickerSheet.kt`, and new/edited files under `app/app/src/test/**`). Everything else showing modified/untracked (`AppContainer.kt`, `data/metadata/**`, `ui/locations/**`, `ui/library/**`, `res/drawable/**`, `docs/METADATA-SOURCES.md`, locations snapshots) is the orchestrator's/other worker's concurrent work — untouched by me, left as-is. + +**Didn't do / judgment calls worth flagging** +- Nothing skipped from the spec. One design decision: `ShelfPickerSheet` owns its own `ModalBottomSheet`, so tapping "Choose a shelf"/"Change" opens it as a second, stacked sheet over the already-open save/detail sheet — this matches "presented as a Material 3 ModalBottomSheet" literally rather than inlining the list in the existing sheet. +- Added tests beyond the stated minimum: `SettingsStoreTest` (round-trip, overwrite, clear, clearAuth), `ShelfPickerSheetTest` (`resolveRecentShelf` — null/missing-shelf/missing-bookcase/found cases), `ScanViewModelTest` and `DetailViewModelTest` (remember-on-save/move, "Not shelved" doesn't overwrite). diff --git a/logs/WAVE6-DONE b/logs/WAVE6-DONE new file mode 100644 index 0000000..76315a2 --- /dev/null +++ b/logs/WAVE6-DONE @@ -0,0 +1,14 @@ +=== WAVE6-DONE written 2026-09-09T15:31:35+00:00 === +Workers finished. The orchestrator was NOT necessarily alive for this. + +--- H1-screens --- +[2026-09-09T15:05:30+00:00] H1-screens: SUCCESS after 1 attempt(s), 0 quota wait(s) +cost=$0.6623552 turns=5 + +--- H2-picker --- +[2026-09-09T15:31:29+00:00] H2-picker: SUCCESS after 1 attempt(s), 0 quota wait(s) +cost=$3.9990499999999995 turns=108 + +NEXT: orchestrator must independently verify before accepting: + cd ~/bookshelf && ./tasks/gw assembleDebug && ./tasks/gw testDebugUnitTest + git status --porcelain # boundary check: who touched what diff --git a/tasks/H1-screens.txt b/tasks/H1-screens.txt new file mode 100644 index 0000000..0ca9d1d --- /dev/null +++ b/tasks/H1-screens.txt @@ -0,0 +1,137 @@ +You are a Sonnet worker on the Bookshelf Android app (~/bookshelf). Read +`docs/SPEC.md` first — it is the authoritative product contract and it wins over +anything you infer from the code. Do not restate it, do not let it drift. + +## Ground rules (violating these fails the wave) +- Build ONLY with `./tasks/gw ` — never `./gradlew`. A second worker shares + this Gradle project dir and concurrent invocations clobber each other. `tasks/gw` + is a flock-serialized wrapper. +- Run builds in the FOREGROUND. Never background a Gradle build and end your turn + saying you'll report later — `claude -p` kills background tasks and you will + never report at all. Builds take up to 10 minutes; just wait. +- You own EXACTLY these files: + app/app/src/main/java/org/modg/bookshelf/ui/locations/LocationsScreen.kt + app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt + app/app/src/main/res/drawable/ic_shelves.xml (new file, create it) + app/app/src/test/** (tests you add) + Touch NOTHING else. Specifically forbidden: any build file + (`app/build.gradle.kts`, `gradle/libs.versions.toml`, `settings.gradle.kts`), + `data/**`, `ui/scan/**`, `ui/detail/**`, `ui/components/**`, `ui/nav/**`, + `ui/settings/**`, `ui/setup/**`, `AppContainer.kt`. Another worker and the + orchestrator own those RIGHT NOW and are editing them concurrently. +- Do not change any public composable signature. `ui/nav/BookshelfNavHost.kt` + calls these screens and you may not edit it. + +## Task 1 — the ghost-bookcase bug (highest priority, a real user-facing defect) +`LocationsScreen.kt` line ~97. The Scaffold hands `content` an `innerPadding` that +accounts for the top app bar. The empty-state branch applies it; the list branch +does NOT: + + ) { innerPadding -> + PaperSurface(...) { + if (state.bookcases.isEmpty()) { + EmptyState(modifier = Modifier.padding(innerPadding), ...) // correct + } else { + LazyColumn(contentPadding = PaddingValues(bottom = 96.dp)) { // BUG + +So the first bookcase row renders UNDERNEATH the app bar and is invisible. A user +created a bookcase, could not see it, created a second one, and ended up with two +real bookcases and no idea why. Fix it by folding `innerPadding` into the +LazyColumn's `contentPadding` so the existing 96.dp bottom inset is PRESERVED and +added to, not replaced — the bottom inset is what keeps the last row clear of the +FAB. Something equivalent to: + + contentPadding = PaddingValues( + top = innerPadding.calculateTopPadding(), + bottom = innerPadding.calculateBottomPadding() + 96.dp, + ) + +Using `contentPadding` rather than `Modifier.padding` is deliberate: it keeps the +list scrolling under the bar instead of clipping the scroll area. + +VERIFY THIS SPECIFICALLY: add a Paparazzi snapshot of `LocationsScreen` in a state +with exactly ONE bookcase, and confirm in the rendered PNG that the bookcase row is +fully visible below the app bar. A one-bookcase list is the exact case that was +broken and it must be the case you prove fixed. If the existing Paparazzi harness +makes rendering this screen with seeded state impractical, say so plainly in your +report rather than skipping it silently. + +## Task 2 — auto-focus the first field in the location dialogs +In `LocationsScreen.kt`, `BookcaseEditDialog` (~line 276) and `ShelfEditDialog` +(~line 299) each open with an unfocused `OutlinedTextField`. The first field +should take focus and raise the keyboard when the dialog appears. Use a +`FocusRequester` + `LaunchedEffect(Unit) { focusRequester.requestFocus() }`. +Bookcase dialog: focus "Name" (not "Note"). Shelf dialog: focus "Label". +Guard the requestFocus call so it cannot throw if the node isn't attached yet. + +## Task 3 — library filter empty state +`LibraryScreen.kt` ~line 204. The filter DropdownMenu always offers "All books" +first, then a flat list of bookcases and shelves. When there are NO bookcases and +NO shelves, the menu contains only "All books" — which is already the active state +and cannot be changed, so it is a menu with nothing in it. + +When `bookcases` and `shelves` are both empty, replace the menu contents with a +single DISABLED item reading "Add a bookcase to enable filtering". Keep the +toolbar filter icon visible and enabled (it is how the feature is discovered) — +only the menu's contents change. When locations DO exist, behaviour is unchanged. + +## Task 4 — replace the Warehouse icon with a real bookcase +`LibraryScreen.kt` line ~100 uses `Icons.Outlined.Warehouse` for the button that +opens Locations. It renders as a barn and reads wrong. `material-icons-extended` +1.7.8 has no bookcase glyph (I checked all 1932 outlined icons), so use Material +Symbols' `shelves`, which is a bookcase frame with shelves and books on them. + +Create `app/app/src/main/res/drawable/ic_shelves.xml` with EXACTLY this content. +This is the SVG path verbatim from Google's CDN. Do not re-derive it, do not + "simplify" it, and do not convert its relative (lowercase) commands to absolute + ones — Android's pathData parser accepts SVG syntax as-is. Material Symbols ship with +`viewBox="0 -960 960 960"` — a negative Y origin that Android `` has no +equivalent for — and the `` is what compensates. +Removing it renders an empty icon. + + + + + + + + + +Then swap the icon at the call site: + + Icon(painterResource(R.drawable.ic_shelves), contentDescription = "Bookcases & shelves") + +Keep the existing contentDescription text. `Icon` applies its own tint over a +Painter exactly as it does over an ImageVector, so the icon still picks up the +theme colour — do NOT hardcode a colour at the call site. You will need imports +for `androidx.compose.ui.res.painterResource` and `org.modg.bookshelf.R`, and the +`Icons.Outlined.Warehouse` import becomes unused — remove it. + +## Verify before you report (all in the FOREGROUND) + ./tasks/gw assembleDebug + ./tasks/gw testDebugUnitTest + ./tasks/gw recordPaparazziDebug + git status --porcelain + +- assembleDebug and testDebugUnitTest must exit 0. The test count is 138 today and + must not go DOWN. +- Grep your build output for the string `always 'false'`. That Kotlin warning class + silently blanked every book cover in this app for months by making a `when` + branch dead code that still compiled. Zero hits on files you touched. +- `git status --porcelain` must show ONLY the files you own. If it shows others, + you have broken the boundary — report it, do not revert someone else's work. +- Do not commit. The orchestrator commits after verifying. + +## Report +Finish with a plain report: what you changed per task, the exact exit codes and +test counts, whether the one-bookcase Paparazzi render actually proves task 1, and +anything you could NOT do. Do not claim success you did not verify — several +previous workers on this project over-claimed and were caught. diff --git a/tasks/H2-picker.txt b/tasks/H2-picker.txt new file mode 100644 index 0000000..abcac64 --- /dev/null +++ b/tasks/H2-picker.txt @@ -0,0 +1,143 @@ +You are a Sonnet worker on the Bookshelf Android app (~/bookshelf). Read +`docs/SPEC.md` first — it is the authoritative product contract and it wins over +anything you infer from the code. Do not restate it, do not let it drift. + +## Ground rules (violating these fails the wave) +- Build ONLY with `./tasks/gw ` — never `./gradlew`. Another worker shares + this Gradle project dir and concurrent invocations clobber each other. + `tasks/gw` is a flock-serialized wrapper. +- Run builds in the FOREGROUND. Never background a Gradle build and end your turn + saying you'll report later — `claude -p` kills background tasks and you will + never report at all. Builds take up to 10 minutes; just wait. +- You own EXACTLY these files: + ui/components/ShelfPickerSheet.kt (new file, create it) + ui/scan/ScanScreen.kt + ui/scan/ScanViewModel.kt + ui/detail/DetailScreen.kt + ui/detail/DetailViewModel.kt + data/prefs/SettingsStore.kt + app/app/src/test/** (tests you add) + (paths relative to app/app/src/main/java/org/modg/bookshelf/) + Touch NOTHING else. Specifically forbidden: any build file + (`app/build.gradle.kts`, `gradle/libs.versions.toml`), `data/metadata/**` + (the ORCHESTRATOR is editing that right now, in this same working tree), + `ui/locations/**`, `ui/library/**`, `ui/nav/**`, `ui/settings/**`, + `ui/setup/**`, `AppContainer.kt`, and every file in `ui/components/` EXCEPT the + new `ShelfPickerSheet.kt` you create. +- Do not change any public composable signature. `ui/nav/BookshelfNavHost.kt` + calls these screens and you may not edit it. +- `AppContainer.settingsStore` is already a public val — you do NOT need to change + AppContainer to reach it. + +## Background: what the user actually reported +They are shelving books a box at a time, scanning a run of books that all belong +on the SAME shelf. Two complaints: +1. The shelf dropdown lists every bookcase×shelf pair in one flat menu. With more + than a couple of bookcases that is unusable. +2. Re-picking the same shelf for every book in the box is tedious. + +## Task 1 — a grouped shelf picker, replacing the flat dropdown +There are currently TWO near-duplicate flat pickers: + - `ShelfPicker` in `ScanScreen.kt` (~line 477) + - the location picker inside `DetailScreen.kt` (~line 273-310) +Both build a `DropdownMenu` containing "Not shelved" followed by one flat item per +(bookcase, shelf) pair, labelled "Bookcase • Shelf". + +Replace BOTH with ONE new shared composable in `ui/components/ShelfPickerSheet.kt`, +presented as a Material 3 `ModalBottomSheet` rather than a dropdown. Contents, top +to bottom: + a. A "Recent" section — see task 2. Omit the whole section when there is no + remembered shelf, or when the remembered shelf no longer exists. + b. "Not shelved" as an always-available choice. + c. One section per bookcase, in `position` order. The bookcase name is a section + header (non-selectable — a bookcase is not a location a book can sit in; + only shelves are). Its shelves are listed under it in `position` order, + labelled with just the shelf label, since the header already gives the + bookcase. Make the currently-selected shelf visually distinct. + d. A bookcase with no shelves shows its header and an inline "No shelves yet" + hint, so an empty bookcase does not look like a rendering bug. +The sheet must scroll. Do NOT add a search field — the user explicitly deferred +that; we will add it later if the grouping alone proves insufficient. + +Design language is in SPEC.md ("feels like books") and the existing components in +`ui/components/` are your reference for type, spacing and the gold hairline rules. +Reuse `GoldDivider` for section separation rather than inventing a new rule. + +## Task 2 — remember the most recently used shelf +Add a `LAST_SHELF_ID` key to `SettingsStore` (a nullable String preference, with a +`Flow` reader, a setter, and a way to clear it), following the exact +shape of the keys already there. Clear it in `clearAuth()` alongside the rest — +signing out of a shared library should not leak the other account's shelf. + +Write it whenever a book's shelf is set to a non-null value: + - `ScanViewModel` — on save, both from a metadata hit and from manual entry. + - `DetailViewModel` — when the user changes a book's location. +Setting a book to "Not shelved" must NOT overwrite the remembered shelf; it isn't +a shelf, and clobbering the memory with it would defeat the whole feature. + +Surface it as the "Recent" section at the top of the picker, labelled with the +full "Bookcase • Shelf" text (the section has no bookcase header to lean on). + +**Do NOT pre-select it.** The user considered and explicitly rejected +pre-selection: the risk of silently mis-shelving a book, when the user forgets to +change it, outweighs saving one tap. A new scan still starts with no shelf chosen; +the remembered shelf is one tap away at the top of the sheet, and that is all. + +Note `ScanViewModel._selectedShelfId` already survives across saves within a single +scanning session (it is deliberately not reset in `recordSave`). Keep that. What +you are adding is persistence ACROSS sessions and screens. + +## Task 3 — three small fixes in ScanScreen.kt +3a. In `SearchingSheet` (~line 268), delete the third Text, the one reading + "Barcode read — you can lower the book." The user says "Searching…" plus the + ISBN already carries it. Update the composable's KDoc, which currently + justifies that line — do not leave a comment explaining code that is gone. + +3b. `LookupFailedSheet` (~line 435) takes a `reason` parameter and never renders + it. `ScanSheetState.LookupFailed(isbn13, reason)` already carries a diagnostic + string, and `MetadataRepository` already builds it as e.g. + "open library: network error; google books: http 429". It is currently dead — + it reaches the UI and is dropped on the floor. + Render it, below the existing explanatory paragraph, in + `MaterialTheme.typography.bodySmall` and `onSurfaceVariant`. This is the only + diagnostic channel we have from a real phone, so it must actually appear. + Keep the existing headline and paragraph as they are — the reason is + supplementary detail, not a replacement for plain-language copy. + NOTE: the orchestrator is concurrently making those reason strings more + specific. Do not depend on their exact wording — render whatever you are + given, and do not parse, match on, or reformat the string. + +3c. `ManualIsbnDialog` (~line 512) opens with an unfocused text field. Auto-focus + it and raise the keyboard, via `FocusRequester` + + `LaunchedEffect(Unit) { focusRequester.requestFocus() }`. Guard the call so it + cannot throw if the node is not attached yet. + +## Verify before you report (all in the FOREGROUND) + ./tasks/gw assembleDebug + ./tasks/gw testDebugUnitTest + ./tasks/gw recordPaparazziDebug + git status --porcelain + +- assembleDebug and testDebugUnitTest must exit 0. The test count is 138 today and + must not go DOWN. Add real tests for the logic you introduce — at minimum the + SettingsStore round-trip, that "Not shelved" does not overwrite the memory, and + that a remembered shelf which no longer exists is not offered. Assertion-free + tests are explicitly forbidden by SPEC's quality bar. +- Add a Paparazzi snapshot of the new picker sheet with at least two bookcases, + one of them empty, and a recent shelf present — that is the state the whole + change exists for, and no one has ever trusted a worker's word on how this app + looks. +- Grep your build output for the string `always 'false'`. That Kotlin warning class + silently blanked every book cover in this app for months by making a `when` + branch dead code that still compiled. Zero hits on files you touched. +- `git status --porcelain` will show files the orchestrator and the other worker + are editing (`data/metadata/**`, `ui/locations/**`, `ui/library/**`, + `res/drawable/**`). That is EXPECTED. Confirm only that no file outside your + own list was changed BY YOU. Never revert or "fix" someone else's work. +- Do not commit. The orchestrator commits after verifying. + +## Report +Finish with a plain report: what you changed per task, exact exit codes and test +counts, what the new Paparazzi PNG shows, and anything you could NOT do. Do not +claim success you did not verify — several previous workers on this project +over-claimed and were caught.