# 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/<id>.jpg` and `covers/<id>.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 `<name>.tmp<pid>`, 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.