Skip to content

feat: use long-poll endpoint for line participants - #691

Open
birme wants to merge 1 commit into
mainfrom
feature-writer/30-long-poll-participants
Open

birme wants to merge 1 commit into
mainfrom
feature-writer/30-long-poll-participants

Conversation

@birme

@birme birme commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add API.fetchLineParticipants, a POST to the manager's long-poll endpoint (/production/:productionId/line/:lineId/participants) that the server holds open until participants change or it times out.
  • Rewrite useLinePolling to seed line metadata once via fetchProductionLine, then keep the participant list current by re-issuing the long-poll request each time it resolves — replacing the previous fixed 1s interval poll of the full line.
  • Preserve the existing consecutive-failure handling (ERROR dispatch at 5, stop at 10) and add AbortController-based cancellation so in-flight requests are aborted on unmount/param change.

Test plan

  • Tests pass (npm test — 147 passed)
  • TypeScript compiles (npm run typecheck)
  • Lint clean (npm run lint)
  • Manual: join a line, confirm participant list updates on join/leave with no 1s polling in the network tab

Closes #30

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

Replace the 1s interval poll of the full production line with the manager's
long-poll participants endpoint. Line metadata is seeded once up front, then
participants are kept current by re-issuing the held-open request each time it
resolves, cutting redundant request volume while preserving the existing
consecutive-failure error handling.

Closes #30
@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Code Review

Verdict: Needs Changes

Summary: Solid long-poll rewrite: routes through the API object and handleFetchRequest, uses AbortController for cancellation, and preserves the 5/10 consecutive-failure escalation. However, a rewritten custom hook ships with no renderHook test, and there is a failure-loop concern. A cluster of 3+ warnings with no blocking item yields Needs Changes per the rubric.


Blocking

None.


Warnings

  • src/components/production-line/use-line-polling.ts (whole file) — Substantially rewritten (interval poll → recursive long-poll) but no use-line-polling.test.ts exists (sibling hooks all have renderHook tests). Add tests covering abort-on-unmount, the 5/10 failure thresholds, and success re-issue. Primary reason for Needs Changes.
  • use-line-polling.ts:76-92 — On a persistent server error the long-poll rejects quickly and poll() re-issues with no backoff, so a fast-failing endpoint spins a tight request loop until 10 failures accumulate. Add a small delay/backoff before re-issuing on failure.
  • use-line-polling.ts:88setLine((prev) => prev ? { ...prev, participants } : prev) silently drops the update if the poll resolves before seeding sets line. Unlikely given current ordering, but a latent gap; guard or comment.

Suggestions

  • Hoist isAbortError to a shared util if other long-poll callers appear.
  • Document the server-side long-poll timeout in fetchLineParticipants so callers understand the resolve cadence.

Domain Note

Affects participant-list presence updates; behavior now depends on the manager's users:change emission and long-poll timeout (see intercom-manager #305/PR). Confirm the two are aligned.

Posted by daily-backlog-pr Phase 3; moving back to Ready.

@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Code Review — Verdict: NEEDS CHANGES

(Posted as a comment: GitHub blocks a formal request-changes review on a self-authored PR under this automation account. Treat this as the review of record.)

Summary: The change replaces 1000ms interval polling with a long-poll loop fetching participants only, seeding line metadata via a one-shot fetchProductionLine. API.fetchLineParticipants correctly routes through the API object and handleFetchRequest, and AbortController wiring is clean. Zero Blocking, but three Warnings — per the review policy (a cluster of 3+ Warnings) this is Needs Changes.

Warnings

  • src/components/production-line/use-line-polling.ts:137-141 — No backoff on the failure path: on a fast-failing endpoint, catch runs handleFailure() then immediately re-invokes poll(), hammering the manager in a tight loop until the 10-failure threshold. Add an (ideally increasing) delay before re-issuing poll().
  • src/components/production-line/use-line-polling.ts — No use-line-polling.test.ts exists, yet the directory establishes the renderHook pattern (use-rtc-connection.test.ts, etc.). This change materially alters control flow (recursive long-poll, abort handling, seed-then-poll ordering) and should have renderHook coverage including abort-on-unmount and failure-threshold-dispatch paths.
  • src/components/production-line/use-line-polling.ts:40-41 — New pure helper isAbortError has no unit test; it's the sole guard preventing abort-driven false ERROR dispatches.

Suggestions

  • src/api/api.ts:12-28fetchLineParticipants uses POST with no body to fetch; confirm the backend contract (a GET may be more idiomatic for a long-poll read).
  • src/api/api.ts:31-37 — local TParticipant duplicates the exported one in types.ts; import the shared type to avoid drift.

Moving back to Ready for backoff + hook/pure-function tests.

@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Automated code-reviewer verdict (daily-backlog-pr Phase 3): NEEDS CHANGES

Note: this run's automation token authored this PR, so GitHub blocks a state-bearing --request-changes review (self-review). Findings posted as a comment; board item moved back to Ready. Needs a different reviewer identity to formally gate/merge.

The core long-poll migration is well-executed — metadata seeded once via fetchProductionLine, then participants kept current by re-issuing the long-poll; AbortController + cancelled flag correctly cancel in-flight requests and prevent post-unmount setState; the setLine(prev => prev ? {...prev, participants} : prev) merge preserves seeded metadata. No blocking issues on the happy path. But:

High

  • Tight retry loop on the error path — no backoff (regression + issue Add exponential backoff to all polling loops on error #596). use-line-polling.ts:62-80: on a failing request handleFailure() returns true and poll() is re-invoked with zero delay. A failing fetch rejects immediately (unlike the held-open success case), so a persistent error (500/network/CORS) fires ~10 requests back-to-back before the 10-failure cap stops it. The prior setInterval(…, 1000) spaced failures 1s apart, so this is a behavioral regression that worsens the request-storm scenario tracked in open issue Add exponential backoff to all polling loops on error #596. Add a delay before re-poll() on the failure branch (use-line-polling.ts:76-79).

Medium

  • No tests for new behavior. Neither API.fetchLineParticipants (api.ts:202-222) nor the rewritten useLinePolling hook has a unit test, despite concurrency-sensitive logic (abort-on-unmount, cancelled guards, seed-then-loop ordering, failure counter). The Test plan claims 147 pass but adds none for the changed surface.
  • 401 not distinguished — no recovery / circuit breaker (issue Add global auth-failure circuit breaker to coordinate all polling on 401 #597). A 401 increments the same failure counter and, after 10 immediate failures, stops polling permanently without coordinating with the re-auth flow. Combined with the tight loop, an expired token yields a 10-request 401 burst. OK to defer to Add global auth-failure circuit breaker to coordinate all polling on 401 #597, but add a comment noting the coupling.

Nits

  • isAbortError guard sits at the right layer; the double consecutiveFailureCount = 0 reset is redundant but harmless. Style conforms.

@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Automated code-reviewer verdict (daily-backlog-pr Phase 3): NEEDS CHANGES

Solid direction: the new REST call goes through the API object + handleFetchRequest, global state is still mutated only via typed dispatch actions, and AbortController cleanup on unmount improves on the old interval. But two blocking items remain.

Blocking

  • No delay/backoff on failure — tight retry loop. use-line-polling.ts:75-79: on a failed fetch, poll() is re-issued immediately with zero delay. If the manager is down / returns 5xx / fails fast, this is an unthrottled hot loop (also spamming logger.red) until the 10-failure cutoff. The old setInterval had an implicit 1s floor. Reintroduce a delay (ideally jitter/backoff) before re-issuing after a failure, and clear that timer in cleanup.
  • No tests for the rewritten hook. No test file added despite an established renderHook convention (use-audio-elements.test.ts, use-rtc-connection.test.ts, etc.). Cover: seed-success→poll, seed-failure→conditional poll, participant merge preserving metadata, abort-on-unmount not dispatching ERROR, and the 5/10 thresholds — via fake timers, not real setTimeout.

Warnings

  • Seed .catch (:91-94) re-drives poll() with no delay either — same tight-loop root cause.
  • Participant-only merge (:74) no-ops when prev is null: if the seed fails once but long-poll works, the list never populates and the line stays null (seed is never retried).
  • TParticipant duplicated as a local type in api.ts:31 vs. the exported one in types.ts:23 — prefer importing the shared type.

Moving issue #30 back to Ready.


Note: this run could not post this as a formal --request-changes review because the PR author (birme) is the same account the automation runs as, and GitHub blocks self-reviews. Posting the verdict as a comment instead. Because it is not a CHANGES_REQUESTED-state review, the Phase-2 escalation guard cannot count it automatically — a human reviewer should formally request changes. Moving the linked issue back to Ready.

@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

daily-backlog-pr Phase 3 — automated review (verdict: Needs Changes)

⚠️ Pipeline identity (birme) is also this PR's author; GitHub blocks self-approve/self-request-changes and main requires 1 approving review. Recording the verdict as a note and flagging for a human reviewer.

Findings

  • Major — unthrottled hot-loop. In the rewritten useLinePolling, both the success and failure paths of poll() re-invoke poll() immediately with no delay. The old code was interval-gated at 1000ms. If the manager's long-poll endpoint returns quickly instead of holding the connection (old/misbehaving manager, an immediate 4xx/5xx, or a proxy that returns instantly), the loop spins as fast as the network round-trip allows. The >= 10 failure cap bounds failures but not fast successes, so an immediate-200 endpoint loops forever. Add a small backoff on the failure path and a minimum floor on success.
  • Minor — no test coverage for the new control flow (abort-on-unmount, no re-issue after cancel, failure counting/cap, hot-loop). CI green only reflects the absence of tests here.
  • Nit — if the initial fetchProductionLine seed fails, poll() proceeds with no line metadata and participant updates are silently dropped until a later seed that never re-fires; consider retrying the seed.

Abort/cleanup and stale-closure handling are otherwise correct.

@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

daily-backlog-pr automated code review. A state-bearing GitHub review could not be posted because this PR was opened by the same account the automation runs as (birme); GitHub forbids approving/requesting-changes on ones own PR. Recording the verdict as a comment instead. Board item moved back to Ready for rework.

Code Review

Verdict: Needs Changes

Summary: The refactor to long-polling is architecturally clean (routes through API/handleFetchRequest, proper AbortController cleanup, no direct state mutation), but there is no backoff on the error path (tight retry loop) and no test coverage for the changed hook.

Blocking

  • src/components/production-line/use-line-polling.ts:79,93 — No backoff on error. When fetchLineParticipants fails fast (401/403/500 returned immediately), poll() re-invokes with zero delay, hammering the manager up to 10 times in quick succession before stopping. The old interval version was implicitly floored at 1s between attempts; that floor is gone. Add a delay (ideally exponential, scaled by consecutiveFailureCount) before re-issuing after a failure.
  • src/components/production-line/use-line-polling.ts — No renderHook Vitest test for the changed hook. Every other hook in this directory has a .test.ts. Cover the happy path (re-issue on resolve, setLine merges participants) and the error/abort path (AbortError swallowed on unmount, escalation at 5 and 10). Use fake timers, not setTimeout waits.

Warnings

  • use-line-polling.ts:74 vs :91-94 — If initial fetchProductionLine rejects, poll() starts while line is still null; the setLine((prev) => prev ? {...} : prev) updater then silently discards every participant update forever. Seed a minimal line object or retry metadata.
  • src/api/api.ts:205fetchLineParticipants uses POST for a semantically read-only long-poll. Confirm the manager contract; GET is more conventional if idempotent.

Suggestions

  • src/api/api.ts:209TParticipant is duplicated in api.ts:31 and types.ts:23; import the canonical type to avoid drift.
  • use-line-polling.ts:13isAbortError is a good candidate to hoist into a shared util.

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Automated code review (daily-backlog-pr) — Needs Changes

Note: posted as a comment rather than a formal "Request changes" review because the pipeline account authored this PR and GitHub blocks self-review.

The core design (long-poll loop with AbortController cancellation) is sound: abort/race handling and the 5/10 consecutive-failure semantics are correctly preserved and the call routes through the API object. Two blockers before merge:

  1. No backoff on fast failures — unthrottled retry loop. In poll() (src/components/production-line/use-line-polling.ts), on a caught error handleFailure() runs and, while consecutiveFailureCount < 10, control falls straight back into poll() with zero delay. If the endpoint fails fast (manager down / immediate 5xx / connection refused), this busy-loops hammering the manager until 10 failures accumulate. The previous setInterval(..., 1000) gave an inherent 1s floor; the rewrite removes it. Add a short delay/backoff before re-issuing on failure.
  2. No tests for a substantially-rewritten hook. The PR ships zero tests. Project convention requires renderHook coverage for changed hooks — especially to lock in that aborted requests do not dispatch ERROR and that the 5/10 thresholds hold.

Moving back to Ready on the board.

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

code-reviewer verdict: NEEDS CHANGES (PR #691, closes #30)

AbortController cleanup and 5/10 failure escalation are correct, but:

  • Blocking: tight-loop on error. The server only holds the request ~25s on success; on 5xx/network failure handleFetchRequest rejects immediately and poll() re-issues with zero delay (failure counts 1–9), hammering the manager. The old 1s interval was naturally rate-limited. Add a backoff delay before re-issuing on the failure path (both the poll catch and the seed fetchProductionLine().catch).
  • Blocking: no test coverage for the changed hook that closes Use long polling API for fetching line participants #30 (every sibling hook has renderHook tests) — cover success re-issue, abort-on-unmount, failure escalation, and no-synchronous-retry.

Note: recorded as a comment, not a state-bearing --request-changes review, because this PR's author and the automation account are both birme and GitHub blocks self-reviews. This does NOT increment the branch-protection review count or the escalation guard.

@birme

birme commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Automated code-review verdict: NEEDS CHANGES — implements #30.
Blocking:

  • The rewritten use-line-polling.ts hook (seed+long-poll loop) ships with no test — project rules require renderHook coverage. Needs: seed→poll ordering, participants merge into line, failure-count escalation (dispatch at 5, stop at 10), cleanup aborting the in-flight request.
  • New API.fetchLineParticipants in api.ts has no unit test (assert POST method, URL shape, Authorization header, AbortSignal threading).
    Warning: setLine((prev) => prev ? {...prev, participants} : prev) silently drops updates while prev is null — if the seed fetch fails but retry proceeds, line stays null forever while poll keeps hammering the endpoint.

Posted as a comment, not a state-bearing GitHub review: the daily-backlog-pr review identity (birme) is also this PR's author, so GitHub blocks a self request-changes. Board item left at In review (#48) and flagged for a human to action the changes below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use long polling API for fetching line participants

1 participant