diff --git a/docs/agent-tasks/01-web-authentication.md b/docs/agent-tasks/01-web-authentication.md new file mode 100644 index 0000000..b0d88ca --- /dev/null +++ b/docs/agent-tasks/01-web-authentication.md @@ -0,0 +1,121 @@ +# Task: password-protect the backend's HTTP API + +## Why + +The backend's HTTP API has no authentication and listens on `0.0.0.0`. The worst part +is the room page's Home Assistant proxy +(`python-backend/musicmouse/services/web/api.py`, `_proxy` and +`POST /api/ha/services/{domain}/{service}`): it forwards *any* service call on *any* +entity with the stored long-lived HA token. Anything on the LAN can unlock a lock, +disarm an alarm or restart Home Assistant through it. Parent mode (`/api/settings`, +`/api/remote/mapping`, `/api/timer`) is only hidden, not protected. + +## What the owner decided + +A browser authenticates **once** with a password, and then stays logged in for about +**a year**. The config stores only a **hash** of the password, never the password +itself. Use this design; do not substitute another one (no entity allow-list instead, +no per-endpoint tokens, no OAuth). + +## Design + +### Config + +- Add `general.web.password_hash` (in `WebConfig`, `musicmouse/config.py`). It is + **required** whenever the `web` section is present. Follow the repo's existing + convention for opting out of something important (see `SIMULATE` for `serial_port` + and `alsa_device`): the literal value `"disabled"` turns authentication off, logs a + warning on **every** boot, and is the only way to run without it. A missing key is a + config error. Config errors already say which key is wrong. +- Hash format: `scrypt$$$

$$` using `hashlib.scrypt` from + the standard library. No new dependency. Choose parameters that verify in well under + a second on a Raspberry Pi 4 (for example n=2**14, r=8, p=1), and measure on the + desktop to sanity-check. Verify with `hmac.compare_digest`. +- Add a CLI subcommand to set it. For example, + `python -m musicmouse --config config.yml --set-password` prompts twice with + `getpass`, hashes, and writes the key back into `config.yml` using the existing + round-trip helpers in `musicmouse/services/web/settings.py` (`load_document`, + `atomic_write`), so comments survive. Exit afterwards and start nothing else. +- Document the key and the command in `python-backend/config.yml.example`. + +### Sessions + +- `POST /api/auth/login` with `{"password": "..."}`. On success it sets a cookie and + returns 204; on failure it returns 401 after a fixed delay of about 1 s (cheap brute-force + damping; no lockout state needed). +- The cookie (`mm_session`): `HttpOnly`, `SameSite=Strict`, `Path=/`, + `Max-Age` = 365 days. **Not** `Secure`: the device is served over plain HTTP on the + LAN, and a `Secure` cookie would never be sent back. +- The cookie value is a stateless signed token, `.`. + The HMAC key is derived from a persistent random secret *and* the current + `password_hash`, so **changing the password logs every browser out** without extra + bookkeeping. Keep the secret in a file under `general.library.cache` (for example + `session-secret`, 32 random bytes, created on first start with mode 0600). Deleting + the cache logs everyone out, which is acceptable; say so in the docs. +- Optional, cheap, and nice: re-issue the cookie with a fresh year on any authenticated + request when fewer than 30 days remain, so a device in daily use never expires. +- `POST /api/auth/logout` clears the cookie. `GET /api/auth` returns 204 or 401, so the + front-end can check without side effects. + +### What is protected + +- **Every** route under `/api`, including the websocket `/api/ws`, except + `/api/auth/*`. Implement it once, as a FastAPI dependency on the router in + `build_router` (or a small middleware), not route by route. The websocket checks the + cookie *before* `accept()` and closes with code 4401 when it is missing or invalid. +- **Not** the static front-end files: the login screen has to load. +- **No localhost exemption.** The kiosk on the device logs in once like everyone else. + An exemption by peer address breaks silently the day a reverse proxy is put in front + of it (every request then looks local). +- MQTT, the IR remote and the serial link are not HTTP and are out of scope. +- Keep the room page's proxy as it is otherwise. Do not add an entity allow-list in + this task. + +### Front-end (`web/`) + +- `src/api/client.ts`: a 401 from any call raises a dedicated `Unauthorized` error. + `fetchOrNullOn404` / `fetchOrNullOnError` must not swallow a 401 as "not configured". +- A login screen: one password field, German copy ("Passwort", "Anmelden", "Falsches + Passwort"), styled like the existing sheets (`.overlay` / `.sheet`). It must work with + only a keyboard (the kiosk has no pointer). Show it when the first `/api/state` or + `/api/library` call, or the websocket, reports unauthorized. On success, reload the + library, the state and the websocket. The simplest correct way is `location.reload()`. +- `src/hooks/usePlayerState.ts`: on a 4401 close, stop the reconnect loop and show the + login screen instead of retrying every 1.5 s. +- The service worker (`public/sw.js`) must not cache `/api/auth/*`. It already avoids + `/api` state; check it. + +## Tests (backend) + +- Every existing web test goes through the API. Give the test client fixture in + `tests/test_web.py` (and `test_remote_api.py`, `test_tippen_api.py`) a logged-in + cookie, so the existing tests stay as they are. Add a separate unauthenticated client + fixture for the new tests. +- New tests: + - Each `/api` route returns 401 without the cookie. Generate the list from the router + so a new route cannot slip through unprotected. + - The websocket is refused without the cookie. + - Login succeeds with the right password and fails with a wrong one. + - A token past its expiry is rejected. + - Changing `password_hash` invalidates existing cookies. + - `"disabled"` lets everything through and logs the warning. + - A missing key is a config error. + - The `--set-password` command writes a hash that verifies and keeps the file's + comments. +- The fixed failure delay must not slow the suite down: inject it, as the codebase + injects `Clock`. + +## Docs to update + +- `python-backend/README.md`: the "Parent mode" paragraph that says there is no + authentication; the route table; a short "Authentication" section. +- `web/README.md`: the "Parent mode" paragraph. +- `docs/REPO_OVERVIEW.md`: the backend ↔ web paragraph and the config summary. +- The comment block above the room-control routes in `api.py`, which justifies the open + proxy with "trusted LAN device". + +## Done when + +`scripts/check.sh` passes, and a manual run against `--simulate` with a hash set shows +the login screen, logs in, survives a backend restart without asking again, and asks +again after the password is changed. diff --git a/docs/agent-tasks/04-one-input-policy.md b/docs/agent-tasks/04-one-input-policy.md new file mode 100644 index 0000000..e3cf0b5 --- /dev/null +++ b/docs/agent-tasks/04-one-input-policy.md @@ -0,0 +1,121 @@ +# Task: one behaviour per button, whatever it is pressed on + +## Why + +The backend's README promises that front-ends only translate input into intents and that +`musicmouse/reactions/` is the one place that decides what happens. Three behaviours +break that promise today, so the same request does different things depending on where +it came from: + +| Behaviour | Web front-end | IR remote | Mouse buttons / knob | MQTT | +|---|---|---|---|---| +| Next/previous on a podcast | seeks ±30 s (`web/src/App.tsx`, `skipPodcast`) | skips a track | skips a track | skips a track | +| Unmute | jumps to 60 % (`UNMUTE_PERCENT` in `App.tsx`) | restores the previous volume (`LircService._toggle_mute`) | - | - | +| Volume step | J/K: 10 % (`VOLUME_STEP` in `web/src/lib/keyboard.ts`); +/- buttons: `volume_increment` | `volume_increment` | `volume_increment` × detents | absolute only | + +There is also a confirmed bug. With `min_volume > 0`, the remote's mute never unmutes: +"mute" asks for volume 0, the player clamps it to `min_volume`, the next press sees +`volume > 0` and mutes again. + +## What the owner decided + +1. **Next/previous on a podcast episode always seeks 30 s** forward/back, from every + input: web (buttons and keys), IR remote, the mouse's buttons, MQTT. +2. **Unmute always restores the volume from before muting**, from every input. +3. **Parent mode gets three separate volume-step settings**: the web keyboard's J/K, + the mouse's rotary knob, and the IR remote. Each is configured independently. + +## Design + +### 1. Podcast skip lives in the backend + +- In `musicmouse/reactions/playback.py`, `next_track` / `previous_track` look at the + current album (`app.album_for(app.player.playlist)`). For a podcast episode they seek + instead: `position ± PODCAST_SKIP_SECONDS` (30, a module constant), clamped to + `[0, duration]`. Take the duration from the library track when the player does not + know it yet, the way `services/web/state.py` does. +- "Is this a podcast?" is decided by **one** helper. If `docs/agent-tasks/06-podcast-kind.md` + has landed, that is `album.kind == "podcast"`. If not, add `is_podcast(album)` next to + `Album` (`musicmouse/library/models.py`) that checks the section's + `album_unit == "episode"`, and let task 06 change its body. +- `web/src/App.tsx`: the `next` / `previous` actions always call `api.next()` / + `api.previous()`. Delete `skipPodcast` and `PODCAST_SKIP_SECONDS`. Drop the optimistic + `position: 0` patch too: the client no longer knows what the backend will do. If the + ≤0.5 s until the next position frame is visibly laggy, let `StateHub` push a + `position` frame right after a next/previous/seek intent has been handled. Don't + re-add client-side policy. +- The mouse's left/right buttons only act while playing (`button_pressed`); keep that. + +### 2. Mute is a player state, not a volume + +- Add `muted: bool` to the `Player` protocol, plus `set_muted(muted, *, source)`, + implemented in `PlayerBase`. `VlcPlayer` applies it with libVLC's + `audio_set_mute`; `FakePlayer` just records it. Muting does **not** touch `volume`, + so unmuting restores it by construction, and `min_volume` cannot interfere. +- Announce changes with a new `StateEvent` (for example `MuteChanged`) and add it to + `SNAPSHOT_EVENTS` in `services/web/hub.py`. A test fails until every `StateEvent` is + sorted into that list or `IGNORED_EVENTS`. +- A new intent, `ToggleMuteRequested`, handled by a reaction. Any volume change + (`SetVolumeRequested`, `VolumeChangeRequested`, the step intent below) also unmutes; + that is what every TV remote does. +- `POST /api/mute/toggle`. The web's Shift+M and the mute button call it. Delete + `UNMUTE_PERCENT` and `onMute`'s volume arithmetic. +- `LircService`: `KEY_MUTE` emits `ToggleMuteRequested`. Delete `_pre_mute_volume` and + `_toggle_mute`. +- The state snapshot (`PlayerStateOut`, `web/src/api/types.ts`) gets `muted`. The volume + bars show muted distinctly instead of showing volume 0. +- MQTT is out of scope. Its volume number stays absolute, and setting it unmutes via + the rule above. + +### 3. Three volume steps + +- Config: replace `general.volume_increment` with + `general.volume_steps: {keyboard: int, knob: int, remote: int}`, all in **device + units** like `min_volume` / `max_volume`, each `ge=1, le=100`. Defaults: `keyboard: 10`, + `knob: 5`, `remote: 5` (today's behaviour for knob and remote). +- `volume_increment` in an existing `config.yml` must fail with a message naming the + replacement ("replaced by volume_steps.knob / .remote / .keyboard"). Config + validation rejects unknown keys anyway; make the message specific rather than the + generic "unknown option". +- One intent: `VolumeStepRequested(direction: Literal[-1, 1], count: int = 1, + input: Literal["keyboard", "knob", "remote"])`. One reaction multiplies by the + configured step and calls `change_volume`. + - `rotary_turned` emits it with `input="knob"`, `count=abs(event.increment)`. + - `LircService` volume keys emit it with `input="remote"`. + - `POST /api/volume` takes `{"steps": n, "input": "keyboard" | "remote"}` instead of + bare `steps`. J/K send `input: "keyboard"`. The on-screen −/+ buttons keep taking + the remote's step, as 1254790 deliberately chose, so they send `input: "remote"`. + - `{"percent": n}` stays for the volume bars. +- Parent mode: `SettingsIn` / `SettingsOut` (`services/web/schemas.py`), + `read_settings` / `write_settings` (`services/web/settings.py`, writing the nested + mapping), and `web/src/components/ParentPanel.tsx` get three fields, labelled in + German (for example "Lautstärke-Schritt: Tastatur (J/K)", "… Drehknopf", + "… Fernbedienung"). They replace the single "rotary step". Saving applies live, like + the other settings. +- Update `python-backend/config.yml.example`, the route table in + `python-backend/README.md`, `web/README.md`'s key table, and `HelpOverlay.tsx`. + +## Tests + +- Reactions (`tests/test_reactions.py`, through the simulator): + - Next/previous on a podcast episode seeks ±30 s and clamps at both ends. Use a + `Kinderpodcasts` folder in the test library (see `tests/conftest.py`). + - Next/previous on music still changes track. + - Mute then unmute restores the exact volume, including with `min_volume > 0` (the + regression). + - A volume change while muted unmutes. + - Each of the three inputs moves by its own configured step. +- `tests/test_lirc.py`: the mute key and the volume keys emit the new intents. +- `tests/test_web.py`: `/api/mute/toggle`; `/api/volume` with `input`; the settings + round-trip for `volume_steps`, which keeps the file's comments. +- `tests/test_config.py`: the defaults, and the specific error for `volume_increment`. +- `web/src/lib/__tests__/keyboard.test.ts`: J/K produce the step action, not a + percentage. + +## Done when + +`scripts/check.sh` passes, `grep -rn "PODCAST_SKIP\|UNMUTE_PERCENT\|volume_increment"` +finds nothing in `web/src` and `python-backend/musicmouse` apart from the config error +that names the old key, and in `--simulate` a +podcast skips 30 s from the web, from `press right` in the REPL, and from a simulated +remote key. diff --git a/docs/agent-tasks/05-firmware-serial-frames.md b/docs/agent-tasks/05-firmware-serial-frames.md new file mode 100644 index 0000000..bd17f73 --- /dev/null +++ b/docs/agent-tasks/05-firmware-serial-frames.md @@ -0,0 +1,75 @@ +# Task: stop the firmware garbling its own messages to the host + +## Why + +`sendMessageToHost` (`esp-firmware/src/Messages.h`) writes one frame as four separate +`Serial.write` calls: magic, type, size, payload. It runs on two FreeRTOS tasks at once: + +- the Arduino `loop()` task, for touch, rotary encoder and button events + (`esp-firmware/src/main.cpp`); +- the RC522 reader's own task (`rc522_task` in `esp-firmware/lib/nfc/rc522.c`, priority + 4), which calls `tagHandler` → `sendMessageToHost(MsgRfidTokenRead{...})` directly. + +When a tag read coincides with a button or touch event, the two frames interleave on +the wire. The host's decoder (`python-backend/musicmouse/devices/wire.py`, +`FrameDecoder`) resynchronises on the magic token and drops what it cannot parse. So +the symptom is an occasionally **lost figure placement/removal or button press**, with a +"Discarding bad frame from firmware" warning in the backend log. + +## What the owner decided + +Fix it in whatever way keeps the firmware simplest. A mutex was the first idea. **Use +the queue below instead, unless it turns out not to work**, because it is simpler: + +- With a queue, every frame and every log line is written from `loop()` alone, so no + lock is needed anywhere, including around the `Serial.println` log lines that could + otherwise land in the middle of a frame. +- It is exactly the pattern `main.cpp` already uses for the rotary encoder + (`eventQueueRotaryEncoder`, drained in `handleRotaryEncoder()` with + `xQueueReceive(..., 0)`). + +A mutex would need wrapping every `Serial.write` / `Serial.print` in the firmware to be +complete. That is more code and easier to get wrong later. + +## Design + +In `esp-firmware/src/main.cpp`: + +1. Create a FreeRTOS queue for tag reads in `setupRfidReader()`, **before** + `rc522_start`, so the callback can never run without it. Item type: the 5-byte tag id + (`MsgRfidTokenRead` itself is fine, it is a packed 5-byte struct). Depth: a handful + (4) is plenty; the reader polls every 125 ms. +2. `tagHandler` (runs on the RC522 task) only fills a `MsgRfidTokenRead`, all zeros + for "removed" exactly as now, and `xQueueSend(queue, &msg, 0)`. If the queue is full, + drop the read; the reader reports the state again on the next change. Nothing in + `tagHandler` touches `Serial` any more. +3. Add `handleRfid()`, called from `loop()` next to `handleRotaryEncoder()`. It drains + the queue with `xQueueReceive(..., 0)` and calls `sendMessageToHost` for each item. +4. Leave `Messages.h` alone. With a single writer, the four `Serial.write` calls are + fine as they are. + +Do not change the wire format, message ids or the host. `python-backend/tests/test_wire.py` +parses `Messages.h` and pins the ids, and it must keep passing. + +Out of scope: the touch thresholds, debouncing, anything in `lib/`. + +## Checking it + +There are no firmware unit tests that reach `main.cpp` (the `native` env only builds the +LED effects), and this machine may not have PlatformIO. So: + +- It must build: `pio run -e esp32` in `esp-firmware/`. If PlatformIO is not + available, say so in the hand-off instead of claiming it builds. +- `pio run -e native` still builds `src/host_test.cpp`. That env excludes `main.cpp`, so + this only guards against accidental changes to shared headers. +- `scripts/check.sh backend` still passes (`test_wire.py`). +- Manual test on the device, with the backend running at `--log-level DEBUG`. Hold a + hand on an ear touch area while repeatedly placing and removing a figure for a + minute, and turn the knob while doing it. Expect every placement and removal to show + up as `RfidTokenRead`, and **no** "Discarding bad frame from firmware" warnings. + Before the fix, the same exercise should produce some. + +## Done when + +The firmware builds, the host tests pass, `tagHandler` contains no `Serial` call, and the +manual test shows no discarded frames. diff --git a/docs/agent-tasks/06-podcast-kind.md b/docs/agent-tasks/06-podcast-kind.md new file mode 100644 index 0000000..6890648 --- /dev/null +++ b/docs/agent-tasks/06-podcast-kind.md @@ -0,0 +1,89 @@ +# Task: make "podcast" a kind of album + +## Why + +`AlbumKind` is `Literal["music", "book"]` (`python-backend/musicmouse/library/sections.py`), +and the `Kinderpodcasts` shelf is declared `kind="book"`. So "is this a podcast?" is +answered differently in each layer: + +- web: `album.section === "Kinderpodcasts"` (`web/src/lib/search.ts`, `groupOf`), and + everything else goes through `groupOf(album) === "podcasts"` (App, PlayView, + BrowseView, MobileBrowse, keyboard.ts, remote.ts, ambience.ts, useTrackDetail); +- backend: `SECTIONS[album.section].album_unit == "episode"` (`MusicLibrary.latest_episode`, + `tippen/rewards.py`); +- typing game: reward kinds `"episode"` and `"podcast_episode"` + (`services/web/tippen_api.py`). + +The shelf's folder name is therefore a de-facto API string the front-end depends on. +`todo.md` already asks for a cleaner podcast data model. + +## What the owner decided + +Add `"podcast"` to the album kind. Podcast-ness is then a property of the album, carried +in the index and the API, and nobody compares section names. + +## Design + +### Backend + +- `AlbumKind = Literal["music", "book", "podcast"]`. `SECTIONS["Kinderpodcasts"]` gets + `kind="podcast"`. +- `Section.album_unit` stays. It is the scanner's structural fact (one album per file), + not a classification. Keep using it in the scanner (`scan_library`) and in + `podcast_feeds.find_feed_shows`. Every place that asks "is this album a podcast?" + switches to `album.kind == "podcast"`: + - `MusicLibrary.latest_episode`; + - `tippen/rewards.py` (`_chronological_episodes`, and the `album_unit == "folder"` + branches that really mean "not a podcast"); + - `tippen_api._animation_kind`; + - the `is_podcast` helper, if task 04 added one: change its body, keep its callers. +- `scanner.scan_album` derives `series` only for `kind == "book"`. Check that episode + albums still get `series = folder.name` (they do, in `scan_episodes`) and that + nothing else branches on `"book"` meaning "book or podcast". + `grep -rn '"book"' python-backend/musicmouse` and decide per hit. +- `FigureConfig.kind` stays `Literal["music", "book"]`. A figurine holding a podcast is + not a thing. Type `Config.figure_kinds` accordingly. +- `cache._album_from_json` validates the kind: accept `"podcast"`. Bump + `_INDEX_VERSION` (with a comment line, like the existing ones), so every cached + podcast album is re-scanned with the new kind instead of being read back as + `"book"`. +- `_ANALYZED_KINDS` stays `("music",)`. +- `services/web/schemas.py`: `AlbumOut.kind` and `PlayerStateOut.kind` become the + `AlbumKind` literal instead of `str`, so the OpenAPI schema says what the values are. + +### Web + +- `src/api/types.ts`: `AlbumKind = "music" | "book" | "podcast"`. +- `src/lib/search.ts`, `groupOf`: map from `kind` alone (`podcast` → `"podcasts"`, + `book` → `"audiobooks"`, else `"music"`). No `section` comparison may remain + anywhere in `web/src`. +- `src/lib/covers.ts`: `isBook` and `aspectOfKind` treat `"book"` specially. Podcasts + are drawn like books today *because* they were `kind: "book"`. Keep them looking + exactly as they do now: decide explicitly whether `isBook` / `aspectOfKind` include + `"podcast"`, and check BrowseView's `mostlyBooks` / `allPodcasts` tiles and the play + view. The result must be visually unchanged; compare screenshots of the podcast + shelf, a podcast category and the podcast play view before and after. +- Anything else that reads `state.kind` or `album.kind`: grep and decide per hit. + +### Leave alone + +The `section` field stays in the API; it is still useful for display and debugging. +Don't rename the shelves, the folders or the `Group` names. + +## Tests + +- `tests/test_library.py`: a `Kinderpodcasts` episode scans as `kind == "podcast"`; a + cache written at the previous `_INDEX_VERSION` is discarded; `latest_episode` still + works. +- `tests/test_tippen_rewards.py` / `test_tippen_api.py`: episode rewards still resolve + and still animate as `podcast_episode`. +- `tests/test_web.py`: `/api/library` reports `"podcast"`. +- `web/src/lib/__tests__/search.test.ts`: `groupOf` uses `kind`; an album whose section + says `Kinderpodcasts` but whose kind is `music` is music. That proves nothing compares + the section any more. + +## Done when + +`scripts/check.sh` passes, `grep -rn "Kinderpodcasts" web/src` finds nothing, the +backend's "is it a podcast" checks all read `album.kind`, and the podcast UI looks the +same as before. diff --git a/docs/agent-tasks/07-backend-and-frontend-cleanups.md b/docs/agent-tasks/07-backend-and-frontend-cleanups.md new file mode 100644 index 0000000..18c3281 --- /dev/null +++ b/docs/agent-tasks/07-backend-and-frontend-cleanups.md @@ -0,0 +1,222 @@ +# Task: eight independent cleanups + +Each section below is a separate change: do them as separate commits, in any order, and +run `scripts/check.sh` after each. Each comes from a code review of commit `a55cc4f`; +the "why" says what is wrong and, where known, which commit introduced it. Do not +change behaviour beyond what a section asks for. + +--- + +## A. Races in library updates and concurrent rescans (review item 7) + +**Why.** `MusicLibrary.refresh()` and `MusicLibrary._publish_analysis()` +(`python-backend/musicmouse/library/__init__.py`) both do +`self._entries = await asyncio.to_thread(...)` from a snapshot taken before the await. +Whichever finishes last wins, so an analysis batch that started before a rescan +finished can overwrite the freshly scanned index (a new podcast episode disappears +until the next rescan). Both also write `index.json` from worker threads at the same +time. Rescans are not serialised either: `POST /api/library/refresh` fires an +unretained `asyncio.create_task` (`# noqa: RUF006` in `services/web/api.py`), and the +podcast poller rescans on its own (`App.rescan_library`, introduced with the poller in +57afc32). + +Separately, `VlcPlayer` (`devices/player.py`) mutates `_playing`, `_index` and +`_playlist_changed` both from libVLC's callback thread (`_on_playing`, `_on_stopped`, +`_on_playlist_end`, `_on_next_item`) and from the event loop (`_poll`, the actions). + +**Do.** +- One `asyncio.Lock` in `MusicLibrary` held around every read-modify-write of + `_entries` plus its `store_index`: the whole of `refresh()`, and the fold-swap-store + in `_publish_analysis()`. The analysis workers keep running while a scan holds the + lock; only publishing waits. +- `App.rescan_library` coalesces. While one rescan runs, further calls wait for the + running one plus at most one follow-up, not N parallel scans. Keep a reference to the + task the refresh endpoint starts, and drop the `noqa`. +- `VlcPlayer`'s libVLC callbacks do nothing but hop onto the loop (the bus already + knows the loop; give `PlayerBase` a small `_on_loop(fn)` using + `loop.call_soon_threadsafe`). All state mutation then happens on the loop thread. + Update the comment above the callbacks. + +**Test.** A library test with a scan made artificially slow (an injected scanner or a +patched `scan_library`) and an analysis publish racing it. The new album must survive. +Two concurrent `rescan_library` calls run the scanner at most twice. + +--- + +## B. Library cache invalidation gaps (review item 8) + +**Why.** `scan_library` (`library/scanner.py`) reuses a cached folder album whenever +its *audio files'* names, sizes and mtimes are unchanged (`Fingerprint`). So: +1. changing a figure's `kind:` in `config.yml` has no effect until the figure's files + change: the cached album keeps the old kind; +2. a `cover.jpg` (or any of `_COVER_NAMES`) added to an album folder later is never + picked up. Podcast episodes include their sidecar covers in the fingerprint; folder + albums do not; +3. covers of albums that no longer exist (deleted folders, episodes pruned by the + podcast poller) stay in `cache/covers/` forever. + +**Do.** +- Folder albums: include the existing `_COVER_NAMES` files in the fingerprint, like + `scan_episodes` does for sidecars. +- Figure albums: reuse a cached entry only if its `kind` equals the configured one. +- After a scan, delete `covers/.jpg` and `covers/.thumb.jpg` for ids not in + the new index. Do **not** garbage-collect `analysis/`: it is expensive, content-keyed + and documented as safe to keep. +- Bump `_INDEX_VERSION` with a comment line, because the fingerprint shape changes. + +**Test.** One test per gap in `tests/test_library.py`. + +--- + +## C. The LED "off" animation is sent twice when a figure is removed (review item 11) + +**Why.** Removing a figure makes `reactions/lighting.figure_placed_or_removed` run +`off_animation`. The player pausing then emits `PlaybackChanged(playing=False)`, and +`web_playback`, whose guard is "no figure on the reader", runs `off_animation` again. +That makes 6 LED writes instead of 3 and restarts the reverse swipe halfway through. +`web_playback` came in with the web front-end (edb6e5e). + +**Do.** Make `off_animation` idempotent: skip a zone whose current effect +(`app.mouse.effect(zone)`) is already the off effect it would send, and skip the button +brightness when it is already 0. That keeps both reactions simple and covers any future +third caller. + +**Test.** In `tests/test_reactions.py`: removing a figure while playing produces +exactly three `LedEffectChanged` events, one per zone. + +--- + +## D. `config.yml` has no single owner (review item 13) + +**Why.** +- `PUT /api/settings` (`services/web/api.py`) applies values with `setattr` on the + pydantic config objects. That skips validation, so the handler re-implements the + min/max/initial volume checks by hand. +- `PUT /api/remote/mapping` replaces `app.config.remote`. +- Both endpoints read, modify and write `config.yml` from worker threads + (`write_settings`, `write_mapping`) with no lock, so two saves at once can lose one. +- Memory is updated *before* the file is written, so a failed write leaves the running + config and the file disagreeing. + +**Do.** One small `ConfigStore` (for example `musicmouse/config_store.py`) owns the path, +the in-memory `Config` and a lock. An update: +1. validates the merged values through the real pydantic models, so the + `_check_volumes` validator runs; no duplicated checks in the endpoint; +2. writes the file (round-trip, comments kept, via the existing `load_document` / + `atomic_write`); +3. only then applies the values to the live objects. + +Objects that read the config live (`ScreenService` reads `auto_off_minutes`, the player's +volume limits) must keep seeing changes: either keep mutating the same objects after +validation, or have them read through the store. Pick one and say which in the +docstring. The settings and remote-mapping endpoints both go through the store. If +`docs/agent-tasks/01-web-authentication.md` has landed, its `--set-password` command +should use it too. + +**Test.** Invalid settings are rejected by the model's own validator, with an +unchanged file and unchanged memory. Two concurrent saves (settings and mapping) both +end up in the file. + +--- + +## E. Generate the front-end's API types (review item 15) + +**Why.** `web/src/api/types.ts` (~300 lines) hand-mirrors +`python-backend/musicmouse/services/web/schemas.py`. FastAPI already describes every +route and model at `/api/openapi.json`. (A third copy lived in the Slint front-end, +which has been removed.) + +**Do.** +- A backend test (for example `tests/test_openapi.py`) builds the app the way + `tests/test_web.py` does and compares `api.openapi()` with a committed + `web/src/api/openapi.json`. With `UPDATE_OPENAPI=1` it rewrites the file instead. The + test fails whenever the API changes and the snapshot was not refreshed. +- `web`: add `openapi-typescript` as a dev dependency and an `npm run gen:api` script + that generates `src/api/schema.d.ts` from that JSON. Make `npm run check` fail when + the generated file is stale: use the tool's own check mode if it has one (verify), + otherwise regenerate to a temp file and diff. +- `types.ts` keeps its exported names (`Album`, `PlayerState`, ...) as aliases of + `components["schemas"][...]`, so no component changes. Move the useful doc comments + onto the pydantic fields (`Field(description=...)`), where they reach both sides. +- The websocket messages (`ServerMessage`) are not in OpenAPI. Keep them hand-written + in `types.ts` with a comment saying so. + +--- + +## F. Copy-pasted helpers (review item 16) + +**Why.** +- `ScreenService.blubb` (`services/screen.py`) is a line-for-line copy of + `sound.play_wav`. `sound.py` was extracted in a55cc4f, but the screen service was + never moved onto it. +- The "did a person do this?" filter (an `IntentEvent`, not `LedEffectRequested`, not + `source == "system"`) is duplicated in `ScreenService._on_event` and + `TimerService._on_event`. +- There are five atomic-write helpers: `library/cache._write_atomic`, + `services/web/settings.atomic_write`, `tippen/progress.save_progress`, + `school_days.save_calendar` and `library/podcast_feeds._save_failed`. Three of them + name the temp file `.tmp`, which collides when two threads of the same + process write the same file. That really happens: settings and remote-mapping saves + both run in `asyncio.to_thread`. + +**Do.** +- `ScreenService.blubb` calls `play_wav(self.alsa_device, wav, timeout=self.config.timeout)`. +- `is_person(event: Event) -> bool` next to the event classes in `events.py`, used by + both services (keep each service's own extra exclusions). +- One `write_atomic(path, text)` (for example `musicmouse/fileio.py`) using + `tempfile.NamedTemporaryFile(dir=path.parent, delete=False)` and `os.replace`, used by + all five. Keep the temp file a dotfile where it sits in the music library, so the + scanner never picks it up. The YAML writer dumps to a string first. + +**Test.** The existing tests cover behaviour. Add one for `write_atomic`, two threads +writing the same path at once, which today can fail. + +--- + +## G. Inject time everywhere (review item 17) + +**Why.** The backend's rule (see `musicmouse/clock.py`) is that anything that waits takes a +`Clock`, and `TimerService` takes a `wall` callable for the date. Exceptions: +- `PodcastFeedService.run` and `StateHub.run` call `asyncio.sleep` directly; +- `services/web/state.clock_state` uses `date.today()`; +- `POST /api/timer` (`services/web/api.py`) uses `datetime.now()`. + +**Do.** Give `PodcastFeedService` and `StateHub` a `clock`, as the other services have, +wired in `__main__._build_services` / `WebService`. Put one `wall: Callable[[], +datetime]` on `App` (default `datetime.now`) and use it in `TimerService`, +`clock_state` and the timer endpoint, so a test can pin "today" in one place. + +**Test.** The podcast service test runs on `FakeClock` instead of real sleeps; a +clock-state test pins the date through `App.wall`. + +--- + +## H. Break up the largest files (review item 19) + +**Why.** Every feature since the rearchitecture was added to the same few files. +- `web/src/App.tsx` is 909 lines with ~40 hooks, routes pages via flags, and + defines `ToolButton`, `CornerButton`, `ConnectionDot`, `Splash` and the assign toast + inline. +- `BrowseView.tsx` is 932 lines. +- About 300 static `style={{...}}` objects sit next to five CSS files. +- `services/web/api.py` is one router for seven features: library, state, playback, + parent mode, typing game, remote, Home Assistant. +- One sign of insertion rather than placement: the comment + `/** The one place a keyboard action, a click or a tap all end up. */` in `App.tsx` + sits above `saveMapping`, but describes `run`. + +**Do** (mechanical moves only, no behaviour change; one commit per bullet): +- `api.py`: one router module per feature under `services/web/routes/`, assembled in + `build_router`. URLs unchanged. +- `App.tsx`: move the four small components into `components/`, the remote-mapping + state into `hooks/useRemoteMapping.ts`, and the kiosk listeners (screen activity, + silencing a ringing timer) into `hooks/useKioskActivity.ts`. Put the misplaced + comment back above `run`. +- `BrowseView.tsx`: split by screen (root shelves, category, results), whichever seams + the file already has. +- Inline styles: move **static** style objects in `App.tsx`, `BrowseView.tsx` and + `ParentPanel.tsx` into classes in the existing CSS files. Leave dynamic ones (colours + from album data, computed sizes) inline. + +Check visually in the dev server, at desktop, phone (`usePhoneLayout`) and `?pi=1` +sizes, that nothing moved. diff --git a/docs/agent-tasks/README.md b/docs/agent-tasks/README.md new file mode 100644 index 0000000..965d5d5 --- /dev/null +++ b/docs/agent-tasks/README.md @@ -0,0 +1,20 @@ +# Agent tasks + +Self-contained instructions for fixes that came out of a code review of commit `a55cc4f`. +Each file says why, what the owner decided, the design, the tests, and when it is done. +The numbers are the review's item numbers. + +| File | What | +|---|---| +| `01-web-authentication.md` | Password login, once per browser for a year, in front of the whole `/api` (including the Home Assistant proxy). | +| `04-one-input-policy.md` | Podcast next/previous seeks 30 s from every input; mute restores the previous volume; three separate volume steps in parent mode. | +| `05-firmware-serial-frames.md` | Stop RFID frames interleaving with button frames on the serial link. | +| `06-podcast-kind.md` | `"podcast"` as an album kind instead of comparing the `Kinderpodcasts` folder name. | +| `07-backend-and-frontend-cleanups.md` | Eight independent cleanups: library races, cache invalidation, double LED off-animation, a config owner, generated API types, shared helpers, injected time, splitting the largest files. | + +Order: `06` before `04` (04's podcast check uses 06's kind; it says how to proceed if +06 has not landed). `07 D` is easier after `01`. The rest are independent. + +For every task: one commit per logical change, `scripts/check.sh` passing before each +commit, no pushing. When a task is done, delete its file in the same commit, so this +folder only ever lists open work.