fix(telemetry): PR #52 review follow-ups — throttle ignores direct fetches, direction typing, engine contract docs #67

New issue

Closed

opened 2026-09-23 22:15:44 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-23 22:15:44 +00:00

Owner

Non-blocking follow-ups from the round-2 review of PR #52 (merged). Line numbers refer to PR #52's head 2b78fdb; re-check them after merge.

Behaviour (Spec)

The throttle window ignores direct fetches, and its first push fires immediately. Reproduced with a fake clock.
- createTrailingThrottle (app/static/js/src/utils.js) runs on the next tick when idle for a full window: a leading edge, while the spec says "trailing".
- Direct refreshOccupancyData() calls (login and switchTab('doors'), timespan change, about 14 admin actions in app/static/js/app.js) don't reset the throttle's lastRunAt. So a push right after one of them causes a second fetch: simulated fetches at t=0 and t=0, or 0 s and 1 s in a burst.
- Fix: have the throttle record every run, e.g. expose a markRan() that refreshOccupancyData calls, or route the direct calls through it. Then decide explicitly whether the first push after idle should wait.

Code quality (Standards)

Shadowed name in the simulator. app/controllers/occupancy_controller.py:~392: for direction, total, cams, spacing in (...) reuses cams, which already holds the list of all cameras and is the fallback in in_cams = [...] or cams. Rename the loop variable (e.g. targets).
ingestMessage / onRawMessage contract is undocumented where callers look. app/static/js/src/telemetry/telemetry_engine.js: the rule that ingestMessage() does not fire onRawMessage is only an inline comment. Move it into ingestMessage's JSDoc, and document onRawMessage (network messages only) and _handleMessage's { fromNetwork } option.
Allowed directions defined twice. The /event controller checks ("IN", "OUT") by hand, and CountingEventInput.direction is Literal["IN", "OUT"]; CountingEventWebhookPayload.direction is still str. Define one Direction type alias and use it in all three. Coordinate with #64, which owns that endpoint.
direction is still a plain string past the input boundary. PassengerFlowEvent.direction and record_counting_event_async(direction: str) are untyped. Keep in mind that stored rows may predate validation (old PASS rows) when narrowing the read model.
Mixed access styles for GroupMember. sync_passenger_flow_from_artemis_async reads member.code in the new loop but still tuple-unpacks for c_code, _ in in_cams in the totals just above. Use attribute access throughout.

Acceptance criteria

Item 1 fixed, with a fake-clock test covering "direct fetch then push" and "first push after idle".
Items 2–6 fixed, or explicitly declined with a reason.
Full suites green (pytest + node --test).

Related: #14, #64, PR #52.

> Non-blocking follow-ups from the round-2 review of PR #52 (merged). Line numbers refer to PR #52's head `2b78fdb`; re-check them after merge. ## Behaviour (Spec) 1. **The throttle window ignores direct fetches, and its first push fires immediately.** Reproduced with a fake clock. - `createTrailingThrottle` (`app/static/js/src/utils.js`) runs on the next tick when idle for a full window: a leading edge, while the spec says "trailing". - Direct `refreshOccupancyData()` calls (login and `switchTab('doors')`, timespan change, about 14 admin actions in `app/static/js/app.js`) don't reset the throttle's `lastRunAt`. So a push right after one of them causes a second fetch: simulated fetches at t=0 and t=0, or 0 s and 1 s in a burst. - Fix: have the throttle record every run, e.g. expose a `markRan()` that `refreshOccupancyData` calls, or route the direct calls through it. Then decide explicitly whether the first push after idle should wait. ## Code quality (Standards) 2. **Shadowed name in the simulator.** `app/controllers/occupancy_controller.py:~392`: `for direction, total, cams, spacing in (...)` reuses `cams`, which already holds the list of all cameras and is the fallback in `in_cams = [...] or cams`. Rename the loop variable (e.g. `targets`). 3. **`ingestMessage` / `onRawMessage` contract is undocumented where callers look.** `app/static/js/src/telemetry/telemetry_engine.js`: the rule that `ingestMessage()` does not fire `onRawMessage` is only an inline comment. Move it into `ingestMessage`'s JSDoc, and document `onRawMessage` (network messages only) and `_handleMessage`'s `{ fromNetwork }` option. 4. **Allowed directions defined twice.** The `/event` controller checks `("IN", "OUT")` by hand, and `CountingEventInput.direction` is `Literal["IN", "OUT"]`; `CountingEventWebhookPayload.direction` is still `str`. Define one `Direction` type alias and use it in all three. Coordinate with #64, which owns that endpoint. 5. **`direction` is still a plain string past the input boundary.** `PassengerFlowEvent.direction` and `record_counting_event_async(direction: str)` are untyped. Keep in mind that stored rows may predate validation (old `PASS` rows) when narrowing the read model. 6. **Mixed access styles for `GroupMember`.** `sync_passenger_flow_from_artemis_async` reads `member.code` in the new loop but still tuple-unpacks `for c_code, _ in in_cams` in the totals just above. Use attribute access throughout. ## Acceptance criteria - [ ] Item 1 fixed, with a fake-clock test covering "direct fetch then push" and "first push after idle". - [ ] Items 2–6 fixed, or explicitly declined with a reason. - [ ] Full suites green (pytest + `node --test`). Related: #14, #64, PR #52.

gabogg referenced this issue

2026-09-23 22:16:11 +00:00

refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14) #52

gabogg referenced this issue from a commit

2026-09-23 22:19:44 +00:00

refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#52)

gabogg changed title from ~~Follow-ups from PR #52 review: throttle ignores direct fetches, direction typing, engine contract docs~~ to fix(telemetry): PR #52 review follow-ups — throttle ignores direct fetches, direction typing, engine contract docs

2026-09-24 10:15:39 +00:00

gabogg added the

ready-for-agent

label

2026-09-24 10:15:40 +00:00

gabogg referenced this issue

2026-09-24 23:05:38 +00:00

docs: audit accuracy, measure coverage, and reorganize #79

gabogg referenced this issue from a commit

2026-09-25 01:11:25 +00:00

fix(telemetry): direct fetches reset the refresh throttle; one Direction type

gabogg referenced this issue from a pull request that will close it,