musicmouse/docs/agent-tasks/07-backend-and-frontend-cleanups.md

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.