ledgrab/AUDIT_REMAINING.md

# LedGrab Architecture Audit — Remaining Items

Roadmap for the architecture-audit refactor sprint that started 2026-05-22.
This file lists every audit finding that is **not yet addressed**; the ones
already landed in commits `563cbac..2f15fbb` are summarised below for
context.

## Already done (10 commits)

| Commit | Findings addressed |
|---|---|
| `563cbac` | C2, C11, C1 (parallel-change only), C3, C4, C6, C7-streams |
| `29bdacf` | C5 (HA/Z2M swap helper; full ABC deferred) |
| `97dae2c` | H1 |
| `5fec8db` | M4 |
| `98fb61d` | H2 |
| `9f3f346` | M5 |
| `05f73ee` | H6 (bindable extraction only) |
| `3b8f00e` + `c1aa2eb` | C7 store-side |
| `2f15fbb` | H3 |
| _uncommitted (2026-05-27 autonomous pass)_ | H6-rest, H8, M7 (foundation + 3 reference files) |

All commits have ≥1 code-review subagent pass with HIGH findings fixed
before commit. Tests pass on each commit; ruff clean; tsc + bundle build
clean for the frontend commit.

The two CRITICAL **data-safety** items (C2 silent CSS fallback, C11
string-replace JSON migration) are fixed. The two CRITICAL
**parallel-change** problems for color-strip + value-source dispatch are
fixed. The two HIGH dispatch problems (H1 effects, H2 rules) are fixed.

---

## Remaining backend items

### HIGH

#### H4 — `Device.__init__` 40+ params mixing per-type fields

**File:** `server/src/ledgrab/storage/device_store.py:46-150`

The `Device` dataclass constructor accepts ~40 parameters that mix common
fields with DMX-only / DDP-only / Hue-only / Yeelight-only / Wiz-only /
LIFX-only / Govee-only / Nanoleaf-only / SPI-only / Chroma-only /
GameSense-only fields. Setting `hue_username` on a WLED device is
silently ignored.

**Approach:** introduce per-device-type config dataclasses
(`DmxConfig`, `HueConfig`, `DdpConfig`, …) and make `Device.config` a
discriminated union. Per-type validation moves to the config classes.
Wire migration: every existing device row needs to be re-parsed; use the
versioned `MigrationRunner` introduced in Phase 1.2.

**Risk:** medium-high. Touches:
- `storage/device_store.py` — Device dataclass, `from_dict`, `to_dict`,
  `create_device`, `update_device`
- `api/schemas/devices.py` — Pydantic schemas
- `api/routes/devices.py` — request validation
- `core/devices/*` — every provider reads device fields
- A new migration to translate flat fields → nested `config`

**Estimated scope:** ~1500 LOC diff, 1-2 dedicated sessions.

#### H5 — `WledTargetProcessor` god class (32 methods, 5 responsibilities)

**File:** `server/src/ledgrab/core/processing/wled_target_processor.py` (1238 LOC)

Conflates:
1. Device connectivity (probe, liveness, reconnect)
2. FPS negotiation (adaptive_fps, keepalive_interval, state_check_interval)
3. LED resampling (`_fit_to_device` — 60 lines of numpy)
4. Preview WebSocket fanout (`_preview_clients`, `_broadcast_led_preview`)
5. Metrics emission (`get_state`, `get_metrics`)

**Approach:** extract `WledDeviceConnector`, `WledPixelSender`,
`TargetFitProcessor`, `TargetPreviewBroadcaster`, `TargetMetricsCollector`.
`WledTargetProcessor` becomes an orchestrator that composes them.

**Risk:** HIGHEST in the audit. This class drives physical LED hardware
in production. A regression caught at runtime (in the user's living
room) is the expensive failure mode. Needs manual verification with at
least one real WLED device after the refactor.

**Coupled with:** C5 (HA/Z2M shared the same shape; should extract a
common `BaseTargetProcessor` ABC at the same time so all three
processors share lifecycle / preview / metrics code).

**Estimated scope:** ~2000 LOC diff, 2-3 dedicated sessions, with manual
device testing after each.

#### H7 — `device-discovery.ts` 1745 LOC

Frontend mirror of H4. The `onDeviceTypeChanged` handler has a giant
switch with 15+ device kinds and 15+ `_showXxxFields` / `_buildXxxItems`
helpers. Adding a device type requires editing 5 separate frontend hooks.

**Approach:** mirror the H4 backend redesign — once the storage layer
has per-type config objects, the frontend can have a per-type field-set
registry. Best done **after** H4 lands so the schemas drive the
registry.

**Estimated scope:** 1-2 sessions; coupled to H4.

#### H8 — `automations.ts` 1410 LOC — ✅ DONE (uncommitted, 2026-05-27)

Frontend mirror of H2 (rule polymorphism). Already addressed on the
backend in `98fb61d`; the frontend dispatch on `RuleType` was
hand-rolled.

**Done:** the two remaining hand-rolled dispatch ladders were converted
to registries keyed by `RuleType`, alongside the pre-existing
`RULE_CHIP_RENDERERS`:
- `RULE_FIELD_RENDERERS` — the `renderFields` if/elif ladder was
  extracted into module-level `_renderXxxFields(container, data)`
  functions (they only ever closed over `container`); the in-row
  `renderFields` is now a 3-line dispatcher.
- `RULE_COLLECTORS` — the `getAutomationEditorRules` if/elif ladder
  became per-type collectors; the loop is now a registry lookup.
- All three registries are typed `Record<RuleType, …>` (compile-time
  exhaustiveness) and an import-time `_assertRuleHandlerCoverage()`
  logs loudly if any registry drifts from `RULE_TYPE_KEYS`. (Frontend
  logs rather than throws — a thrown error at import would brick the
  whole bundle, not just the editor — the one intentional divergence
  from the backend's raising `_assert_rule_handler_coverage`.)

Adding a new rule type now means: one entry in `RULE_TYPE_KEYS`,
`RULE_TYPE_ICONS`, and each of the three registries — and tsc + the
coverage check flag any omission.

Verified: tsc + bundle build clean; typescript-reviewer APPROVE (the
extracted renderer bodies are byte-identical to the originals; no stray
closure captures; http_poll widget-stash + HA entity loading preserved).

### MEDIUM

#### M1 — `ProcessorManager.add_target` shotgun (11 args, WLED-leak)

**File:** `server/src/ledgrab/core/processing/processor_manager.py:396`

Method is named generically (`add_target`) but accepts `protocol="ddp"`
and `keepalive_interval` — WLED-only fields. HA and Z2M have sibling
methods with their own bespoke params.

**Approach:** extract a `TargetFactory` (per-kind builders, similar to
`value_source_factories.py` from Phase 7). Couple with H5/C5 work.

#### M2 — `TargetContext` god-bag

**File:** `server/src/ledgrab/core/processing/processor_manager.py`

`@dataclass TargetContext` exposes ~8 attributes (device_store,
color_strip_stream_manager, value_stream_manager, metrics_history,
mqtt_manager, ha_manager, …). Processors silently depend on whichever
fields they read. Tests have to construct a huge mock context.

**Approach:** make per-processor explicit dependency injection. Couple
with H5 work.

#### M3 — Validation duplicated across layers

Field-level constraints (composite nesting depth, name uniqueness, span
ranges) are enforced in route + schema + store. Adding a new constraint
means editing 3 places.

**Approach:** move all validation to the model/schema layer (Pydantic
validators + dataclass `__post_init__`). Routes trust the schema; store
trusts the model.

**Risk:** moderate — cross-cutting; needs careful review of which layer
currently owns which constraint.

#### M6 — `ws_stream.py` mixed concerns (699 LOC)

**File:** `server/src/ledgrab/api/routes/color_strip_sources/ws_stream.py`

The worst part (stream-creation dispatch) was fixed in Phase 2.1 — it
now calls `color_strip_kinds.build_stream(source, deps)`. The remaining
699 lines mix config parsing + WebSocket lifecycle + frame loop. Could
extract the frame loop into a separate `PreviewFrameLoop` class.

**Estimated scope:** half a session. Low impact since the parallel-change
problem is already fixed.

#### M7 — No shared frontend API client — 🟡 FOUNDATION DONE (uncommitted, 2026-05-27)

**File:** every `static/js/features/*.ts`

`fetchWithAuth(...)` + bespoke error-unwrapping is copy-pasted in every
feature's save / load function. ~45 files, ~243 call sites.

**Done:** `static/js/core/api-client.ts` now provides typed
`apiGet` / `apiPost` / `apiPut` / `apiPatch` / `apiDelete` that wrap
`fetchWithAuth` (so auth, 401-relogin, retry, timeout, and the offline
toast are unchanged) and collapse the repeated
`if (!resp.ok) { detail || HTTP <status> } … resp.json()` dance into one
call returning a typed body and throwing `ApiError` on failure. The
`detail` unwrap is hardened to join FastAPI validation arrays instead of
stringifying to `[object Object]`. **35 feature/core files migrated**
(covers GET/POST/PUT/DELETE, typed response bodies, custom i18n error
messages, silent-failure GETs, bulk `Promise.allSettled` deletes,
inline-error saves, array-`detail` joins, fire-and-forget POSTs, and
local catch handling) — reviewer-approved for behaviour parity across
the riskier divergences. Migrated files include the integration sources
(weather / HA / MQTT / HTTP), the template families (capture / audio /
audio-processing / pattern), the scene-preset CRUD, the simple-CRUD
entity files (sync-clocks / audio-sources / game-integration /
gradient / displays / device-discovery), the light-target editors
(z2m / ha), the preferences modules (dashboard-layout / card-modes /
notifications-watcher), the calibration editors (simple + advanced),
the entire `automations.ts` and `devices.ts` CRUD surfaces, and several
core utilities (`api-client.ts` itself, `cache.ts`, `command-palette.ts`,
`graph-connections.ts`, `tag-input.ts`, `process-picker.ts`,
`perf-charts.ts`, `icon-picker.ts`, `update.ts`, `integrations.ts`).

Also added **14 new locale keys** (en / ru / zh) so the fallback
messages the migration surfaces — `pattern.error.save_failed`,
`audio_processing.error.save_failed`, `audio_template.error.save_failed`,
`audio_template.error.load_failed`, `templates.error.save_failed`,
`templates.error.load_failed`, `gradient.error.save_failed`,
`target.error.load_failed`, `device.error.load_failed`,
`automations.error.{load,save,delete,toggle}_failed`, plus
`gradient.error.delete_failed` for ru/zh — are translated instead of
hardcoded English. A scan confirms **no `errorMessage: '<English>'`
strings remain** in the migrated diff.

**Remaining:** 9 feature files (~94 call sites). All but one are the
big god-modules whose migration is best done as part of their C8/C9/C10
splits: `streams.ts` (18), `settings.ts` (18), `targets.ts` (16),
`dashboard.ts` (15), `color-strips/index.ts` (8), `graph-editor.ts` (7),
`assets.ts` (6 — also blocked by multipart upload + blob download paths
that legitimately bypass the JSON client), and `value-sources.ts` (5).
The lone leaf file still on `fetchWithAuth` is `pairing-flow.ts` (1) —
its branching on raw `Response.status` codes (200 / 409 / 4xx) doesn't
fit the api-client contract, so it stays on raw fetch by design.
Migration is mechanical but **not** a blind find/replace — each site
carries its own localised error key that must be preserved as the
`errorMessage` option, and binary/multipart endpoints (e.g.
`assets.ts` file upload / blob download) must stay on raw
`fetchWithAuth` (the client is JSON-only). Each migrated file ideally
gets manual UI smoke-testing. **Behaviour note:** migrated GET sites now
prefer the server's `detail` over the generic localised fallback when
present — matching what the write paths already did; intended, but
user-visible.

#### M8 — Global `_cached*` `let` vars

Mutable module-level state mutated from multiple feature modules. No
subscription model — features manually `invalidate()` after CRUD.

**Approach:** introduce a reactive cache (EventEmitter pattern or a tiny
store like Nano Stores). Couple with M7 (the API client can drive cache
invalidation on write).

#### M9 — `dashboard.ts` 1421 LOC

Frontend god-module orchestrating + rendering device / target / CSS
cards. Couple with C8/C9/C10 frontend split work.

#### M10 — Duplicate frontend modal classes

`ValueSourceModal`, `StreamEditorModal`, `TargetEditorModal`,
`AddDeviceModal`, etc. each reimplement pristine-check / undo / focus
management.

**Approach:** introduce a `FormModal<T>` base class.

#### M11 — Hardcoded `_getSectionForSource` / `_getTabForSource`

Routing tables duplicated across multiple feature files (streams.ts,
value-sources.ts). Adding a new stream type requires hunting strings.

**Approach:** single routing registry keyed by source_type.

#### M12 — Late imports masking cycles

Partially addressed by the kind registries (Phase 2.1, 2.2). Some
late-imports still exist in `value_stream.py`, `audio_stream.py`, the
target processors. Resolving them requires restructuring module layout
to break the circular dependencies.

**Estimated scope:** small follow-up after H5.

### LOW

#### L1 — `(src as any).field` casts in `value-sources.ts`

Discriminated unions aren't narrowed properly. Couple with C8 frontend
split.

#### L2 — Mutable state without locks

`_preview_clients`, `_last_preview_data`, `_color_stream`,
`_css_stream` are mutated from multiple async tasks without explicit
locks. Production has not exhibited issues but the contract is fragile.

**Approach:** add explicit `asyncio.Lock` per processor. Couple with H5.

#### L3 — `Calibration.validate()` raises instead of returning result

**File:** `server/src/ledgrab/core/capture/calibration.py:164`

All 4 call sites currently rely on the raise; converting to
`ValidationResult` would force every caller to check a return value
without adding safety. **Recommendation:** skip — current design is
appropriate.

#### L4 — `_SOURCE_TYPE_MAP` is module-private

No public `GET /api/v1/source-types` discovery endpoint. Frontend
hardcodes the list of source types in `types.ts`.

**Approach:** add a discovery route + matching frontend fetch. Couple
with H6 frontend split (since `types.ts` is involved).

#### L5 — `AudioValueStream` implicit state machine

**File:** `server/src/ledgrab/core/processing/value_stream.py:169-383`

`get_value()` can be called before `start()`; transitions are implicit.
**Approach:** explicit State pattern. Low value (production callers
always start before reading).

---

## Remaining frontend items (all)

### CRITICAL

- **C8** — `value-sources.ts` 1972 LOC (4 god-functions, type-dispatch ladders)
- **C9** — `graph-editor.ts` 2707 LOC (layout + interaction + state + WS sync + …)
- **C10** — `streams.ts` 2341 LOC (picture / audio / template kitchen-sink)

### Other frontend (severity in main list above)

- **H6 rest** — ✅ DONE (uncommitted, 2026-05-27): `types.ts` (1140 LOC)
  split into 18 per-entity files under `types/` (joining the existing
  `bindable.ts`); `types.ts` is now a ~200-line pure re-export barrel, so
  every `import { … } from '../types.ts'` still resolves. Reviewer
  confirmed all 102 exported symbols preserved, none renamed.
- **H7** — `device-discovery.ts` 1745 LOC (couple with H4)
- **H8** — `automations.ts` 1410 LOC (mirror H2)
- **M7** — shared API client
- **M8** — reactive cache
- **M9** — `dashboard.ts` 1421 LOC
- **M10** — `FormModal<T>` base
- **M11** — routing registry
- **L1** — narrowing the discriminated unions

The frontend remainder is **multi-day work** even when broken up by
finding. Recommended approach: a dedicated frontend sprint with the
typescript-reviewer agent + manual UI testing for each god-module
split. Order:

1. Finish `types.ts` split (H6) — pure organisation, low risk, unblocks
   the rest
2. Introduce API client (M7) — every feature file gains a cleaner shape
3. Split `value-sources.ts` (C8) — uses the API client + per-type
   registry pattern
4. Split `streams.ts` (C10)
5. Split `graph-editor.ts` (C9) — needs the most care; the file owns
   the entire visual editor
6. Polish: `dashboard.ts` (M9), `device-discovery.ts` (H7),
   `automations.ts` (H8), `FormModal` (M10), routing registry (M11),
   reactive cache (M8), narrowing (L1)

---

## Recommended ordering for future sessions

### Session A — Frontend sprint (multi-day)

Address H6-rest, C8, C9, C10, H7, H8, M7-M11, L1. See order above.
Critical to have typescript-reviewer feedback + manual UI testing after
each split.

> **Progress (2026-05-27, uncommitted):** steps 1 & 2 of the order above
> are done — H6-rest (`types.ts` split) and M7-foundation (`api-client.ts`
> + 3 reference migrations). H8 (automations registry) also landed. Still
> open: C8, C9, C10, H7, the remaining ~40 M7 file migrations, M8-M11, L1.
> Next per the order: introduce the API client everywhere (finish M7),
> then split `value-sources.ts` (C8).

### Session B — Device redesign (1-2 sessions)

Address H4 alone. Touches device storage + provider classes; needs a
data migration. Once H4 lands, H7 frontend mirror can follow.

### Session C — BaseTargetProcessor ABC (2-3 sessions)

Address C5 (full) + H5 + M1 + M2 + L2 together. Highest risk in the
audit because it drives physical LED hardware. Each step needs manual
verification with a real device.

### Session D — Polish (half a session)

Address M3, M6 (remainder), M12 (remainder), L3 (decision: skip), L4,
L5.

---

## Pattern reference for new contributors

Three registry-pattern templates that already exist in the codebase and
should be the model for the remaining dispatch ladders:

1. **Class-level handler dict + import-time coverage assertion**
   - `core/processing/effect_stream.py::_RENDERERS`
     (`@_effect_renderer` decorator + `@_collect_effect_renderers`
     class decorator)
   - `core/automations/automation_engine.py::AutomationEngine._RULE_HANDLERS`
     (module-level binding after class definition)
   - `api/routes/output_targets.py::_TARGET_RESPONSE_BUILDERS`
     (response-shape dispatch keyed by storage class)

2. **Per-type free functions + dependency-bag dataclass**
   - `core/processing/color_strip_kinds.py` (`StreamDeps` + `STREAM_BUILDERS`)
   - `core/processing/value_kinds.py` (`ValueStreamDeps` + `STREAM_BUILDERS`)
   - `storage/value_source_factories.py` (`CREATE_BUILDERS` + `UPDATE_APPLIERS`)

3. **Versioned migration runner**
   - `storage/data_migrations.py` (`MigrationRunner` + `DataMigration` ABC)
   - Used for any storage rename / field-shape change in the future.
   - Audit-table contract: atomic transaction covers
     applied-check + apply + record, so partial-failure cannot leave
     data rewritten but unrecorded.

Adding a new feature that touches dispatch should reach for one of
these three patterns before writing a fresh if/elif chain.