Production-readiness pass: security hardening, performance improvements, new services (send_message, set_repeat, refresh_library), diagnostics, reauth flow, image proxy, per-instance device IDs, exponential WS reconnect backoff, ID validation, stale device cleanup, and supporting integration plumbing. Three rounds of independent code review applied. See RELEASE_NOTES.md for the full changelog.
This commit is contained in:
@@ -1,111 +1,154 @@
|
||||
# Claude Code Session Notes
|
||||
# Claude Code – Project Notes
|
||||
|
||||
This file documents mistakes and lessons learned during the development of this integration.
|
||||
Operator notes for working on this repo with Claude Code (or any other LLM
|
||||
coding agent). Keep this file accurate; future agents read it as ground truth.
|
||||
|
||||
## Mistakes Made
|
||||
## Project at a glance
|
||||
|
||||
### 1. Missing `/emby` Prefix on API Endpoints
|
||||
- **What this is.** A Home Assistant custom integration that exposes Emby
|
||||
Server clients as `media_player` entities, with WebSocket real-time updates
|
||||
and a REST polling safety net.
|
||||
- **Layout.** All code lives under
|
||||
`custom_components/emby_player/`. HACS metadata at the repo root
|
||||
(`hacs.json`, `RELEASE_NOTES.md`, `README.md`).
|
||||
- **Distribution.** HACS custom repo, hosted on a private Gitea instance
|
||||
(`git.dolgolyov-family.by`). No GitHub mirror — `manifest.json` keeps
|
||||
`codeowners: []` because hassfest validates handles against GitHub.
|
||||
- **Min HA version.** `2024.10.0` (declared in `hacs.json`); the Python is
|
||||
expected to be 3.12+ as shipped by HAOS.
|
||||
- **Quality scale.** Targets `silver`.
|
||||
|
||||
**Problem:** Initially created all API endpoints without the `/emby` prefix.
|
||||
## Module map
|
||||
|
||||
```python
|
||||
# Wrong
|
||||
ENDPOINT_SYSTEM_INFO = "/System/Info"
|
||||
ENDPOINT_USERS = "/Users"
|
||||
| File | Role |
|
||||
| ---- | ---- |
|
||||
| `__init__.py` | Entry setup/unload, runtime data dataclass, hub device, manifest-version lookup, `async_remove_config_entry_device` |
|
||||
| `api.py` | REST client. Owns no aiohttp session. Validates IDs and image types before URL build. Pins working `/emby` vs `""` prefix on connect. |
|
||||
| `websocket.py` | WS client. API key in headers (never URL). Exponential backoff + jitter. Detaches async callbacks via task. |
|
||||
| `coordinator.py` | `DataUpdateCoordinator` subclass. Frozen dataclasses. WS events update sessions in place. REST poll merges via `last_seen`. `_safe_int` for malformed payloads. |
|
||||
| `media_player.py` | The entity class. Image proxy via `async_get_media_image`. Stale device prune. Enqueue → Emby command map. |
|
||||
| `config_flow.py` | User + reauth flows. Uses `getattr` fallbacks for HA-version-shifting helpers. |
|
||||
| `browse_media.py` | Media browser; wraps all errors as `BrowseError`. |
|
||||
| `services.py` / `services.yaml` | `send_message`, `set_repeat`, `refresh_library`. |
|
||||
| `diagnostics.py` | Redacted entry + sessions dump; session IDs hashed. |
|
||||
| `const.py` | All constants, including `EMBY_ID_PATTERN`, `ALLOWED_IMAGE_TYPES`, timeouts, backoff bounds. |
|
||||
| `manifest.json` | HA integration metadata + ssdp / zeroconf hints. |
|
||||
| `strings.json` / `translations/en.json` | UI strings (config flow, services, reauth). |
|
||||
|
||||
# Correct
|
||||
ENDPOINT_SYSTEM_INFO = "/emby/System/Info"
|
||||
ENDPOINT_USERS = "/emby/Users"
|
||||
```
|
||||
## Workflow expectations
|
||||
|
||||
**Impact:** Connection to Emby server failed with "cannot connect" errors.
|
||||
1. **Plan first.** For anything bigger than a typo, use `planner` /
|
||||
`architect` agents (or sketch a TodoWrite plan) before editing files.
|
||||
2. **Edit minimally.** Match the project's style — explicit type hints,
|
||||
`from __future__ import annotations`, frozen dataclasses for state.
|
||||
3. **Verify locally.**
|
||||
- Parse check: `python -c "import ast, pathlib; [ast.parse(p.read_text(encoding='utf-8'), filename=str(p)) for p in pathlib.Path('custom_components/emby_player').glob('*.py')]"`
|
||||
- JSON validation for `manifest.json`, `strings.json`, `translations/*.json`, `hacs.json`.
|
||||
4. **Review before commit.** Spawn `python-reviewer` (or `code-reviewer`)
|
||||
after any non-trivial change. Two-round reviews are the norm here — the
|
||||
first pass usually surfaces hidden issues.
|
||||
5. **Never commit without explicit user approval.** This rule has been
|
||||
broken before — don't.
|
||||
|
||||
**Lesson:** Always verify API endpoint formats against official documentation. Emby Server requires the `/emby` prefix for all API calls.
|
||||
## Conventions / invariants
|
||||
|
||||
---
|
||||
- **Aiohttp session is owned by HA.** API and WebSocket clients accept an
|
||||
injected `aiohttp.ClientSession` and never close it. Do not add
|
||||
`_ensure_session` or `_owns_session` flags back.
|
||||
- **IDs are validated.** Anything that lands in a URL path (`session_id`,
|
||||
`item_id`, `user_id`, `parent_id`) must go through `_validate_emby_id`
|
||||
(regex `^[A-Za-z0-9_-]{1,128}$`). Don't bypass.
|
||||
- **Image type whitelist.** `image_type` must be in `ALLOWED_IMAGE_TYPES`
|
||||
(see `const.py`). Adding a new image type means updating the constant.
|
||||
- **API key never goes into URLs.** Image fetches use the `X-Emby-Token`
|
||||
header via `EmbyApiClient.fetch_image`. The HA media-player image proxy
|
||||
(`async_get_media_image` / `async_get_browse_image`) is the public path.
|
||||
- **WebSocket auth via headers.** Same reason: no query-string leakage to
|
||||
proxy logs.
|
||||
- **Device identity is per-config-entry.** `device_id` is derived from
|
||||
`instance_id.async_get(hass)` + `entry.entry_id`. Multiple HA installs on
|
||||
one Emby server must not collide.
|
||||
- **Frozen dataclasses for coordinator state.** Use
|
||||
`dataclasses.replace(...)` to produce a new session; never mutate.
|
||||
- **Auth failure routing.** Authentication errors during `_async_update_data`
|
||||
must raise `ConfigEntryAuthFailed` so HA triggers the reauth flow. Do not
|
||||
re-introduce a custom subclass of `UpdateFailed` for auth.
|
||||
- **No raw `print` / no emoji** in code or comments. Use `_LOGGER`.
|
||||
- **Comments explain WHY only.** Identifier names should carry the WHAT.
|
||||
|
||||
### 2. Incorrect Volume Control API Format
|
||||
## Emby API gotchas (historical, still worth remembering)
|
||||
|
||||
**Problem:** Tried multiple incorrect formats for the SetVolume command before finding the correct one.
|
||||
- **`/emby` prefix.** Most Emby Server deployments require `/emby/` in front
|
||||
of API paths. The client probes both `/emby/System/Info` and
|
||||
`/System/Info` on `test_connection()` and pins the working prefix in
|
||||
`self._prefix`. Don't hardcode the prefix into endpoint constants.
|
||||
- **General commands.** Emby's `/Sessions/{id}/Command` endpoint expects
|
||||
`{"Name": "<command>", "Arguments": {...}}`, NOT
|
||||
`/Command/{commandName}?...`. Arguments must be string-typed values, so
|
||||
`_send_command` stringifies everything.
|
||||
- **WebSocket `ForceKeepAlive`.** Emby will close the WS connection if you
|
||||
don't echo back `KeepAlive` to `ForceKeepAlive`. Already handled in
|
||||
`_handle_message`; don't strip that path.
|
||||
- **Non-admin API keys.** `GET /Users` returns 401/403 for non-admin tokens.
|
||||
`EmbyApiClient.get_users` falls back to `/Users/Public`.
|
||||
- **Ticks.** Emby uses 100ns ticks (`TICKS_PER_SECOND = 10_000_000`). Use
|
||||
`round()` not `int()` when converting seconds → ticks to avoid off-by-one
|
||||
on resume positions.
|
||||
- **`NumberSelector` returns float.** Always `int(...)` before passing to
|
||||
the API layer (port, scan_interval, etc.).
|
||||
- **`PlaySessionId` vs `SessionId`.** WS playback events carry both;
|
||||
`SessionId` is the per-client session, `PlaySessionId` is per-stream and
|
||||
can collide across devices. The coordinator only keys off `SessionId`.
|
||||
|
||||
```python
|
||||
# Attempt 1 - Wrong endpoint with body
|
||||
endpoint = f"/Sessions/{session_id}/Command/SetVolume"
|
||||
data = {"Arguments": {"Volume": 50}}
|
||||
## Performance budget
|
||||
|
||||
# Attempt 2 - Wrong: query parameter
|
||||
endpoint = f"/Sessions/{session_id}/Command/SetVolume?Volume=50"
|
||||
- **WS connected.** Trust WS for live updates. REST poll runs every 5 min
|
||||
(`DEFAULT_SCAN_INTERVAL_WS = 300`) as a safety net; the merge logic in
|
||||
`_async_update_data` keeps WS-current sessions intact.
|
||||
- **WS down.** Polling falls back to the user-configured `scan_interval`
|
||||
(default 10 s; range 5–60 s).
|
||||
- **Image fetches.** Run on a separate 30 s timeout
|
||||
(`IMAGE_FETCH_TIMEOUT_SECONDS`), independent of the 15 s REST timeout.
|
||||
|
||||
# Correct format
|
||||
endpoint = f"/Sessions/{session_id}/Command"
|
||||
data = {"Name": "SetVolume", "Arguments": {"Volume": "50"}} # Arguments as STRINGS
|
||||
```
|
||||
## Security checklist (anything that touches the API key or user URLs)
|
||||
|
||||
**Impact:** Volume control didn't work even though mute/unmute worked fine.
|
||||
- [ ] API key only travels in headers (`X-Emby-Token`) or HA's own redacted
|
||||
config entry storage — never URLs.
|
||||
- [ ] All IDs flowing into REST paths validated.
|
||||
- [ ] `image_type` validated against `ALLOWED_IMAGE_TYPES`.
|
||||
- [ ] Diagnostics redact API key and hash session/device/user IDs.
|
||||
- [ ] `verify_ssl` honoured for HTTPS; defaults to verifying.
|
||||
|
||||
**Lesson:** Emby general commands must be sent to `/Command` endpoint (not `/Command/{CommandName}`) with:
|
||||
- `Name` field containing the command name
|
||||
- `Arguments` as a dict with STRING values, not integers
|
||||
## Files NOT to edit casually
|
||||
|
||||
---
|
||||
- `hacs.json` — bumping the `homeassistant` floor is a breaking change for
|
||||
installed users; coordinate with a release.
|
||||
- `manifest.json` `version` — must be bumped together with
|
||||
`RELEASE_NOTES.md` and a Git tag.
|
||||
- `RELEASE_NOTES.md` v0.x.0 sections — append new sections; don't rewrite
|
||||
history.
|
||||
|
||||
### 3. NumberSelector Returns Float, Not Int
|
||||
## Release flow
|
||||
|
||||
**Problem:** Home Assistant's `NumberSelector` widget returns float values, but port numbers must be integers.
|
||||
1. Land all changes on a feature branch.
|
||||
2. Bump `manifest.json` version + add a section to `RELEASE_NOTES.md` (newest
|
||||
first).
|
||||
3. Open a PR on Gitea; wait for review.
|
||||
4. After merge, tag `vX.Y.Z` on the default branch — the Gitea release
|
||||
workflow under `.gitea/` cuts the release.
|
||||
|
||||
```python
|
||||
# Wrong - port could be 8096.0
|
||||
self._port = user_input.get(CONF_PORT, DEFAULT_PORT)
|
||||
## Anti-patterns we've already paid for
|
||||
|
||||
# Correct
|
||||
self._port = int(user_input.get(CONF_PORT, DEFAULT_PORT))
|
||||
```
|
||||
|
||||
**Impact:** Potential type errors or malformed URLs with decimal port numbers.
|
||||
|
||||
**Lesson:** Always explicitly convert NumberSelector values to the expected type.
|
||||
|
||||
---
|
||||
|
||||
### 4. Inconsistent Use of Constants
|
||||
|
||||
**Problem:** Hardcoded endpoint paths in some methods instead of using defined constants.
|
||||
|
||||
```python
|
||||
# Wrong - hardcoded
|
||||
endpoint = f"/Sessions/{session_id}/Playing"
|
||||
|
||||
# Correct - using constant
|
||||
endpoint = f"{ENDPOINT_SESSIONS}/{session_id}/Playing"
|
||||
```
|
||||
|
||||
**Impact:** When the `/emby` prefix was added to constants, hardcoded paths were missed, causing inconsistent behavior.
|
||||
|
||||
**Lesson:** Always use constants consistently. When fixing issues, search for all occurrences of the affected strings.
|
||||
|
||||
---
|
||||
|
||||
### 5. Import Confusion Between Local and HA Constants
|
||||
|
||||
**Problem:** Initially imported `CONF_HOST` and `CONF_PORT` from `homeassistant.const` in some files, while also defining them in local `const.py`.
|
||||
|
||||
```python
|
||||
# Inconsistent imports
|
||||
from homeassistant.const import CONF_HOST, CONF_PORT # in __init__.py
|
||||
from .const import CONF_HOST, CONF_PORT # in config_flow.py
|
||||
```
|
||||
|
||||
**Impact:** Potential confusion and maintenance issues, though values were identical.
|
||||
|
||||
**Lesson:** Choose one source for constants and use it consistently across all files. For custom integrations, prefer local constants for full control.
|
||||
|
||||
---
|
||||
|
||||
## Best Practices Established
|
||||
|
||||
1. **Test API endpoints with curl first** - Verify the exact request format before implementing in code
|
||||
2. **Add debug logging** - Include request URLs and response status codes for troubleshooting
|
||||
3. **Handle multiple API formats** - Some servers may respond differently; implement fallbacks when sensible
|
||||
4. **Type conversion** - Always explicitly convert UI input values to expected types
|
||||
5. **Consistent constant usage** - Define constants once and use them everywhere
|
||||
6. **Wait for user approval before committing** - Always let the user test changes before creating git commits
|
||||
- Bypassing constants and hardcoding endpoint paths — see history of the
|
||||
missing `/emby` prefix.
|
||||
- Reading API responses without `isinstance` checks — Emby sometimes
|
||||
returns `null` or unexpected shapes for fields like `RunTimeTicks`.
|
||||
Use the `_safe_int` helper in `coordinator.py` for any numeric field
|
||||
coming off the wire.
|
||||
- Reusing a fixed `DEVICE_ID` across HA instances — Emby will evict
|
||||
whichever instance last registered, silently breaking the other.
|
||||
- Including the API key in URLs returned to the browser via
|
||||
`media_image_url`. The fix is the image proxy in `media_player.py`; don't
|
||||
reintroduce the URL-with-key shortcut "for performance".
|
||||
- `except Exception: pass` — always at least `_LOGGER.debug(...)` so
|
||||
failures are diagnosable.
|
||||
|
||||
Reference in New Issue
Block a user