Add agent task files for the review's remaining fixes
Authentication (01), one input policy for skip/mute/volume steps (04), the firmware's interleaved serial frames (05), podcast as an album kind (06), and eight smaller cleanups (07). Each says why, what was decided, the design, the tests and when it is done. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
222
docs/agent-tasks/07-backend-and-frontend-cleanups.md
Normal file
222
docs/agent-tasks/07-backend-and-frontend-cleanups.md
Normal file
@@ -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/<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.
|
||||
Reference in New Issue
Block a user