follow-up(telemetry): P3 cleanups from #94 review (door transition broadcast) #102

New issue

Open

opened 2026-09-25 12:14:48 +00:00 by gabogg · 1 comment

gabogg commented

2026-09-25 12:14:48 +00:00

Owner

Follow-ups from the pass-2 review of #94 (comment on #94). All P3; no P1/P2 remained.

Standards

Unreachable sync fallback. _transition_events (door_service.py) falls back to door_repo.get_by_code_sync() for doors missing from self.doors. No current path reaches it (every path starts from a known door), it's untested, and on the async poll or reconciliation path it would be blocking SQLite I/O on the event loop. Delete it, or make it async and test it. Both review axes flagged this.
Raw transition dicts (§2.3). The same 11-key dict travels through four paths and is re-read key by key with str()/int() coercions. Use a typed input record.
Dual-key fallback. exclusionSource / exclusion_source; pick one canonical key.
Engine state logic duplicated. telemetry_engine.js _ingestDoorTransitions has a third inline state→flags mapping and now a state→label mapping.
Labels differ from the server's. The engine shows ⚫ Sin conexión for offline, and 🟡 Abierta / 🟢 Asegurada for REMAIN_OPEN/REMAIN_CLOSED; the server's DoorState.label gives ⚪ Fuera de Línea, Permanecer Abierta, 🔒 Permanecer Cerrada.
Misleading log. "Door broadcast deferred: no running event loop" implies a retry; it's dropped.

Spec

Duplicate emission after reconciliation. No test shows that a reconciliation right after a poll that already recorded the change emits nothing more (spec: "duplicate emission prevention").
Stale PR verification counts (description says 9 backend + 9 engine tests; now 11 + 10). Refresh them in the merged PR record if useful.

🤖 Generated with Claude Code

Follow-ups from the pass-2 review of #94 (comment on #94). All P3; no P1/P2 remained. ## Standards - [ ] **Unreachable sync fallback.** `_transition_events` (`door_service.py`) falls back to `door_repo.get_by_code_sync()` for doors missing from `self.doors`. No current path reaches it (every path starts from a known door), it's untested, and on the async poll or reconciliation path it would be blocking SQLite I/O on the event loop. Delete it, or make it async and test it. Both review axes flagged this. - [ ] **Raw transition dicts (§2.3).** The same 11-key dict travels through four paths and is re-read key by key with `str()`/`int()` coercions. Use a typed input record. - [ ] **Dual-key fallback.** `exclusionSource` / `exclusion_source`; pick one canonical key. - [ ] **Engine state logic duplicated.** `telemetry_engine.js` `_ingestDoorTransitions` has a third inline state→flags mapping and now a state→label mapping. - [ ] **Labels differ from the server's.** The engine shows `⚫ Sin conexión` for offline, and `🟡 Abierta` / `🟢 Asegurada` for REMAIN_OPEN/REMAIN_CLOSED; the server's `DoorState.label` gives `⚪ Fuera de Línea`, `Permanecer Abierta`, `🔒 Permanecer Cerrada`. - [ ] **Misleading log.** "Door broadcast deferred: no running event loop" implies a retry; it's dropped. ## Spec - [ ] **Duplicate emission after reconciliation.** No test shows that a reconciliation right after a poll that already recorded the change emits nothing more (spec: "duplicate emission prevention"). - [ ] **Stale PR verification counts** (description says 9 backend + 9 engine tests; now 11 + 10). Refresh them in the merged PR record if useful. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added the

ready-for-agent

label

2026-09-25 12:14:48 +00:00

gabogg referenced this issue from a commit

2026-09-25 12:19:45 +00:00

feat(telemetry): broadcast door state transitions as their own message (#55) (#94)

~~gabogg referenced this issue 2026-09-25 18:14:27 +00:00~~

fix(doors): async webhook persistence and transition cleanup (#95, #102) #112

gabogg referenced this issue

2026-09-25 18:20:37 +00:00

fix(doors): async webhook persistence and transition cleanup (#95, #102) #112

gabogg referenced this issue from a commit

2026-09-25 18:33:44 +00:00

fix(doors): preserve webhook broadcasts after cancellation

gabogg referenced this issue

2026-09-25 18:43:42 +00:00

fix(doors): async webhook persistence and transition cleanup (#95, #102) #112

gabogg commented

2026-09-25 18:43:44 +00:00

Author

Owner

PR #112 second-pass review follow-ups (minor; keep this issue open after merge):

Overview state normalization remains inconsistent. In app/static/js/src/telemetry/telemetry_engine.js, _ingestDoors() consults the new doorStatePresentation() but its final assignment still forces every non-open overview state to CLOSED. Reproduction: a doors_update with doorState: 4 and is_offline: true yields a snapshot with doorState: 1, stateKey: "closed", is_closed: true, and is_offline: true. REMAIN_CLOSED is also collapsed. Have overview ingestion and snapshot flags use the shared presentation map, then test OFFLINE and commanded states across transition followed by overview.
New test functions lack type annotations. For example, tests/test_door_transition_broadcast.py:153,160,189,296 adds unannotated signatures, contrary to docs/standards/code-standards.md §2.2. Add explicit parameter and return annotations to the touched tests/helpers.

The prior review's duplicated polling transition construction and cancellation finding are fixed in PR #112.

PR #112 second-pass review follow-ups (minor; keep this issue open after merge): - **Overview state normalization remains inconsistent.** In `app/static/js/src/telemetry/telemetry_engine.js`, `_ingestDoors()` consults the new `doorStatePresentation()` but its final assignment still forces every non-open overview state to `CLOSED`. Reproduction: a `doors_update` with `doorState: 4` and `is_offline: true` yields a snapshot with `doorState: 1`, `stateKey: "closed"`, `is_closed: true`, and `is_offline: true`. `REMAIN_CLOSED` is also collapsed. Have overview ingestion and snapshot flags use the shared presentation map, then test OFFLINE and commanded states across transition followed by overview. - **New test functions lack type annotations.** For example, `tests/test_door_transition_broadcast.py:153,160,189,296` adds unannotated signatures, contrary to `docs/standards/code-standards.md` §2.2. Add explicit parameter and return annotations to the touched tests/helpers. The prior review's duplicated polling transition construction and cancellation finding are fixed in PR #112.

gabogg referenced this issue

2026-09-25 18:44:15 +00:00

fix(doors): async webhook persistence and transition cleanup (#95, #102) #112

gabogg referenced this issue from a commit

2026-09-25 18:45:11 +00:00

Merge pull request 'fix(doors): async webhook persistence and transition cleanup (#95, #102)' (#112) from fix/door-transition-async-cleanup into master