Skip to content

fix(review): paginate findPreviewUrlFromChecks's check-runs read - #7838

Closed
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:fix/paginate-checks-preview-url-7779
Closed

fix(review): paginate findPreviewUrlFromChecks's check-runs read#7838
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:fix/paginate-checks-preview-url-7779

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #7779.

findPreviewUrlFromChecks (src/review/visual/preview-url.ts) fetched only page 1 of a commit's check-runs (?per_page=100) and never followed pagination — even though the exact same endpoint, in the same file, is walked correctly via the file's own findAcrossPages helper by both getPreviewBuildState and findPreviewUrlFromPrComments. The file header comment already reasons about this exact truncation (">100 check-runs would push the Cloudflare Workers Builds check-run onto page 2+ … return null/absent as if it genuinely didn't exist"), but this one function never got the fix its neighbours have.

Called from capture.ts:163 as the primary preview-URL-discovery step. On a repo with matrix builds / many external checks (>100 check-runs on the head SHA), the Workers Builds check-run lands on page 2+, so findPreviewUrlFromChecks returned null while the later getPreviewBuildState (which paginates) correctly reported succeeded — the system knew a preview deploy succeeded but never surfaced its URL, leaving a permanent loading spinner instead of the screenshot.

The fix

Reuse findAcrossPages (the pattern the issue requires — no new pagination loop) for the check-runs read, moving the per-run scan into its probe so the walk stops as soon as a page yields a usable URL. The pre-existing best-effort behavior is preserved: the paginated walk is wrapped in .catch(() => null) so a check-runs read error still degrades to "no URL from this source" rather than throwing, and the earlier combined-commit-status short-circuit is unchanged.

Tests (added to test/unit/preview-url.test.ts, mirroring the sibling getPreviewBuildState pagination tests)

  • Regression (the bug): a commit whose page 1 is 100 non-preview check-runs advertising a next page, with the Workers Builds check-run on page 2 — asserts the preview URL is found and that page 2 was actually fetched. Verified bug-catching: reverting to the single-page read makes this fail (URL missed).
  • Skips a completed non-success check-run (continue) and resolves the URL from the details_url ?? output.summary ?? output.text fallback chain.
  • No preview anywhere + no check_runs array → null (no throw).
  • Check-runs read error → .catch degrades to null.
  • Combined commit-status success short-circuit returns before check-runs are ever fetched.
  • 100% of the changed lines and branches covered (measured on the diff, unsharded).

Validation

  • preview-url.ts typecheck clean; test/unit/preview-url.test.ts (28) + consumer queue-3.test.ts (145) green
  • git diff --check clean; scoped to the two files above; rebased onto latest main, mergeable-clean

findPreviewUrlFromChecks fetched only page 1 of a commit's check-runs and
never followed pagination, unlike its siblings getPreviewBuildState and
findPreviewUrlFromPrComments which walk the same endpoint via the file's own
findAcrossPages helper. On a commit with >100 check-runs the Cloudflare
Workers Builds check-run can land on page 2+, so this discovery returned null
even though the check existed -- the PR then showed a permanent loading spinner
instead of the real preview screenshot. Reuse findAcrossPages (as the issue
requires) so the check-runs scan walks every page, stopping as soon as a page
yields a usable preview URL, and preserve the pre-existing best-effort
degrade-to-null on a read error.
@shin-core
shin-core requested a review from JSONbored as a code owner July 21, 2026 14:38
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 21, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.76%. Comparing base (0ff0b8f) to head (717fb3c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7838      +/-   ##
==========================================
- Coverage   91.37%   88.76%   -2.61%     
==========================================
  Files         729       89     -640     
  Lines       74688    21348   -53340     
  Branches    22793     3720   -19073     
==========================================
- Hits        68246    18950   -49296     
+ Misses       5396     2203    -3193     
+ Partials     1046      195     -851     
Flag Coverage Δ
shard-1 44.02% <37.50%> (-10.47%) ⬇️
shard-2 2.23% <0.00%> (-52.97%) ⬇️
shard-3 63.43% <100.00%> (+12.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/visual/preview-url.ts 75.37% <100.00%> (+2.85%) ⬆️

... and 640 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 21, 2026
@loopover-orb

loopover-orb Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-21 14:50:20 UTC

2 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This closes #7779 by making findPreviewUrlFromChecks reuse the existing findAcrossPages helper for the check-runs read, exactly mirroring the sibling getPreviewBuildState/findPreviewUrlFromPrComments pattern already in the file, and correctly preserves the .catch(() => null) best-effort degrade plus the commit-status short-circuit. The per-run scan logic (skip/extract) is moved unchanged into the probe callback, so the fix is a faithful pagination retrofit rather than a behavior rewrite. Tests directly exercise the bug scenario (Workers Builds check-run landing on page 2) and assert page 2 was actually fetched, which is real coverage of the fixed path, not a fabricated one.

Nits — 3 non-blocking
  • The double-cast in the selectItems callback (`payload as { check_runs?: ... } ... ?? []`) duplicates the check_runs shape already declared in the generic — could extract a shared type instead of restating it inline (src/review/visual/preview-url.ts:243-245).
  • The CI checks 'validate' and 'validate-tests-merge' failed with no detail provided; given the branch is 1 commit behind default, this is more likely a merge/base drift issue than a defect in this diff — worth a rebase to confirm.
  • Consider extracting the shared check_runs item type (used in both the generic type param and the selectItems cast) into a single named type to avoid repeating the same inline object shape twice in one call (src/review/visual/preview-url.ts:239-245).

CI checks failing

  • validate
  • validate-tests-merge
Flagged checks (non-blocking)
  • Contributor trust — Contributor flagged for review

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7779
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 90 registered-repo PR(s), 50 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 90 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff replaces the single-page check-runs fetch in findPreviewUrlFromChecks with a call to the file's existing findAcrossPages helper, matching the pattern used by its sibling functions as required, and adds a regression test with a check-run on page 2 plus additional coverage tests.

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 90 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/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.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests-merge)). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

findPreviewUrlFromChecks doesn't paginate check-runs, unlike its sibling functions in the same file

1 participant