fix(review): make the visual-preview poll budget durable per head SHA - #6335
Conversation
Closes #6323 MAX_PREVIEW_POLLS (processors.ts) was meant to bound the visual-preview self-poll to 5 attempts before giving up on a preview deploy that never becomes discoverable. It did nothing in practice: the counter only lived inside the dedicated recapture-preview job chain's own `attempt` payload field, and at least three other re-review triggers (CI-completion webhooks, deployment_status webhooks, the sweep pass) call reReviewStoredPullRequest without threading it through -- each one independently re-arms a fresh 5-attempt budget whenever buildCapture reports previewPending. Confirmed live on JSONbored/metagraphed#6036: 7 check-runs completing over ~8 minutes, each capable of re-arming the countdown, produced 12+ re-review comment edits over 52+ minutes on a single PR, still ongoing when observed -- pure wasted Browser Rendering + queue/webhook cost on a PR whose "after" side could never resolve anyway. New src/review/visual/preview-poll-budget.ts tracks attempts durably, keyed by head SHA, in an R2 marker mirroring actions-fallback.ts's own isFallbackDispatchInFlight/markFallbackDispatched pattern exactly (fail-open reads, best-effort writes, a 24h max-age fail-safe expiry). buildCapture consults + increments it before treating a still-building preview as poll-worthy; once exhausted for a head, previewPending becomes false and the honest "review manually" FAILED placeholder shows instead of an eternal loading spinner. Because every caller already only reschedules when previewPending is true, no other call site needs to change -- the fix is fully contained to where the budget check lives. processors.ts's local MAX_PREVIEW_POLLS is consolidated into the new module's exported MAX_PREVIEW_POLL_ATTEMPTS (one source of truth) and left in place as a now-redundant secondary bound for the dedicated self-poll job chain -- harmless, but no longer the thing that actually stops a never-resolving preview from polling forever.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-16 02:43:40 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://gittensory.aethereal.dev/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6335 +/- ##
=======================================
Coverage 95.60% 95.60%
=======================================
Files 596 597 +1
Lines 47135 47160 +25
Branches 15006 15014 +8
=======================================
+ Hits 45063 45088 +25
Misses 1290 1290
Partials 782 782
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
MAX_PREVIEW_POLLS(processors.ts) was meant to bound the visual-preview self-poll to 5 attempts before giving up on a preview deploy that never becomes discoverable. It did nothing in practice: the counter only lived inside the dedicatedrecapture-previewjob chain's ownattemptpayload field, and at least three other re-review triggers — the CI-completion (check_run/check_suite) webhook handler, thedeployment_statuswebhook handler, and the sweep pass — callreReviewStoredPullRequestwithout threading it through, so it silently reads back as0and re-arms a fresh 5-attempt budget every time.src/review/visual/preview-poll-budget.tstracks attempts durably, keyed by head SHA, in an R2 marker mirroringactions-fallback.ts's ownisFallbackDispatchInFlight/markFallbackDispatchedpattern exactly — same fail-open-on-read-error contract, same best-effort-write contract, same max-age fail-safe expiry (24h) so a marker can never permanently block a genuinely new push.buildCaptureconsults + increments the budget before treating a still-building preview as poll-worthy. Once exhausted for a head,previewPendingbecomesfalseand the honest "review manually" FAILED placeholder shows instead of an eternally-spinning "loading" one.capture.previewPendingistrue, no other call site needed to change — the fix is fully contained to where the budget check lives, regardless of which trigger caused thebuildCapturecall.processors.ts's localMAX_PREVIEW_POLLSis consolidated into the new module's exportedMAX_PREVIEW_POLL_ATTEMPTS(one source of truth) and left in place as a now-redundant secondary bound for the dedicated self-poll job chain — harmless, but no longer the thing that actually stops a never-resolving preview from polling forever.Closes #6323
Test plan
test/unit/preview-poll-budget.test.ts(13 tests, 100% stmt/branch/line coverage): count tracking, per-head independence, max-age expiry (and thatfirstAttemptAtis preserved across increments, not reset), malformed/missing-field marker degradation, fail-open reads, best-effort writestest/unit/visual-capture.test.tsextended: abuildingbuildState records one attempt; pastMAX_PREVIEW_POLL_ATTEMPTSfor a head,buildCapturereturnspreviewPending: falseand theplaceholder=failedURL, and does NOT keep incrementing past the capvisual-capture.test.tssuite (106 tests) andqueue-4.test.ts(89 tests, including therecapture-previewjob-dispatch tests) pass unchanged — the fix is backward compatible with every existing scenarionpm run typecheckcleannpm run test:cigate green