follow-up(telemetry): P3 cleanups from #91 review (throttle and Direction type) #98

New issue

Open

opened 2026-09-25 12:14:45 +00:00 by gabogg · 0 comments

gabogg commented

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

Owner

Follow-ups from the pass-2 review of #91 (comment on #91). All P3; the one P2 (stale PR description) was fixed before merge.

Standards

Redundant lastRunAt write. fire() in app/static/js/src/utils.js sets lastRunAt, then refreshOccupancyData() calls markRan() again. Drop one or document that re-marking is expected.
Validator idiom. PassengerFlowEvent uses @field_validator(..., mode="before") for upper-casing while the module otherwise uses Annotated[..., BeforeValidator(_upper_direction)].
Test smell. tests/test_direction_type.py parametrises over two models with different contracts and then branches on model is ...; split into two tests with honest names.
Nit. occupancy_controller.py calls payload.direction.upper() twice; bind a local.

Spec

markRan() wiring untested (from pass 1). The throttle tests call schedule.markRan() by hand; nothing fails if refreshOccupancyData() stops calling it.
JSDoc overstates retries. utils.js says "the next push retries when it ends"; nothing retries on its own. Suggested: "a push arriving during the window is held until it ends; nothing retries on its own."
Write-path return type. record_counting_event_async accepts Direction but returns PassengerFlowEvent with direction: str.
Enforce direction on write. The repository still accepts any direction string on write (occupancy_repository.py); a DB CHECK or validated write model would stop non-IN/OUT rows at the source.

🤖 Generated with Claude Code

Follow-ups from the pass-2 review of #91 (comment on #91). All P3; the one P2 (stale PR description) was fixed before merge. ## Standards - [ ] **Redundant `lastRunAt` write.** `fire()` in `app/static/js/src/utils.js` sets `lastRunAt`, then `refreshOccupancyData()` calls `markRan()` again. Drop one or document that re-marking is expected. - [ ] **Validator idiom.** `PassengerFlowEvent` uses `@field_validator(..., mode="before")` for upper-casing while the module otherwise uses `Annotated[..., BeforeValidator(_upper_direction)]`. - [ ] **Test smell.** `tests/test_direction_type.py` parametrises over two models with different contracts and then branches on `model is ...`; split into two tests with honest names. - [ ] **Nit.** `occupancy_controller.py` calls `payload.direction.upper()` twice; bind a local. ## Spec - [ ] **`markRan()` wiring untested (from pass 1).** The throttle tests call `schedule.markRan()` by hand; nothing fails if `refreshOccupancyData()` stops calling it. - [ ] **JSDoc overstates retries.** `utils.js` says "the next push retries when it ends"; nothing retries on its own. Suggested: "a push arriving during the window is held until it ends; nothing retries on its own." - [ ] **Write-path return type.** `record_counting_event_async` accepts `Direction` but returns `PassengerFlowEvent` with `direction: str`. - [ ] **Enforce direction on write.** The repository still accepts any `direction` string on write (`occupancy_repository.py`); a DB `CHECK` or validated write model would stop non-IN/OUT rows at the source. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added the

ready-for-agent

label

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

gabogg referenced this issue from a commit

2026-09-25 12:17:39 +00:00

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