refactor(devices): per-provider typed configs (phases 1-4)
Phase 1 — DeviceConfig hierarchy (device_config.py): - 17 @dataclass(frozen=True) subclasses (WLEDConfig, AdalightConfig, …) sharing BaseDeviceConfig; DeviceConfig = Union[all 17] - Device.to_config() in device_store.py: single flat→typed dispatch point Phase 2+3 — Typed provider signatures + call-site migration: - ProviderDeps(device_store) frozen dataclass in led_client.py - LEDDeviceProvider.create_client(config, *, deps) abstract signature - create_led_client(config, *, deps) factory dispatches via config.device_type - All 17 providers narrowed to their specific config type; drop kwargs.get() - GroupLEDClient.connect() uses device.to_config() + create_led_client() - wled_target_processor: replaced 21-field DeviceInfo unpacking with to_config() + dataclasses.replace(config, use_ddp=…) for DDP override - device_test_mode: build typed config via to_config() + ProviderDeps - Deleted DeviceInfo dataclass, _get_device_info(), _DEVICE_FIELD_DEFAULTS - TargetContext: replaced get_device_info callback with is_test_mode_active Phase 4 — Test migration: - 47-case test suite in tests/core/devices/test_device_config.py (100% coverage) - test_group_device.py TestGroupLEDClient migrated to GroupConfig + ProviderDeps - Removed legacy keyword-arg init path from GroupLEDClient
This commit is contained in:
@@ -0,0 +1,274 @@
|
||||
# Refactor Plan: Per-Provider Typed Device Configs
|
||||
|
||||
**Status:** Planned, not started.
|
||||
**Target branch:** `refactor/device-typed-configs`
|
||||
**Intended executor:** Sonnet agent (one phase per invocation; human review between phases).
|
||||
|
||||
## Goal
|
||||
|
||||
Replace the flat [`DeviceInfo`](../../server/src/ledgrab/core/processing/target_processor.py) dataclass (and the `**kwargs`-based `LEDDeviceProvider.create_client(url, **kwargs)` contract) with a **discriminated union of per-provider config dataclasses**. Each provider owns its config type and reads typed fields instead of guessing kwargs.
|
||||
|
||||
## Motivation
|
||||
|
||||
Current pain points:
|
||||
|
||||
- [server/src/ledgrab/core/processing/wled_target_processor.py](../../server/src/ledgrab/core/processing/wled_target_processor.py) unpacks ~21 fields by hand into `create_led_client(**kwargs)`.
|
||||
- Every provider's `create_client` starts with `kwargs.get("x", default)` — no type safety, no IDE hints, no way to know at a glance which fields a provider actually uses.
|
||||
- Adding a new per-device-type field requires threading it through `Device` → `DeviceInfo` → `_DEVICE_FIELD_DEFAULTS` → call-site unpacking → kwargs bag → provider.
|
||||
- Fields leak across device types (a WLED device carries `ble_govee_key=""` at runtime for no reason).
|
||||
|
||||
## Scope guardrails
|
||||
|
||||
- **Storage schema (SQLite) unchanged.** Columns stay, dead-for-this-type fields stay, no destructive migration.
|
||||
- **Frontend HTML/TS unchanged in phases 1-4.** It already branches on `device_type` with show/hide logic. Frontend changes are deferred to Phase 5.
|
||||
- **API schemas are last.** Phase 5 converts `DeviceCreate`/`DeviceUpdate`/`DeviceResponse` to a Pydantic v2 discriminated union. This is the only breaking external change and can be deferred indefinitely if needed.
|
||||
|
||||
---
|
||||
|
||||
## Phase 1 — Config hierarchy (foundation, non-breaking)
|
||||
|
||||
### Create
|
||||
|
||||
**File:** `server/src/ledgrab/core/devices/device_config.py`
|
||||
|
||||
Pattern:
|
||||
|
||||
```python
|
||||
from dataclasses import dataclass
|
||||
from typing import List, Literal, Optional, Union
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class BaseDeviceConfig:
|
||||
device_id: str
|
||||
device_url: str
|
||||
led_count: int
|
||||
software_brightness: int = 255
|
||||
test_mode_active: bool = False
|
||||
auto_shutdown: bool = False
|
||||
rgbw: bool = False
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class WLEDConfig(BaseDeviceConfig):
|
||||
device_type: Literal["wled"] = "wled"
|
||||
use_ddp: bool = False
|
||||
|
||||
# ... one @dataclass(frozen=True) per provider
|
||||
```
|
||||
|
||||
### Config field inventory
|
||||
|
||||
Base: `device_id`, `device_url`, `led_count`, `software_brightness`, `test_mode_active`, `auto_shutdown`, `rgbw`.
|
||||
|
||||
| Config | Extra fields beyond Base |
|
||||
| -------------- | ------------------------ |
|
||||
| WLEDConfig | `use_ddp: bool = False` |
|
||||
| AdalightConfig | `baud_rate: Optional[int] = None` |
|
||||
| AmbiLEDConfig | `baud_rate: Optional[int] = None` |
|
||||
| DMXConfig | `dmx_protocol`, `dmx_start_universe`, `dmx_start_channel` |
|
||||
| ESPNowConfig | `baud_rate`, `espnow_peer_mac`, `espnow_channel` |
|
||||
| HueConfig | `hue_username`, `hue_client_key`, `hue_entertainment_group_id` |
|
||||
| SPIConfig | `spi_speed_hz`, `spi_led_type` |
|
||||
| ChromaConfig | `chroma_device_type` |
|
||||
| GameSenseConfig| `gamesense_device_type` |
|
||||
| BLEConfig | `ble_family`, `ble_govee_key` |
|
||||
| GroupConfig | `group_mode`, `group_device_ids` (**no `device_store` here** — see Phase 2) |
|
||||
| OpenRGBConfig | `zone_mode` |
|
||||
| MockConfig | `send_latency_ms: int = 0` |
|
||||
| DemoConfig | `send_latency_ms: int = 0` |
|
||||
| MQTTConfig | (none) |
|
||||
| WSConfig | (none) |
|
||||
| USBHIDConfig | (none — `hid_usage_page` is parsed from the URL, not config) |
|
||||
|
||||
```python
|
||||
DeviceConfig = Union[
|
||||
WLEDConfig, AdalightConfig, AmbiLEDConfig, DMXConfig, ESPNowConfig,
|
||||
HueConfig, SPIConfig, ChromaConfig, GameSenseConfig, BLEConfig,
|
||||
GroupConfig, MQTTConfig, WSConfig, USBHIDConfig, OpenRGBConfig,
|
||||
MockConfig, DemoConfig,
|
||||
]
|
||||
```
|
||||
|
||||
### Add
|
||||
|
||||
**`Device.to_config() -> DeviceConfig`** in [server/src/ledgrab/storage/device_store.py](../../server/src/ledgrab/storage/device_store.py) (around lines 14-97 where `Device` lives).
|
||||
|
||||
- Dispatches on `self.device_type`.
|
||||
- Constructs the right subclass, pulling only relevant columns.
|
||||
- Ignores columns that don't apply to the type.
|
||||
- This is the **only** place that knows the flat→typed mapping.
|
||||
|
||||
### Do NOT touch in Phase 1
|
||||
|
||||
- Provider signatures (still `create_client(self, url, **kwargs)`).
|
||||
- `create_led_client` factory.
|
||||
- Any call site.
|
||||
- `DeviceInfo` itself.
|
||||
|
||||
### Acceptance
|
||||
|
||||
- New unit test `server/tests/core/devices/test_device_config.py`:
|
||||
- For each provider, build a `Device` with that `device_type`, call `to_config()`, assert right subclass and right fields.
|
||||
- Edge case: extra/irrelevant Device fields must not leak into the wrong config type.
|
||||
- `cd server && ruff check src/ tests/ --fix` — green.
|
||||
- `cd server && py -3.13 -m pytest tests/ --no-cov -q` — green (existing tests untouched, new test passes).
|
||||
- `cd server && npx tsc --noEmit` — green (no TS impact this phase, just a sanity check).
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 + Phase 3 — Provider API migration + call-site migration (single PR)
|
||||
|
||||
**These must land in one commit** because the provider signature change would otherwise break the 3 call sites immediately.
|
||||
|
||||
### Change the abstract base
|
||||
|
||||
[server/src/ledgrab/core/devices/led_client.py](../../server/src/ledgrab/core/devices/led_client.py):
|
||||
|
||||
```python
|
||||
class LEDDeviceProvider(ABC):
|
||||
@abstractmethod
|
||||
def create_client(self, config: DeviceConfig, *, deps: ProviderDeps) -> LEDClient: ...
|
||||
```
|
||||
|
||||
`ProviderDeps` is a tiny new dataclass:
|
||||
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class ProviderDeps:
|
||||
device_store: "DeviceStore"
|
||||
# Add future cross-cutting runtime deps here (http_client, etc.)
|
||||
```
|
||||
|
||||
`create_led_client`:
|
||||
|
||||
```python
|
||||
def create_led_client(config: DeviceConfig, *, deps: ProviderDeps) -> LEDClient:
|
||||
return get_provider(config.device_type).create_client(config, deps=deps)
|
||||
```
|
||||
|
||||
### Update every provider (17 files)
|
||||
|
||||
- Narrow signature per provider: e.g. `WLEDDeviceProvider.create_client(self, config: WLEDConfig, *, deps: ProviderDeps)`.
|
||||
- Drop all `kwargs.get("x")` lookups — read typed fields directly.
|
||||
- Providers that don't need `deps` just ignore it.
|
||||
- **GroupDeviceProvider** is the only current consumer of `deps`: reads `deps.device_store`.
|
||||
|
||||
### Call sites (3)
|
||||
|
||||
1. [server/src/ledgrab/core/processing/wled_target_processor.py](../../server/src/ledgrab/core/processing/wled_target_processor.py) lines ~120-148 — the 21-field unpacking. Replace with:
|
||||
```python
|
||||
config = device.to_config()
|
||||
self._led_client = create_led_client(config, deps=self._provider_deps)
|
||||
```
|
||||
`self._provider_deps` is plumbed in from `ProcessorManager` when the target processor is constructed.
|
||||
2. [server/src/ledgrab/core/processing/device_test_mode.py](../../server/src/ledgrab/core/processing/device_test_mode.py) lines 72-78 — minimal test-mode client. Build a synthetic config via a helper `_minimal_config_for_test_mode(device)` (keeps just `device_id`, `device_url`, `led_count`, `baud_rate`) and pass it.
|
||||
3. [server/src/ledgrab/core/devices/group_client.py](../../server/src/ledgrab/core/devices/group_client.py) lines 47-70 — child client construction inside the group. Same pattern: `child_config = child_device.to_config()`; pass `deps` through.
|
||||
|
||||
### Delete
|
||||
|
||||
- `DeviceInfo` dataclass in [server/src/ledgrab/core/processing/target_processor.py](../../server/src/ledgrab/core/processing/target_processor.py) lines 71-109.
|
||||
- `ProcessorManager._get_device_info()` and `_DEVICE_FIELD_DEFAULTS` in [server/src/ledgrab/core/processing/processor_manager.py](../../server/src/ledgrab/core/processing/processor_manager.py) lines 230-275 — `Device.to_config()` subsumes this. Verify no other callers via `ast-index usages "_get_device_info"`.
|
||||
|
||||
### Acceptance
|
||||
|
||||
- `ast-index search "device_info\."` — no hits in non-test code.
|
||||
- `ast-index search "DeviceInfo"` — no hits outside archival comments.
|
||||
- `cd server && py -3.13 -m pytest tests/ --no-cov -q` — all tests pass.
|
||||
- Manual smoke: start server, create a WLED device, start processing, verify LEDs update (or mock output shows frames).
|
||||
- `cd server && ruff check src/ tests/ --fix` — green.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4 — Test migration
|
||||
|
||||
Update these files:
|
||||
|
||||
- `server/tests/storage/test_device_store.py` — add `to_config()` cases per device type.
|
||||
- `server/tests/api/routes/test_devices_routes.py` — should be mostly untouched (API schemas still flat until Phase 5).
|
||||
- `server/tests/e2e/test_device_flow.py` — update internal assertions only if they touch `DeviceInfo` directly.
|
||||
- `server/tests/test_group_device.py` — construct child clients with `GroupConfig`.
|
||||
- Any fixture helper that builds a fake `DeviceInfo` — migrate to the right `*Config` subclass.
|
||||
|
||||
### Acceptance
|
||||
|
||||
- `cd server && py -3.13 -m pytest tests/ --no-cov -q` — all green.
|
||||
- Coverage of `device_config.py` and `Device.to_config()` ≥ 90%.
|
||||
|
||||
---
|
||||
|
||||
## Phase 5 — API discriminated union (OPTIONAL, separate PR)
|
||||
|
||||
**Do not start until Phases 1-4 are merged and stable.** Flag this to the human before beginning. This is the only phase with an externally breaking change.
|
||||
|
||||
### Backend
|
||||
|
||||
[server/src/ledgrab/api/schemas/devices.py](../../server/src/ledgrab/api/schemas/devices.py) — replace flat `DeviceCreate`/`DeviceUpdate` with Pydantic v2 tagged unions:
|
||||
|
||||
```python
|
||||
class WLEDDeviceCreate(BaseModel):
|
||||
device_type: Literal["wled"]
|
||||
name: str
|
||||
url: str
|
||||
led_count: int
|
||||
use_ddp: bool = False
|
||||
# ... base fields only
|
||||
|
||||
DeviceCreate = Annotated[
|
||||
Union[WLEDDeviceCreate, AdalightDeviceCreate, ...],
|
||||
Field(discriminator="device_type"),
|
||||
]
|
||||
```
|
||||
|
||||
Add `model_config = ConfigDict(extra="ignore")` on each union member for **one release cycle** so existing clients (frontend, HAOS integration, curl scripts) that send extra fields don't 422 immediately. Add a deprecation note and tighten to `extra="forbid"` in a follow-up.
|
||||
|
||||
### Frontend
|
||||
|
||||
- [server/src/ledgrab/static/js/features/devices.ts](../../server/src/ledgrab/static/js/features/devices.ts) and related — when building the POST/PATCH body, scope the payload to the selected `device_type` using the show/hide knowledge already in `device-discovery.ts`.
|
||||
- **No plain `<select>` elements** — any new pickers use IconSelect or EntitySelect (see root CLAUDE.md UI rules).
|
||||
|
||||
### Tests
|
||||
|
||||
- Update `test_devices_routes.py` to assert discriminated union rejection of mismatched shapes.
|
||||
- Add round-trip tests: create device of each type via API → fetch → compare fields.
|
||||
|
||||
### Acceptance
|
||||
|
||||
- `cd server && py -3.13 -m pytest tests/ --no-cov -q` — green.
|
||||
- `cd server && npx tsc --noEmit && npm run build` — green.
|
||||
- Manual smoke for at least 3 device types (WLED, DMX, Hue) — create, edit, delete via UI.
|
||||
- HAOS integration still works against the server (spot-check; not automated).
|
||||
|
||||
---
|
||||
|
||||
## Conventions the implementing agent must follow
|
||||
|
||||
- **Project task tracker is `TODO.md`** — check the "Refactor: Per-Provider Device Configs" section, tick boxes as phases land. Do **not** use the `TodoWrite` tool.
|
||||
- **Auto-restart after Python changes.** See [contexts/server-operations.md](../../contexts/server-operations.md).
|
||||
- **No commits without explicit user approval.** Present each phase's diff for review first.
|
||||
- **Pre-commit gate every phase:**
|
||||
- `cd server && ruff check src/ tests/ --fix`
|
||||
- `cd server && py -3.13 -m pytest tests/ --no-cov -q`
|
||||
- Phase 5 additionally: `cd server && npx tsc --noEmit && npm run build`
|
||||
- **No plain `<select>`** — Phase 5 uses IconSelect / EntitySelect.
|
||||
- **Android parity:** if you add any new runtime dep to `server/pyproject.toml`, update `android/app/build.gradle.kts` per the root [CLAUDE.md](../../CLAUDE.md) "Android Dependency Sync" section. This refactor should not need any new deps.
|
||||
- **Data migration policy:** storage schema is unchanged, so no JSON-file migration is needed. But if you rename any serialized field during `to_dict`/`from_dict`, add migration logic per the root [CLAUDE.md](../../CLAUDE.md) "Data Migration Policy" section.
|
||||
- **Use `ast-index`** for code search (`ast-index search`, `ast-index usages`, `ast-index callers`, `ast-index class`). Fall back to Grep only for regex/string-literal/comment searches.
|
||||
- **Never run `cd` in Bash.** Use absolute paths or the project-relative `cd server && <cmd>` idiom (one-shot, same invocation).
|
||||
|
||||
## Known risks
|
||||
|
||||
1. **Frozen dataclass + inheritance + defaults** — Python's `@dataclass(frozen=True)` with inheritance requires every subclass field to have a default if any parent field does. Base has defaulted fields. Verify in Phase 1. If it breaks, use `kw_only=True` (Python 3.10+).
|
||||
2. **`use_ddp` origin** — currently inferred from `self._protocol == "ddp"` at the call site, not from Device storage. Options: add a column (schema change, more work), **or** keep inference logic inside `Device.to_config()` (recommended — no schema change). Prefer the latter.
|
||||
3. **Test-mode minimal client** ([device_test_mode.py](../../server/src/ledgrab/core/processing/device_test_mode.py) lines 72-78) may not have all `BaseDeviceConfig` fields available. Build a synthetic config via a named helper; do not leak the hack into `Device.to_config()`.
|
||||
4. **Group `device_store` import cycle** — `GroupConfig` must **not** hold `device_store` (would pull storage into the config module). `ProviderDeps` is the deliberate cut.
|
||||
5. **BLE optional import** — `BLEDeviceProvider` is conditionally registered (see [led_client.py](../../server/src/ledgrab/core/devices/led_client.py) lines 321-330). Ensure `BLEConfig` still imports cleanly even when `bleak` is absent — put `BLEConfig` in `device_config.py` (not in `ble_provider.py`) so it's always importable.
|
||||
|
||||
## Deliverables per phase
|
||||
|
||||
1. Branch: `refactor/device-typed-configs`.
|
||||
2. One commit per phase, conventional-commit messages:
|
||||
- `refactor(devices): phase 1 — add DeviceConfig hierarchy`
|
||||
- `refactor(devices): phases 2+3 — typed provider signatures + call-site migration`
|
||||
- `refactor(devices): phase 4 — test migration to typed configs`
|
||||
- `refactor(devices): phase 5 — API discriminated union` (separate PR)
|
||||
3. Phase-by-phase diffs presented for user review **before** each commit.
|
||||
4. Final PR body linking all phases, with manual test plan per device type touched.
|
||||
Reference in New Issue
Block a user