Spriteandclaude
d6d02f788c
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 under the top app bar and was invisible.
Both of the user's bookcases were always real; they just could not see the first
one, so they made a second. Every other screen was checked for the same class of
bug — Locations was the only one.
Metadata lookup is now designed against a measurement rather than a guess
(docs/METADATA-SOURCES.md § "Measured again 2026-09-09"):
- Google Books keyless is dead for everyone, permanently. The user's
residential-IP test returned a quota error naming a shared anonymous PROJECT,
not an IP, so the earlier "your phone may well get answers" guess is wrong and
is now marked CORRECTED in place. Because combine() turns any Failed-with-no-
Found into Unavailable, that standing failure meant every Open Library hiccup
surfaced as "one or more sources couldn't be reached". The API key is
deliberately deferred by the user; this commit leaves the source broken.
- Our own timeouts were manufacturing failures. Over 30 live requests, 13%
failed — all fast TLS resets under 2.5s — while successes ran to a median of
4.3s and a max of 22.0s. Two of 26 successes exceeded the old 12s callTimeout,
so ~8% of lookups that were about to work were cancelled and reported as
unreachable. Timeouts are now 25s/20s/20s.
That asymmetry (cheap failures, expensive successes) is what RetryPolicy encodes.
It retries TRANSPORT and SERVER_ERROR with a 250ms/750ms jittered backoff, and
deliberately does not retry TIMEOUT (the budget is already spent) or RATE_LIMITED
(hammering a quota is how an intermittent block becomes a permanent one — this
project's IP has already been refused outright once during research).
SourceResult.Failed now carries a FailureKind alongside its human reason, and the
reason names the specific failure ("tls connection reset, 3 attempts") instead of
a generic "network error". That string was already threaded to the UI and dropped
on the floor; LookupFailedSheet now renders it. It is the only diagnostic channel
we have from a real phone, so nothing may parse it.
Also from the same feedback round:
- Grouped ModalBottomSheet shelf picker, replacing two near-duplicate flat
dropdowns that listed every bookcase x shelf pair. Sections per bookcase,
empty bookcases say so, and the most recently used shelf is pinned on top.
- The recent shelf persists across sessions (SettingsStore.LAST_SHELF_ID) and is
cleared on sign-out. It is offered, never pre-selected: the user weighed that
and chose one tap over the risk of silently mis-shelving a book.
- Locations dialogs and the manual-ISBN dialog auto-focus their first field.
- The library filter menu offers "Add a bookcase to enable filtering" instead of
a lone "All books" that is already the active state and cannot be changed.
- The Locations button is Material Symbols' "shelves" (a bookcase) instead of
Warehouse (a barn). material-icons-extended 1.7.8 has no bookcase glyph.
- The scan sheet drops "you can lower the book" — the ISBN echo already says it.
assembleDebug exit 0; testDebugUnitTest 172 tests, 1 skipped, 0 failures (was
138); verifyPaparazziDebug exit 0; assembleRelease exit 0, signed with the real
release key; zero "always 'false'" warnings on a --rerun-tasks rebuild.
Three soft spots are recorded in docs/HANDOFF.md and are NOT verified: the
ghost-bookcase Paparazzi snapshot renders a lookalike of the screen rather than
the screen, the auto-focus calls swallow their own failure and no emulator exists
here, and the picker opens as a sheet stacked on the save sheet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LSnVqWdiQNEcPFRq1hGZAi
144 lines
8.3 KiB
Plaintext
144 lines
8.3 KiB
Plaintext
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 <task>` — 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<String?>` 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.
|