Skip to content

fix(benchmark): render fork benchmark comments without dashboard uploads#2485

Open
NathanDrake2406 wants to merge 4 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/fork-benchmark-comments
Open

fix(benchmark): render fork benchmark comments without dashboard uploads#2485
NathanDrake2406 wants to merge 4 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/fork-benchmark-comments

Conversation

@NathanDrake2406

Copy link
Copy Markdown
Contributor

Overview

Area Details
Goal Let fork PR benchmark publisher runs produce GitHub comments without requiring the private dashboard ingest secret.
Core change Missing dashboard uploads now produce an explicit skipped-upload response, and the formatter can render paired PR comparisons directly from perf-results.json.
Primary files benchmarks/perf/upload-results.mjs, benchmarks/perf/format-pr-comment.mjs, tests/performance-benchmarks.test.ts
Expected impact Upstream dashboard-backed comments keep the existing path. Fork PR publisher runs can still generate local comparison comments when dashboard upload is unavailable.

Why

The trusted publisher validates benchmark artifacts before publishing, but fork repositories do not have COMPAT_INGEST_SECRET. That missing secret is a valid capability boundary, not a benchmark failure. The publisher should skip dashboard upload while still using the paired samples already present in the validated PR artifact.

Area Principle / invariant What this PR changes
Dashboard upload Writing to the shared benchmark dashboard requires the ingest secret. The uploader records { "uploaded": false, "reason": "missing_secret" } instead of silently omitting the response file.
PR comments Comment rendering should depend on validated benchmark data, not on private dashboard access. The formatter derives paired rows from baselineSamples and samples when no dashboard comparison is available.
Existing upstream path Dashboard-backed upstream comments should remain unchanged. If the upload response includes comparison, the existing dashboard comparison and link are still used.

What changed

Scenario Before After
Fork PR without COMPAT_INGEST_SECRET Upload skipped, performance-upload.json was absent, and format-pr-comment.mjs failed with ENOENT. Upload skip is explicit, and the formatter can render from the local paired artifact.
Missing upload response file The formatter crashed while reading the response. The formatter treats ENOENT as a skipped upload and falls back to local paired comparison data.
Dashboard upload succeeds Formatter used uploaded comparison data and linked detailed traces. Same behavior.

Validation

  • vp test run --project unit tests/performance-benchmarks.test.ts
  • vp check tests/performance-benchmarks.test.ts benchmarks/perf/format-pr-comment.mjs benchmarks/perf/upload-results.mjs
  • git diff --check

References

Link Why it matters
https://github.com/NathanDrake2406/vinext/actions/runs/28583352418 Original fork publisher failure: validation passed, dashboard upload skipped, comment formatter failed on missing performance-upload.json.
https://github.com/NathanDrake2406/vinext/actions/runs/28584440974/job/84752398100 Confirms workflow_run publisher checks out the fork default branch, so branch-only fork PRs cannot test the trusted publisher fix before it lands on main.

Fork pull request benchmark publishing treats a missing dashboard ingest secret as a skipped upload, but the PR comment formatter still required the dashboard response file. That makes fork CI fail after validation whenever a PR benchmark run has paired samples but no COMPAT_INGEST_SECRET.

Write an explicit skipped upload response when the secret is unavailable, and render the PR comment from the validated paired benchmark artifact when dashboard comparison data is absent. This keeps upstream dashboard comments unchanged while allowing fork PRs to post local benchmark comparisons.
Fork benchmark publisher failures happen when the trusted publisher tries to render a PR comment after the dashboard upload skipped without producing performance-upload.json.

Update the formatter regression test to omit the upload response file entirely so it covers that ENOENT path directly.
@NathanDrake2406 NathanDrake2406 changed the title fix(perf): render fork benchmark comments without dashboard uploads fix(benchmark): render fork benchmark comments without dashboard uploads Jul 2, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jul 2, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2485
npm i https://pkg.pr.new/vinext@2485

commit: bfcb65e

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared bfcb65e against base 5471b41 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 126.4 KB 126.4 KB ⚫ -0.0%
Client entry size (gzip) vinext 120.5 KB 120.5 KB ⚫ -0.0%
Dev server cold start vinext 2.61 s 2.58 s ⚫ -1.1%
Production build time vinext 3.26 s 3.24 s ⚫ -0.5%
RSC entry closure size (gzip) vinext 98.0 KB 98.0 KB ⚫ -0.0%
Server bundle size (gzip) vinext 164.5 KB 164.5 KB ⚫ -0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx

Copy link
Copy Markdown
Member

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the fork-benchmark-comment change. The core logic is sound and well-tested: the uploader now writes an explicit skipped-upload marker, and the formatter falls back to localComparison(resultBenchmarks) when no dashboard comparison is present. I verified the two new tests pass and traced the workflow wiring in .github/workflows/perf-publish.yml (the response path always exists in CI, and readUploadResponse's ENOENT branch is a reasonable extra safety net).

The upstream dashboard path is preserved: when uploadedComparison is truthy, comparisonSource === uploadedComparison, so line 36's comparisonSource.baseline and line 37's measurement mapping behave exactly as before, and dashboardUrl still links to the dashboard.

A few minor, non-blocking observations below. None block merge.

Comment thread benchmarks/perf/format-pr-comment.mjs Outdated
Comment thread benchmarks/perf/format-pr-comment.mjs
Comment thread benchmarks/perf/format-pr-comment.mjs
@ask-bonk

ask-bonk Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review posted successfully. Summary of my findings on PR #2485:

Verdict: Approve with minor notes — none blocking.

The change correctly solves the fork-PR problem: the uploader writes an explicit { uploaded: false, reason: "missing_secret" } marker instead of leaving the response file absent, and the formatter derives paired comparison rows locally when no dashboard comparison is present. I verified:

  • The two new tests pass (skipped upload response, renders paired PR comments).
  • The upstream dashboard path is fully preserved — when uploadedComparison is truthy, comparisonSource === uploadedComparison, so the baseline handling, measurement mapping, and dashboardUrl are unchanged.
  • The workflow wiring (perf-publish.yml) always creates the response file, so readUploadResponse's ENOENT branch is just a safety net.

Three minor observations posted as inline comments:

  1. localComparison baseline measuredAt uses the head commit's timestamp (consistent with the pre-existing paired path at line 34, and not surfaced in the comment — just flagged).
  2. The uploaded/reason fields are written but never read; the only discriminant is response.comparison. Suggested a clarifying comment.
  3. An unlikely edge case: if a local comparison ever held unpaired-only benchmarks, the summary counts could contradict the per-row Current only labels. Not reachable for fork PRs (which carry paired samples), so informational only.

github run

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.

2 participants