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

Merged

gabogg merged 2 commits from fix/telemetry-throttle-direction-typing into master 2026-09-25 12:17:38 +00:00

Conversation 2 Commits 2 Files changed 8 +167 -38

gabogg commented 2026-09-25 01:11:43 +00:00

Owner

Copy link

Closes #67

Summary

Direct overview fetches now share the WebSocket refresh throttle, so a push right after a direct fetch no longer fetches the overview twice. IN/OUT has one Direction type on the write path. The stored-event read model deliberately stays a normalised str so legacy rows stay readable.

Problem

Follow-ups from the round-2 review of PR #52:

1. The occupancy overview throttle only counted its own runs. Direct refreshOccupancyData() calls (login, switchTab('doors'), timespan change, about 14 admin actions) didn't start a window, so a WebSocket push right after one of them fetched the overview a second time.
2. The simulator's loop variable cams shadowed the list of all cameras.
3. The rule that ingestMessage() never reaches onRawMessage lived only in an inline comment.
4. / 5. IN/OUT was defined in several places and direction stayed a plain str in the stored-event model and the service.
5. The passenger flow totals mixed tuple unpacking and attribute access for GroupMember.

Approach

Throttle (item 1). createTrailingThrottle now returns a schedule function with a markRan() method. refreshOccupancyData() calls it on every fetch, whether direct or push-triggered. If a push is already waiting when a direct fetch happens, its timer is re-armed for the end of the new window instead of firing early. getOccupancyRefreshThrottle() builds the throttle lazily, so both paths share it.

Decision on "first push after idle": it still runs on the next tick. A trailing-only design would delay the first change after a quiet period by up to 15 s on the operations deck. The throttle exists to coalesce bursts, not to delay isolated updates. This is now written in the JSDoc and covered by a test.

Direction (items 4–5). Direction = Annotated[Literal["IN", "OUT"], BeforeValidator(upper)] and DIRECTIONS: tuple[Direction, ...] = ("IN", "OUT"), both in occupancy_models.py. They are used by CountingEventInput and record_counting_event_async; the controller passes a validated, upper-cased value.

• Declined for the read model: PassengerFlowEvent.direction stays str, upper-cased on read. recent_passages builds it from raw stored rows. Production held only IN/OUT when checked (2026-09-24), but the repository write path doesn't enforce that, so narrowing the read model could turn one bad row into a 500 on the overview. tests/test_direction_type.py asserts that PASS is accepted on the read model and rejected on CountingEventInput.
• Partly declined: CountingEventWebhookPayload.direction stays str, and /event checks against DIRECTIONS. Typing it with the alias would turn the documented 400 INVALID_DIRECTION (translated in i18n.js, asserted in two tests) into a generic 422 VALIDATION_ERROR. #64 is expected to delete /event, so changing its error contract now isn't worth it. A comment on the field explains this.

Items 2, 3, 6. Renamed the loop variable to targets. Moved the contract into JSDoc on ingestMessage, onRawMessage and _handleMessage({ fromNetwork }). GroupMember now uses m.code throughout.

Verification

• node --test tests/frontend/*.test.js: 70 passed. There are 3 new fake-clock tests: a push right after a direct fetch waits for the window; a waiting push moves behind a direct fetch; and the first push after idle runs at once.
• pytest: all passed (282 before the new file). New tests/test_direction_type.py checks that out normalises to OUT on both models, that the write model rejects PASS, and that the read model keeps it.
• ruff check / ruff format --check: clean. Pre-commit passed.

Architectural Impact

createTrailingThrottle now returns a schedule function with a markRan() method (a public return-type change), and the occupancy controller's direct and push-triggered refreshes share one window. The write-side Direction contract validates IN/OUT, while stored passage readback accepts legacy directions. No database schema change.

Checklist

• Direct and push-triggered refreshes share a throttle window.
• Legacy passage rows remain readable while new events validate direction.
• Ruff and full pytest pass.

Review follow-up

The read model deliberately accepts legacy stored directions; write input still validates IN/OUT. The direct-fetch throttle starts before the request, so a failed fetch waits for the next window. Analytics direction filters outside this PR retain their existing string contracts.

🤖 Generated with Claude Code

Closes #67 ## Summary Direct overview fetches now share the WebSocket refresh throttle, so a push right after a direct fetch no longer fetches the overview twice. `IN`/`OUT` has one `Direction` type on the write path. The stored-event read model deliberately stays a normalised `str` so legacy rows stay readable. ## Problem Follow-ups from the round-2 review of PR #52: 1. The occupancy overview throttle only counted its own runs. Direct `refreshOccupancyData()` calls (login, `switchTab('doors')`, timespan change, about 14 admin actions) didn't start a window, so a WebSocket push right after one of them fetched the overview a second time. 2. The simulator's loop variable `cams` shadowed the list of all cameras. 3. The rule that `ingestMessage()` never reaches `onRawMessage` lived only in an inline comment. 4. / 5. `IN`/`OUT` was defined in several places and `direction` stayed a plain `str` in the stored-event model and the service. 6. The passenger flow totals mixed tuple unpacking and attribute access for `GroupMember`. ## Approach **Throttle (item 1).** `createTrailingThrottle` now returns a `schedule` function with a `markRan()` method. `refreshOccupancyData()` calls it on every fetch, whether direct or push-triggered. If a push is already waiting when a direct fetch happens, its timer is re-armed for the end of the new window instead of firing early. `getOccupancyRefreshThrottle()` builds the throttle lazily, so both paths share it. *Decision on "first push after idle":* it still runs on the next tick. A trailing-only design would delay the first change after a quiet period by up to 15 s on the operations deck. The throttle exists to coalesce bursts, not to delay isolated updates. This is now written in the JSDoc and covered by a test. **Direction (items 4–5).** `Direction = Annotated[Literal["IN", "OUT"], BeforeValidator(upper)]` and `DIRECTIONS: tuple[Direction, ...] = ("IN", "OUT")`, both in `occupancy_models.py`. They are used by `CountingEventInput` and `record_counting_event_async`; the controller passes a validated, upper-cased value. - **Declined for the read model:** `PassengerFlowEvent.direction` stays `str`, upper-cased on read. `recent_passages` builds it from raw stored rows. Production held only `IN`/`OUT` when checked (2026-09-24), but the repository write path doesn't enforce that, so narrowing the read model could turn one bad row into a 500 on the overview. `tests/test_direction_type.py` asserts that `PASS` is accepted on the read model and rejected on `CountingEventInput`. - **Partly declined:** `CountingEventWebhookPayload.direction` stays `str`, and `/event` checks against `DIRECTIONS`. Typing it with the alias would turn the documented `400 INVALID_DIRECTION` (translated in `i18n.js`, asserted in two tests) into a generic `422 VALIDATION_ERROR`. #64 is expected to delete `/event`, so changing its error contract now isn't worth it. A comment on the field explains this. **Items 2, 3, 6.** Renamed the loop variable to `targets`. Moved the contract into JSDoc on `ingestMessage`, `onRawMessage` and `_handleMessage({ fromNetwork })`. `GroupMember` now uses `m.code` throughout. ## Verification - `node --test tests/frontend/*.test.js`: **70 passed**. There are 3 new fake-clock tests: a push right after a direct fetch waits for the window; a waiting push moves behind a direct fetch; and the first push after idle runs at once. - `pytest`: **all passed** (282 before the new file). New `tests/test_direction_type.py` checks that `out` normalises to `OUT` on both models, that the write model rejects `PASS`, and that the read model keeps it. - `ruff check` / `ruff format --check`: clean. Pre-commit passed. ## Architectural Impact `createTrailingThrottle` now returns a `schedule` function with a `markRan()` method (a public return-type change), and the occupancy controller's direct and push-triggered refreshes share one window. The write-side Direction contract validates IN/OUT, while stored passage readback accepts legacy directions. No database schema change. ## Checklist - [x] Direct and push-triggered refreshes share a throttle window. - [x] Legacy passage rows remain readable while new events validate direction. - [x] Ruff and full pytest pass. ## Review follow-up The read model deliberately accepts legacy stored directions; write input still validates IN/OUT. The direct-fetch throttle starts before the request, so a failed fetch waits for the next window. Analytics direction filters outside this PR retain their existing string contracts. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 01:11:43 +00:00

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

The occupancy overview throttle only knew about push-triggered runs, so a
push right after a direct refresh (login, tab switch, admin action)
fetched again. The throttle now exposes markRan(), refreshOccupancyData()
calls it on every fetch, and a waiting run moves behind a direct one. The
first push after an idle window still runs at once; that is now stated as
deliberate in the docs and covered by a test.

Also from the PR #52 review follow-ups:
- one Direction type (Literal IN/OUT, case-normalised) used by
  CountingEventInput, PassengerFlowEvent and record_counting_event_async;
  /event checks against DIRECTIONS and keeps its INVALID_DIRECTION error
- rename the simulator loop variable that shadowed the camera list
- attribute access for GroupMember in the passenger flow totals
- document the ingestMessage / onRawMessage / _handleMessage contract

Closes #67

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg referenced this pull request 2026-09-25 01:27:35 +00:00

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

gabogg commented 2026-09-25 08:39:17 +00:00

Author

Owner

Copy link

Code review — pass 1 (two-axis)

Reviewed the PR's own diff (git diff <base>...<head>); Standards and Spec ran as independent passes and are reported separately, not reranked. Severity: P1 must fix · P2 fix before merge · P3 minor. Per review policy, pass 1 findings are all to be fixed on the branch.

Standards

The diff breaks no layering, error-schema, or testing standard. The controller stays thin, /event keeps HTTPException with X-Error-Code: INVALID_DIRECTION (code-standards §3), both changes have new tests (§4.3), and Direction/DirectionType follow the CONTEXT.md glossary. No P1 bugs.

Documented-standard findings

• P3 (hard, process): PR description is missing sections. git-and-workflow.md §2.2 requires Summary, Architectural Impact, Verification and Checklist. The PR has Problem, Approach and Verification (satisfies AGENTS.md §4) but no Checklist and no Architectural Impact section. It changes the public createTrailingThrottle return type and narrows PassengerFlowEvent.direction, so both deserve a line there.

Judgement calls (baseline smells / typing)

• P3: type annotation doesn't match what the controller passes. app/controllers/occupancy_controller.py:359 passes payload.direction (plain str, possibly lower-case) into record_counting_event_async(direction: Direction) at app/services/occupancy_service.py:617. Works at runtime because CountingEventInput upper-cases, but a type checker would reject it. Pass payload.direction.upper() or annotate the parameter as str.
• P3, possible Primitive Obsession (remaining). CountingEventWebhookPayload.direction: str (occupancy_models.py:515) is declined with a reason — fine. direction: str | None still appears in analytics_controller.py:110,207, analytics_service.py:522,623 and occupancy_repository.py:1670. Outside the diff; a follow-up issue would do if "one Direction type" is meant literally.
• P3, nit: DIRECTIONS: tuple[str, ...] (occupancy_models.py:213) could be tuple[Direction, ...].
• P3, redundant write. On the throttle's own path, fire() sets lastRunAt (app/static/js/src/utils.js:110), then calls refreshOccupancyData, which calls markRan() again (app/static/js/app.js:1642). Harmless; drop the assignment in fire() or note in the JSDoc that re-marking is expected.
• P3, undocumented behaviour. markRan() runs before the fetch (app.js:1642). A failed or 401 fetch still starts a 15 s window, so the next push waits instead of retrying. Probably acceptable, but document it.
• Possible Middle Man (not worth acting on). requestOccupancyRefresh (app.js:568) is now a 2-line wrapper; it keeps call sites readable.

Notes

• The fire() re-arm loop is correct: pending stays true across the re-arm, so a waiting push is never duplicated; covered by the new fake-clock tests.

Spec

Verdict: All six items are addressed. No P1 or P2 issues. Three P3 notes.

(a) Missing or partial

• P3, tests/frontend/test_trailing_throttle.test.js:80-121. The spec asks for "a fake-clock test covering 'direct fetch then push' and 'first push after idle'". Both tests exist, but they call schedule.markRan() by hand. Nothing checks that refreshOccupancyData() actually calls it (app/static/js/app.js:1639-1642). If that one line were removed, the original bug would come back and no test would fail.

(b) Scope creep

None. The getOccupancyRefreshThrottle() extraction (app.js:547-571) is the smallest change that lets both paths share one throttle. The fire() re-arm in utils.js:101-111 is the fix the spec asked for.

(c) Implemented but questionable

• P3, app/schemas/occupancy_models.py:438. The spec says "Keep in mind that stored rows may predate validation (old PASS rows) when narrowing the read model." PassengerFlowEvent also backs OccupancyOverviewResponse.recent_passages (occupancy_service.py:1838-1890). A single non-IN/OUT row in the DB would now make the overview return a 500. The PR explains the decision: production holds only IN and OUT rows. But nothing stops such a row being written later; the repository still accepts any evt.get("direction","IN").upper() (occupancy_repository.py:1092,1153). Acceptable; a DB CHECK constraint could be a follow-up.

Open design points

Point	Status
"Then decide explicitly whether the first push after idle should wait"	Decided and sound. Runs on the next tick. Reason in the JSDoc (utils.js:76-77), covered by a test.
"route the direct calls through it" vs "expose a markRan()"	Decided and sound. Uses markRan(). A pending push is re-armed rather than firing twice; covered by a test.
Item 4: "use it in all three" / "Coordinate with #64"	Decided and sound (partly declined). CountingEventWebhookPayload.direction stays str to keep the documented 400 INVALID_DIRECTION response (asserted in test_i18n.py:209, test_batched_occupancy_broadcast.py:250); the check now uses DIRECTIONS. Reason documented at occupancy_models.py:513-514.
Item 5: narrowing the read model	Decided but questionable (P3). See (c).

Items 2, 3, 6

• Item 2: cams renamed to targets (occupancy_controller.py:393-400). Done.
• Item 3: JSDoc covers ingestMessage, onRawMessage and _handleMessage({ fromNetwork }) (telemetry_engine.js:352-356, 507-513, 556-563). Done.
• Item 6: occupancy_service.py:2165-2168 uses m.code throughout. Done.
• P3, occupancy_service.py:616. direction: Direction on record_counting_event_async is only a type hint; normalisation happens in CountingEventInput. Behaviour correct; noted for clarity.

Reviewer note: the Standards pass suggested narrowing can't break reads because PassengerFlowEvent is only built from validated input. Verified on the branch: OccupancyLiveResponse.recent_passages: list[PassengerFlowEvent] (occupancy_models.py:494) is populated from raw repository rows (get_passenger_flow_telemetry_async), so the P3 above stands.

---

Summary: Standards 6 (all P3) — worst: Direction annotation vs lower-case str passed from controller. Spec 3 (all P3) — worst: narrowed PassengerFlowEvent.direction can 500 the overview on a legacy non-IN/OUT row.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed the PR's own diff (`git diff <base>...<head>`); Standards and Spec ran as independent passes and are reported separately, not reranked. Severity: **P1** must fix · **P2** fix before merge · **P3** minor. Per review policy, pass 1 findings are all to be fixed on the branch. ## Standards The diff breaks no layering, error-schema, or testing standard. The controller stays thin, `/event` keeps `HTTPException` with `X-Error-Code: INVALID_DIRECTION` (code-standards §3), both changes have new tests (§4.3), and `Direction`/`DirectionType` follow the CONTEXT.md glossary. No P1 bugs. ### Documented-standard findings - **P3 (hard, process): PR description is missing sections.** git-and-workflow.md §2.2 requires **Summary**, **Architectural Impact**, **Verification** and **Checklist**. The PR has Problem, Approach and Verification (satisfies AGENTS.md §4) but no Checklist and no Architectural Impact section. It changes the public `createTrailingThrottle` return type and narrows `PassengerFlowEvent.direction`, so both deserve a line there. ### Judgement calls (baseline smells / typing) - **P3: type annotation doesn't match what the controller passes.** `app/controllers/occupancy_controller.py:359` passes `payload.direction` (plain `str`, possibly lower-case) into `record_counting_event_async(direction: Direction)` at `app/services/occupancy_service.py:617`. Works at runtime because `CountingEventInput` upper-cases, but a type checker would reject it. Pass `payload.direction.upper()` or annotate the parameter as `str`. - **P3, possible Primitive Obsession (remaining).** `CountingEventWebhookPayload.direction: str` (`occupancy_models.py:515`) is declined with a reason — fine. `direction: str | None` still appears in `analytics_controller.py:110,207`, `analytics_service.py:522,623` and `occupancy_repository.py:1670`. Outside the diff; a follow-up issue would do if "one Direction type" is meant literally. - **P3, nit:** `DIRECTIONS: tuple[str, ...]` (`occupancy_models.py:213`) could be `tuple[Direction, ...]`. - **P3, redundant write.** On the throttle's own path, `fire()` sets `lastRunAt` (`app/static/js/src/utils.js:110`), then calls `refreshOccupancyData`, which calls `markRan()` again (`app/static/js/app.js:1642`). Harmless; drop the assignment in `fire()` or note in the JSDoc that re-marking is expected. - **P3, undocumented behaviour.** `markRan()` runs before the fetch (`app.js:1642`). A failed or 401 fetch still starts a 15 s window, so the next push waits instead of retrying. Probably acceptable, but document it. - **Possible Middle Man (not worth acting on).** `requestOccupancyRefresh` (`app.js:568`) is now a 2-line wrapper; it keeps call sites readable. ### Notes - The `fire()` re-arm loop is correct: `pending` stays true across the re-arm, so a waiting push is never duplicated; covered by the new fake-clock tests. ## Spec **Verdict:** All six items are addressed. No P1 or P2 issues. Three P3 notes. ### (a) Missing or partial - **P3, `tests/frontend/test_trailing_throttle.test.js:80-121`.** The spec asks for *"a fake-clock test covering 'direct fetch then push' and 'first push after idle'"*. Both tests exist, but they call `schedule.markRan()` by hand. Nothing checks that `refreshOccupancyData()` actually calls it (`app/static/js/app.js:1639-1642`). If that one line were removed, the original bug would come back and no test would fail. ### (b) Scope creep None. The `getOccupancyRefreshThrottle()` extraction (`app.js:547-571`) is the smallest change that lets both paths share one throttle. The `fire()` re-arm in `utils.js:101-111` is the fix the spec asked for. ### (c) Implemented but questionable - **P3, `app/schemas/occupancy_models.py:438`.** The spec says *"Keep in mind that stored rows may predate validation (old `PASS` rows) when narrowing the read model."* `PassengerFlowEvent` also backs `OccupancyOverviewResponse.recent_passages` (`occupancy_service.py:1838-1890`). A single non-IN/OUT row in the DB would now make the overview return a 500. The PR explains the decision: production holds only IN and OUT rows. But nothing stops such a row being written later; the repository still accepts any `evt.get("direction","IN").upper()` (`occupancy_repository.py:1092,1153`). Acceptable; a DB `CHECK` constraint could be a follow-up. ### Open design points | Point | Status | |---|---| | *"Then decide explicitly whether the first push after idle should wait"* | **Decided and sound.** Runs on the next tick. Reason in the JSDoc (`utils.js:76-77`), covered by a test. | | *"route the direct calls through it"* vs *"expose a `markRan()`"* | **Decided and sound.** Uses `markRan()`. A pending push is re-armed rather than firing twice; covered by a test. | | Item 4: *"use it in all three"* / *"Coordinate with #64"* | **Decided and sound (partly declined).** `CountingEventWebhookPayload.direction` stays `str` to keep the documented `400 INVALID_DIRECTION` response (asserted in `test_i18n.py:209`, `test_batched_occupancy_broadcast.py:250`); the check now uses `DIRECTIONS`. Reason documented at `occupancy_models.py:513-514`. | | Item 5: narrowing the read model | **Decided but questionable (P3).** See (c). | ### Items 2, 3, 6 - **Item 2:** `cams` renamed to `targets` (`occupancy_controller.py:393-400`). Done. - **Item 3:** JSDoc covers `ingestMessage`, `onRawMessage` and `_handleMessage({ fromNetwork })` (`telemetry_engine.js:352-356, 507-513, 556-563`). Done. - **Item 6:** `occupancy_service.py:2165-2168` uses `m.code` throughout. Done. - **P3, `occupancy_service.py:616`.** `direction: Direction` on `record_counting_event_async` is only a type hint; normalisation happens in `CountingEventInput`. Behaviour correct; noted for clarity. > Reviewer note: the Standards pass suggested narrowing can't break reads because `PassengerFlowEvent` is only built from validated input. Verified on the branch: `OccupancyLiveResponse.recent_passages: list[PassengerFlowEvent]` (`occupancy_models.py:494`) is populated from raw repository rows (`get_passenger_flow_telemetry_async`), so the P3 above stands. --- **Summary:** Standards 6 (all P3) — worst: `Direction` annotation vs lower-case `str` passed from controller. Spec 3 (all P3) — worst: narrowed `PassengerFlowEvent.direction` can 500 the overview on a legacy non-IN/OUT row. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 09:09:07 +00:00

fix: preserve legacy passage reads and clarify throttle retries 2676094642

gabogg commented 2026-09-25 10:28:20 +00:00

Author

Owner

Copy link

Code review — pass 2 (two-axis)

Re-reviewed at head 2676094. Each axis verified every pass-1 finding against the code (not the commit messages), then reviewed the fix commits for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per review policy, from pass 2 on P1/P2 block merge; P3s go to one follow-up issue for this PR.

Standards

Head 2676094.

Pass-1 findings

Finding	Verdict	Evidence
P3: PR description missing sections (git-and-workflow §2.2)	◐ partial	Now has Architectural Impact and Checklist, but no Summary heading; both new sections sit below the 🤖 Generated with footer; Architectural Impact doesn't mention the public createTrailingThrottle return-type change (schedule + markRan). Approach is now stale (see below).
P3: Direction annotation vs lower-case str from controller	✅ fixed	occupancy_controller.py:360 passes cast(Direction, payload.direction.upper()) after the DIRECTIONS check at :350.
P3: remaining direction: str in analytics/repository	⊘ declined	Reason in the description's "Review follow-up": analytics direction filters keep their string contracts.
P3 nit: DIRECTIONS: tuple[str, ...]	✅ fixed	occupancy_models.py:213 is tuple[Direction, ...].
P3: redundant lastRunAt write in fire() then markRan()	❌ not addressed	fire() still sets lastRunAt = now() then fn() → refreshOccupancyData → markRan() (app.js:1639-1642). No JSDoc note, no reason given.
P3: markRan() before fetch — failed fetch starts the window	✅ fixed	Documented in JSDoc utils.js:82 and the Review follow-up.
Possible Middle Man requestOccupancyRefresh	n/a	No action requested.

New findings

• P3 (process, description accuracy): Approach still says Direction is used by "PassengerFlowEvent (the stored-event read model)" and keeps "Why narrowing the read model is safe". 2676094 reverted that narrowing (occupancy_models.py:438-439, direction: str), so the description contradicts the code and its own Architectural Impact line. Update/remove those lines; add a Summary.
• P3, possible Duplicated Code / inconsistent idiom: PassengerFlowEvent normalises with @field_validator(..., mode="before") calling _upper_direction (occupancy_models.py:446-449), while the module otherwise uses the Annotated[..., BeforeValidator(_upper_direction)] alias pattern (:212). direction: Annotated[str, BeforeValidator(_upper_direction)] matches and drops 4 lines.
• P3, test smell: tests/test_direction_type.py:15-25 parametrises over two models then branches on model is ... twice. The models now have different contracts (write rejects PASS, read keeps it); two plain tests are clearer. The name "normalised_on_every_model" hides that PASS is accepted on one.
• P3, nit: occupancy_controller.py:350 and :360 call payload.direction.upper() twice; bind a local once.

No P1/P2. Layering, error schema (INVALID_DIRECTION 400 kept), async hygiene and the JSDoc-only frontend change are fine. Re-ran test_direction_type, test_i18n, test_batched_occupancy_broadcast (17 passed) and test_trailing_throttle.test.js (7 passed) at 2676094. (ruff not available in the throwaway worktree.)

Merge readiness (this axis): Ready — only P3s remain (→ follow-up issue). Fixing the stale Approach text before merge is recommended.

Spec

Checked at head 2676094 against pass-1 head 85bbec6. New tests run in a throwaway worktree: node --test tests/frontend/test_trailing_throttle.test.js 7/7, pytest tests/test_direction_type.py 3/3.

Pass-1 findings

Finding	Verdict	Evidence
P3: no test checks that refreshOccupancyData() calls markRan()	❌ not addressed	2676094 does not touch tests/frontend/test_trailing_throttle.test.js. markRan() is still only called by hand in the tests (lines 85, 104, 116). Deleting the call at app.js:1641-1642 would fail nothing. The PR description does not decline this.
P3: narrowed PassengerFlowEvent.direction could 500 on a legacy row	✅ fixed (by reverting)	occupancy_models.py:438 back to direction: str, with a comment and an upper-casing field_validator (446-449). test_direction_type.py asserts PASS is accepted on the read model and rejected on CountingEventInput.
P3: direction: Direction on record_counting_event_async was only a hint; controller passed raw str	✅ fixed	occupancy_controller.py:360 passes cast(Direction, payload.direction.upper()); the DIRECTIONS guard at line 350 makes the cast sound.

Does "preserve legacy passage reads" undermine "one Direction type"? No — it only reverts the narrowing. Item 4 ("Define one Direction type alias and use it in all three": /event check, CountingEventInput, webhook payload) is untouched by 2676094. The change affects item 5; the service parameter is now typed, and the read model went back to str as the spec anticipated: "Keep in mind that stored rows may predate validation (old PASS rows) when narrowing the read model." Versus master, the read model now only upper-cases stored directions on read — harmless, since the repository upper-cases on write.

New findings

• P2 — PR description contradicts the code on item 5. Spec: "Items 2–6 fixed, or explicitly declined with a reason." The Approach section still says Direction is "used by CountingEventInput, PassengerFlowEvent (the stored-event read model)…" and keeps the bullet "Why narrowing the read model is safe:". Only the appended "Review follow-up" and the code comment at occupancy_models.py:437 say the read model was not narrowed. Rewrite that Approach bullet as a stated decline ("read model stays str for legacy rows; normalised only"). Text-only.
• P3 — JSDoc retry sentence overstates behaviour. Spec item 1: "have the throttle record every run". utils.js:82: "A failed direct fetch still starts the window; the next push retries when it ends." The throttle never retries on its own; a fetch happens only if a push arrives, and then waits for the window end. Suggested: "…a push arriving during the window is held until it ends; nothing retries on its own."
• P3 — write-path return type looser than needed. record_counting_event_async accepts Direction but returns PassengerFlowEvent with direction: str, so callers lose the validated type. Acceptable given the decline; follow-up issue.

Quick re-check of the whole diff: no other P1/P2.

Merge readiness (this axis): Not ready until the P2 (description-only edit) is fixed. The unaddressed pass-1 P3 (markRan() wiring untested) should be fixed or explicitly declined; other P3s can go to a follow-up issue.

---

Summary: Blocking (P2): Spec — PR description's Approach still claims the read model was narrowed (contradicts 2676094); description-only fix. Standards ready (P3 only). Worst per axis: Standards → stale description / missing Summary (P3); Spec → the same contradiction (P2). Unaddressed pass-1 P3: nothing tests that refreshOccupancyData() calls markRan().

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at head `2676094`. Each axis verified every pass-1 finding against the code (not the commit messages), then reviewed the fix commits for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per review policy, from pass 2 on **P1/P2 block merge; P3s go to one follow-up issue** for this PR. ## Standards Head 2676094. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P3: PR description missing sections (git-and-workflow §2.2) | ◐ partial | Now has **Architectural Impact** and **Checklist**, but no **Summary** heading; both new sections sit *below* the `🤖 Generated with` footer; Architectural Impact doesn't mention the public `createTrailingThrottle` return-type change (`schedule` + `markRan`). Approach is now stale (see below). | | P3: `Direction` annotation vs lower-case `str` from controller | ✅ fixed | `occupancy_controller.py:360` passes `cast(Direction, payload.direction.upper())` after the `DIRECTIONS` check at `:350`. | | P3: remaining `direction: str` in analytics/repository | ⊘ declined | Reason in the description's "Review follow-up": analytics direction filters keep their string contracts. | | P3 nit: `DIRECTIONS: tuple[str, ...]` | ✅ fixed | `occupancy_models.py:213` is `tuple[Direction, ...]`. | | P3: redundant `lastRunAt` write in `fire()` then `markRan()` | ❌ not addressed | `fire()` still sets `lastRunAt = now()` then `fn()` → `refreshOccupancyData` → `markRan()` (`app.js:1639-1642`). No JSDoc note, no reason given. | | P3: `markRan()` before fetch — failed fetch starts the window | ✅ fixed | Documented in JSDoc `utils.js:82` and the Review follow-up. | | Possible Middle Man `requestOccupancyRefresh` | n/a | No action requested. | ### New findings - **P3 (process, description accuracy):** Approach still says `Direction` is used by "`PassengerFlowEvent` (the stored-event read model)" and keeps "**Why narrowing the read model is safe**". 2676094 reverted that narrowing (`occupancy_models.py:438-439`, `direction: str`), so the description contradicts the code and its own Architectural Impact line. Update/remove those lines; add a Summary. - **P3, possible Duplicated Code / inconsistent idiom:** `PassengerFlowEvent` normalises with `@field_validator(..., mode="before")` calling `_upper_direction` (`occupancy_models.py:446-449`), while the module otherwise uses the `Annotated[..., BeforeValidator(_upper_direction)]` alias pattern (`:212`). `direction: Annotated[str, BeforeValidator(_upper_direction)]` matches and drops 4 lines. - **P3, test smell:** `tests/test_direction_type.py:15-25` parametrises over two models then branches on `model is ...` twice. The models now have different contracts (write rejects `PASS`, read keeps it); two plain tests are clearer. The name "normalised_on_every_model" hides that `PASS` is accepted on one. - **P3, nit:** `occupancy_controller.py:350` and `:360` call `payload.direction.upper()` twice; bind a local once. No P1/P2. Layering, error schema (`INVALID_DIRECTION` 400 kept), async hygiene and the JSDoc-only frontend change are fine. Re-ran `test_direction_type`, `test_i18n`, `test_batched_occupancy_broadcast` (17 passed) and `test_trailing_throttle.test.js` (7 passed) at 2676094. (ruff not available in the throwaway worktree.) **Merge readiness (this axis):** Ready — only P3s remain (→ follow-up issue). Fixing the stale Approach text before merge is recommended. ## Spec Checked at head `2676094` against pass-1 head `85bbec6`. New tests run in a throwaway worktree: `node --test tests/frontend/test_trailing_throttle.test.js` 7/7, `pytest tests/test_direction_type.py` 3/3. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P3: no test checks that `refreshOccupancyData()` calls `markRan()` | ❌ not addressed | `2676094` does not touch `tests/frontend/test_trailing_throttle.test.js`. `markRan()` is still only called by hand in the tests (lines 85, 104, 116). Deleting the call at `app.js:1641-1642` would fail nothing. The PR description does not decline this. | | P3: narrowed `PassengerFlowEvent.direction` could 500 on a legacy row | ✅ fixed (by reverting) | `occupancy_models.py:438` back to `direction: str`, with a comment and an upper-casing `field_validator` (446-449). `test_direction_type.py` asserts `PASS` is accepted on the read model and rejected on `CountingEventInput`. | | P3: `direction: Direction` on `record_counting_event_async` was only a hint; controller passed raw `str` | ✅ fixed | `occupancy_controller.py:360` passes `cast(Direction, payload.direction.upper())`; the `DIRECTIONS` guard at line 350 makes the cast sound. | **Does "preserve legacy passage reads" undermine "one Direction type"?** No — it only reverts the narrowing. Item 4 (*"Define one `Direction` type alias and use it in all three"*: `/event` check, `CountingEventInput`, webhook payload) is untouched by `2676094`. The change affects item 5; the service parameter is now typed, and the read model went back to `str` as the spec anticipated: *"Keep in mind that stored rows may predate validation (old `PASS` rows) when narrowing the read model."* Versus master, the read model now only upper-cases stored directions on read — harmless, since the repository upper-cases on write. ### New findings - **P2 — PR description contradicts the code on item 5.** Spec: *"Items 2–6 fixed, or explicitly declined with a reason."* The **Approach** section still says `Direction` is "used by `CountingEventInput`, `PassengerFlowEvent` (the stored-event read model)…" and keeps the bullet "**Why narrowing the read model is safe:**". Only the appended "Review follow-up" and the code comment at `occupancy_models.py:437` say the read model was not narrowed. Rewrite that Approach bullet as a stated decline ("read model stays `str` for legacy rows; normalised only"). Text-only. - **P3 — JSDoc retry sentence overstates behaviour.** Spec item 1: *"have the throttle record every run"*. `utils.js:82`: "A failed direct fetch still starts the window; the next push retries when it ends." The throttle never retries on its own; a fetch happens only if a push arrives, and then waits for the window end. Suggested: "…a push arriving during the window is held until it ends; nothing retries on its own." - **P3 — write-path return type looser than needed.** `record_counting_event_async` accepts `Direction` but returns `PassengerFlowEvent` with `direction: str`, so callers lose the validated type. Acceptable given the decline; follow-up issue. Quick re-check of the whole diff: no other P1/P2. **Merge readiness (this axis):** Not ready until the P2 (description-only edit) is fixed. The unaddressed pass-1 P3 (`markRan()` wiring untested) should be fixed or explicitly declined; other P3s can go to a follow-up issue. --- **Summary:** **Blocking (P2):** Spec — PR description's Approach still claims the read model was narrowed (contradicts 2676094); description-only fix. Standards ready (P3 only). Worst per axis: Standards → stale description / missing Summary (P3); Spec → the same contradiction (P2). Unaddressed pass-1 P3: nothing tests that `refreshOccupancyData()` calls `markRan()`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 12:14:45 +00:00

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

gabogg merged commit c631c1c71d into master 2026-09-25 12:17:38 +00:00

gabogg deleted branch fix/telemetry-throttle-direction-typing 2026-09-25 12:17:38 +00:00

gabogg referenced this pull request from a commit 2026-09-25 12:17:39 +00:00

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

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

blocked

Waits on the issues listed in its "Blocked by:" line before implementation

  

bug

Category: something is broken; behaviour differs from the spec or docs

  

enhancement

Category: new feature or improvement, including refactors, chores and docs

  

high-priority

Address before ordinary-priority work; independent of triage readiness and blockers

  

low-priority

Address after ordinary-priority work; independent of triage readiness and blockers

  

needs-info

Waiting on reporter for more information

  

needs-triage

Maintainer needs to evaluate this issue

  

ready-for-agent

Fully specified, ready for an AFK agent

  

ready-for-human

Requires human implementation

  

referenced

Previously closed issue reopened or referenced by an open issue; visible history marker

  

research

Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  

wontfix

Will not be actioned

No labels

blocked

bug

enhancement

high-priority

low-priority

needs-info

needs-triage

ready-for-agent

ready-for-human

referenced

research

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gabogg/hikcentral!91

No description provided.