[ci] Run performance benchmarks on demand

[atomiks]

Render counts are the only worthwhile signal in the benchmark suite because they're deterministic, so any change is meaningful. The timing numbers are noise on the other hand: the baseline and head currently run on different machines under different conditions, so deltas mostly measure the environment. And the full suite takes long enough that it blocks the rest of CI on every relevant PR.

Previously the timing suite ran on every push, measuring each side once in a fixed base-then-head order — expensive, and runner drift systematically biased the head result. Now each PR gets a cheap, deterministic render-count diff automatically, while the full suite runs only on /benchmark, measuring both sides twice in counterbalanced ABBA order so drift cancels and nondeterministic benchmarks fail loudly.

Changes

• PRs get an automatic Render counts check that compares the merge base against the PR head and reports only render-count increases and decreases.
• The full timing suite moves behind an explicit /benchmark comment from a maintainer. It runs current master and the exact PR head in ABBA order (master, PR, PR, master) on a single CircleCI runner, so both sides are measured on the same machine under the same conditions in one run.
• The full run updates the existing consolidated MUI performance comment with timing, paint, and render results.
• Fork PR collection stays unprivileged; a separate workflow validates its output before publishing the check.

Since the full suite no longer gates every PR, it's also free to grow — more benchmarks and more samples are fine when the run is on demand.

[pkg-pr-new]

• base-ui-tanstack-start

• vite-css-base-ui-example

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/react@5403
  

  pnpm add https://pkg.pr.new/mui/base-ui/@base-ui/utils@5403
  

commit: 9526f59

[code-infra-dashboard]

Bundle size

Bundle	Parsed size	Gzip size
@base-ui/react	0B(0.00%)	0B(0.00%)

Details of bundle changes

---

Check out the code infra dashboard for more information about this PR.

[netlify]

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	9526f59
🔍 Latest deploy log	https://app.netlify.com/projects/base-ui/deploys/6a712ebce1ab4e000822bafa
😎 Deploy Preview	https://deploy-preview-5403--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[atomiks]

@claude review

[github-actions]

PR review

Nothing here touches library runtime code, so there is no user-facing regression risk — but the new machinery has one deterministic misbehaviour (the publish workflow treats a routine push as artifact tampering) plus several load-bearing assumptions about @mui/internal-benchmark that this repo never demonstrates. @mui/internal-benchmark is not installed in the review environment and there is no network access, so findings 2, 3, 4 and 7 name the mechanism and what would confirm it rather than asserting the outcome; each of them silently or expensively breaks the new feature if the assumption is wrong, and all four are cheap to check before merge. Nothing is merge-blocking.

Bugs (13)

1. 🟠 The trust pin runs before the staleness check, so a normal push fails the publish workflow with a tampering message

Location: .github/workflows/render-counts-publish.yml:71

if (pullRequest.head.sha !== context.payload.workflow_run.head_sha) {
  throw new Error('The collector artifact does not describe the run that produced it.');
}
if (pullRequest.state !== 'open' || pullRequest.head.sha !== collectorContext.headSha) {
  core.notice('The pull request is closed or has advanced past this collector run.');
  return;
}

The two checks are in the wrong order. pulls.get returns the PR's current head, so as soon as the author pushes again after a collector run completes, pullRequest.head.sha is the new commit while workflow_run.head_sha is still the old one — and the first check throws. The "has advanced past this collector run" branch it was written for is unreachable for the advance case: the second check can only fire when the artifact's headSha was forged, or when the PR is closed at exactly its collected head.

The severities are also inverted: a benign race produces a red workflow run accusing the artifact of not describing its own run, while a genuinely forged headSha gets the quiet core.notice + return.

Failure scenario: Author pushes sha2 while the collector for sha1 is finishing (the collector's cancel-in-progress does not help once it has already completed). "Publish render counts" fails with The collector artifact does not describe the run that produced it. — on every PR where that timing happens.

Fix: Pin trust on the artifact, then treat divergence from the live PR as a skip:

if (collectorContext.headSha !== context.payload.workflow_run.head_sha) {
  throw new Error('The collector artifact does not describe the run that produced it.');
}
if (pullRequest.state !== 'open' || pullRequest.head.sha !== collectorContext.headSha) {
  core.notice('The pull request is closed or has advanced past this collector run.');
  return;
}

This is equally safe — an artifact claiming a victim prNumber still fails the first check, since workflow_run.head_sha is not attacker-chosen.

2. 🟠 The whole ABBA comparison rests on BENCHMARK_OUTPUT_PATH, and the failure only surfaces after four full suite runs

Location: .circleci/config.yml:362

CIRCLE_PR_NUMBER="$BENCHMARK_PR_NUMBER" \
  BENCHMARK_BRANCH=master \
  BENCHMARK_OUTPUT_PATH=$RESULTS/master-1.json \
  pnpm --dir /tmp/benchmark-master test:benchmark

BENCHMARK_RENDER_COUNTS is not set in this job, so vitest.config.ts leaves the package's own reporters in place and the new renderCountReporter.ts — the only thing in this repo that reads BENCHMARK_OUTPUT_PATH — never runs. The four slot files therefore depend entirely on @mui/internal-benchmark's default reporter honouring that variable. The deleted BENCHMARK_BASELINE_PATH=/tmp/base-worktree/test/performance/benchmarks/results.json shows the old behaviour was a fixed <cwd>/benchmarks/results.json, and renderCountReporter.ts:34 uses exactly that as its fallback.

Failure scenario: If the package ignores the variable, master-1.json/master-2.json never exist (both master runs overwrite the same default file), and the job dies on aggregateBenchmarks.mts with an ENOENT — after two installs, two release:builds and four full benchmark suites, i.e. close to an hour of a maintainer's on-demand run.

Fix: Confirm the variable against the package's reporter source. Either way, add a cheap fail-fast right after the first run so a misconfiguration costs minutes instead of an hour:

test -s "$RESULTS/master-1.json" || { echo "BENCHMARK_OUTPUT_PATH was not honoured." >&2; exit 1; }

If it is unsupported, copy <worktree>/test/performance/benchmarks/results.json into the slot file after each run instead.

3. 🟠 optimizeDeps is replaced wholesale, and render-count mode silently no-ops when config.test is unset

Location: test/performance/vitest.config.ts:31

config.optimizeDeps = {
  exclude: ['@mui/internal-benchmark'],
  include: ['react-dom/client'],
};

Assignment, not merge — anything createBenchmarkVitestConfig() put in optimizeDeps (include, entries, esbuildOptions) is discarded, and only in render-count mode. The comment two lines above shows the author already knows this config matters for getting react-dom/profiling into the optimizer graph before the single measured iteration, which is exactly the property being clobbered.

The same block is gated on && config.test (line 6). If the base config ever expresses its suite through projects or workspace instead of a top-level test, render-count mode degrades silently: no plugin, no reporter, no output file — the run "succeeds" and the artifact upload fails with "no files found".

Failure scenario: A dependency is discovered on first request during the only measured iteration, Vite full-reloads the page mid-benchmark, and that benchmark's render array comes back truncated. Every PR then gets a bogus new / removed / ±N row, unreproducible outside render-count mode.

Fix: Merge instead of assign, and fail loudly rather than no-op:

if (process.env.BENCHMARK_RENDER_COUNTS === 'true') {
  if (!config.test) {
    throw new Error('Render-count mode needs a top-level `test` config to attach its reporter to.');
  }
  // ...
  config.optimizeDeps = {
    ...config.optimizeDeps,
    exclude: [...(config.optimizeDeps?.exclude ?? []), '@mui/internal-benchmark'],
    include: [...(config.optimizeDeps?.include ?? []), 'react-dom/client'],
  };
}

4. 🟠 Nothing pins the uploaded report's commit identity to the PR head

Location: .circleci/config.yml:362

The pipeline is triggered on branch: 'master' (.github/workflows/benchmark.yml:110), so CircleCI's own CIRCLE_SHA1 is master's tip for all four runs. The job threads CIRCLE_PR_NUMBER and BENCHMARK_BRANCH through explicitly — precisely because an API-triggered pipeline has no PR association — but never a commit SHA, even though $BENCHMARK_SHA and $BASE_SHA are both in hand. Previously the head run executed in the CircleCI checkout at the PR head, so CIRCLE_SHA1 was correct by construction; now both sides run from detached /tmp worktrees.

validateCompatibleMetadata cannot catch this: it only compares run 1 against run 2 of the same side, so identical commitSha values on head and base pass unnoticed, and publishBenchmark.mts uploads without checking either.

Failure scenario: If the package derives commitSha from CIRCLE_SHA1 rather than git rev-parse HEAD in cwd, head-combined.json and its embedded base carry the same master SHA, and the dashboard attributes the PR's timings to an unrelated master commit.

Fix: Set whatever SHA variable the package reads (or CIRCLE_SHA1="$BENCHMARK_SHA" for the two head runs and ="$BASE_SHA" for the two master runs), and add an assertion in publishBenchmark.mts so this class of error can never ship silently:

if (report.base.commitSha === report.commitSha) {
  throw new Error('The head and base reports carry the same commit SHA.');
}

5. 🟠 Pipeline parameters are interpolated into the job environment unquoted

Location: .circleci/config.yml:307

BENCHMARK_SHA: << pipeline.parameters.benchmark-sha >>
BENCHMARK_PR_NUMBER: << pipeline.parameters.benchmark-pr-number >>
BENCHMARK_PR_BRANCH: << pipeline.parameters.benchmark-branch >>

CircleCI substitutes << >> textually before YAML parsing, so the parameter value becomes YAML syntax. With the declared '' defaults these render as bare keys with no value (YAML null) on every pipeline, since job definitions are compiled regardless of the when: filter. More importantly, git allows !, &, {, }, |, >, %, @, # and backticks in branch names, all of which are YAML indicators. The shell-side regex validation at lines 316-326 runs far too late to help — the config has to parse first.

Failure scenario: /benchmark on a PR whose head branch is #5403-fix renders BENCHMARK_PR_BRANCH: #5403-fix, which YAML reads as a comment; a branch starting with & or @ fails config compilation outright. Either way benchmark.yml has already replied "Started the full benchmark for <sha>", so the maintainer waits for a run that never produced results.

Fix: Quote all three interpolations (BENCHMARK_PR_BRANCH: '<< pipeline.parameters.benchmark-branch >>'), and validate pr.head.ref against /^[\w./-]+$/ in benchmark.yml before POSTing.

6. 🟠 /benchmark can fail with no feedback on the pull request

Location: .github/workflows/benchmark.yml:49

const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({

The job grants pull-requests: write specifically so "a rejected request or a missing token is only visible in the Actions tab" cannot happen — but the three github.rest.* calls before the first reply() are unguarded. getCollaboratorPermissionLevel documents a repository Administration permission requirement for fine-grained tokens, and repos.getContent at headSha 404s if the PR deletes .circleci/config.yml. Any rejection makes github-script throw before anything is posted.

Failure scenario: A maintainer comments /benchmark, sees absolutely nothing happen on the PR, and has to go find a red run in the Actions tab — the exact outcome the extra permission was added to avoid.

Fix: Wrap the script body in try { … } catch (error) { await reply(\Could not start the full benchmark: ${error.message}`); core.setFailed(error.message); }. If the permission endpoint does need administration: read, pulls.get(...).data.author_association (OWNER/MEMBER/COLLABORATOR`) is a drop-in that works with the granted scopes.

7. 🟡 Dropping code-infra/install-deps leaves nothing guaranteeing pnpm is on PATH

Location: .circleci/config.yml:344

      - run:
          name: Install both revisions
          command: |
            set -euo pipefail
            pnpm --dir /tmp/benchmark-head install --frozen-lockfile

test_benchmark is now the only job in the file that goes from checkout straight to pnpm without code-infra/install-deps. The globally-set COREPACK_ENABLE_DOWNLOAD_PROMPT: '0' suggests corepack is the delivery mechanism for pnpm; if install-deps is what runs corepack enable, the first install step dies with pnpm: command not found and /benchmark never works at all.

Failure scenario: Every /benchmark invocation fails in its second step.

Fix: Confirm against the pinned orb (code-infra.yml@00cd0db); if the executor image does not ship enabled corepack shims, add an explicit corepack enable right after checkout.

8. 🟡 The render-count comment can be duplicated on long-lived PRs

Location: .github/workflows/render-counts-publish.yml:172

const { data: comments } = await github.rest.issues.listComments({
  ...
  per_page: 100,
});
const previous = comments.find((comment) => comment.body?.startsWith(marker));

Issue comments come back oldest-first with no pagination here, so the marker is only findable while the PR has fewer than 100 comments before the render-count comment was created.

Failure scenario: A PR accumulates 100+ comments and only then starts touching packages/react. previous is undefined on every push, so a fresh render-count comment is appended each time instead of the existing one being updated.

Fix: const comments = await github.paginate(github.rest.issues.listComments, { …, per_page: 100 });.

9. 🟡 The /benchmark acknowledgement has no link, and the CircleCI status lands on master

Location: .github/workflows/benchmark.yml:110

branch: 'master',

`Started the full benchmark for \`${headSha}\`. Results replace the consolidated ` +

The pipeline runs on master, so ci/circleci: Full benchmark is reported as a commit status against master's tip rather than the PR — and a nondeterministic benchmark is a hard failure by design (aggregateBenchmarks.mts:93), so a failed run paints an unrelated merged commit red. Meanwhile the reply comment contains no pipeline URL at all; pipeline.id is only written to the Actions log via core.notice.

Failure scenario: The full benchmark fails on a nondeterministic fixture. The PR shows nothing (the upload never happens), and the maintainer has no link from the acknowledgement to the run that failed.

Fix: Include the pipeline URL in the reply (https://app.circleci.com/pipelines/github/${owner}/${repo}/${pipeline.number}), and either post an explicit commit status against $BENCHMARK_SHA from the job or state in the reply that the CircleCI check appears on master.

10. 🟡 Four full suites share one step, against CircleCI's 10-minute silence timeout

Location: .circleci/config.yml:356

CircleCI's default no_output_timeout is 10 minutes per step. This step now runs four complete benchmark suites back to back at the package's unmodified warmup and sample counts, and the step above it builds the whole monorepo twice. A single long, quiet fixture anywhere in there kills the job with Too long with no output, discarding up to 45 minutes of work.

Failure scenario: The job dies mid-run; store_artifacts (correctly when: always) uploads a partial /tmp/benchmark-results, and there is no signal for which of the four runs was in flight.

Fix: Add no_output_timeout: 30m to the ABBA step and to "Build both revisions".

11. 🟡 Pushes to master no longer upload any benchmark datapoint

Location: .circleci/config.yml:431

      - benchmark_required_status:
          name: 'Benchmark tests'

The old test_benchmark ran in the pipeline workflow on every push, including master, where it skipped the baseline and uploaded the head result with BENCHMARK_UPLOAD: 'true'. BENCHMARK_UPLOAD no longer appears anywhere in the repo, and nothing replaces it on a schedule, so the dashboard's continuous master timeline stops at this commit. The on-demand run uploads a self-contained head/base pair, which is a comparison rather than a timeline entry.

Failure scenario: The MUI performance dashboard's master history goes stale after merge, so there is no longer any long-range trend to notice slow drift against.

Fix: If the master timeline is still wanted, add a scheduled master benchmark workflow (the typescript-next cron in this file is a ready template). If it is intentionally being dropped, say so in the test/README.md section, since the current text does not mention it.

12. 🟡 startTime is pooled with different weights than the mean it accompanies

Location: test/performance/scripts/aggregateBenchmarks.mts:115

startTime:
  (firstRender.startTime * first.iterations + secondRender.startTime * second.iterations) /
  (first.iterations + second.iterations),

combineStats weights actualDuration by outlier-adjusted sample counts (iterations - outliers), but startTime here uses raw iterations.

Failure scenario: A render with outliers: 3 in run 1 and 0 in run 2 over 20 iterations each gets its duration weighted 17:20 and its startTime weighted 20:20. The combined timeline is internally inconsistent, and anything deriving gaps between renders picks up a bias that grows with the outlier rate.

Fix: Hoist the firstUsed/secondUsed computation out of combineStats and reuse it for startTime.

13. ℹ️ benchmark_required_status is the only job invoked without org-global

Location: .circleci/config.yml:431

Every other job in the pipeline workflow carries <<: *default-context. Withholding the context from a stub that runs on every fork PR is defensible hygiene, but if code-infra/mui-node's image is pulled with credentials from that context, the required ci/circleci: Benchmark tests check becomes subject to anonymous registry rate limits — the worst check in the repo to make flaky. Worth confirming against the pinned orb.

Tests (2)

1. 🟠 The render-count diff logic is neither typechecked nor tested

Location: .github/workflows/render-counts-publish.yml:88

Roughly 120 lines of the most intricate new logic in this PR — artifact validation, size and shape limits, the base/head diff, new/removed handling, markdown-cell escaping, title pluralisation, comment upsert — live as a github-script string inside YAML. This PR simultaneously builds the infrastructure for exactly this kind of code: test/performance/scripts/, tsconfig.scripts.json, and test:scripts on node --test, with the two other new scripts split into logic + CLI entrypoint precisely so they can be unit-tested.

Failure scenario: No test pins escapeCell against a fixture name containing a backtick or a pipe, the 0 renders case against row.base ?? '—', or the new/removed rows. Any of them can only be discovered by pushing to a real PR and reading the rendered comment, and a typo is invisible until then because the file is not on any typecheck path.

Fix: Extract to test/performance/scripts/diffRenderCounts.mts exporting readReport(json) and diffRenderCounts(base, head) returning { title, summary, rows }, add diffRenderCounts.test.mts, and reduce the workflow step to reading two files and calling it.

2. 🟠 The nondeterminism guard — the assertion the whole ABBA design advertises — is untested

Location: test/performance/scripts/aggregateBenchmarks.test.mts:60

The two tests cover the happy path and a missing benchmark name. Untested:

• the render-sequence divergence assert (combineEntry, line 93) — "nondeterministic benchmarks fail loudly" is the headline guarantee of the new protocol, and nothing checks that a differing id/phase at some index actually throws, or that a differing renders.length does;
• validateCompatibleMetadata — no test that mismatched commitSha or metricDefinitions are rejected, which is the check that would otherwise catch finding 4's class of error;
• combineStats with non-zero outliers, where the pooling weights stop being equal and finding 12's inconsistency becomes observable;
• differing metric names between runs.

Failure scenario: A refactor inverts or drops the render-sequence assert. Every green test still passes, and the ABBA run silently averages two structurally different render timelines into a plausible-looking result — the one outcome the design exists to prevent.

Fix: Add cases for a mutated renders[0].phase, a shorter renders array, a mismatched commitSha, and a pair with outliers: 3 asserting the outlier-weighted mean.

Simplifications (1)

1. 🟡 publishBenchmark.mts re-implements the dashboard upload contract

Location: test/performance/scripts/publishBenchmark.mts:22

const apiUrl = process.env.CI_REPORT_API_URL ?? 'https://frontend-public.mui.com';

The host, both endpoint paths (/api/ci-reports/upload, /api/ci-reports/sync-pr-comment), the OIDC bearer shape and the skipped response convention are all hardcoded here, duplicating what BENCHMARK_UPLOAD=true used to do inside @mui/internal-benchmark. Aggregation genuinely has to happen outside the package, but the transport does not — and a change on the dashboard side now has to be tracked in this repo as well as the shared one.

Two smaller things nearby: new URL('/api/…', apiUrl) discards any path prefix on CI_REPORT_API_URL, and await syncResponse.json() throws a raw parse error (not the carefully-worded message below it) if the endpoint answers 200 with an empty body.

Failure scenario: The dashboard renames an endpoint or changes the auth header. Every other MUI repo picks it up with a package bump; base-ui's /benchmark fails until someone edits this file.

Fix: Call an upload helper from @mui/internal-benchmark/ciReport if one is exported (the schema already lives there); if not, worth requesting upstream. Meanwhile guard the json() parse.

Docs (1)

1. 🟠 No documented way to reproduce a render-count change locally

Location: test/README.md:170

The new section explains the check, the comment, the rollout constraint and the /benchmark security model thoroughly — but never says how a contributor investigates a result. A PR now gets a check saying "3 additional renders" with a per-benchmark table and no local command to reproduce it, even though one exists and is short:

BENCHMARK_RENDER_COUNTS=true BENCHMARK_OUTPUT_PATH=/tmp/head.json pnpm test:benchmark

The new pnpm --dir test/performance test:scripts, typescript and typescript:fixtures commands are likewise undocumented, so a contributor whose PR fails the "Test benchmark scripts" step in test_static has to read .circleci/config.yml to find out what to run.

One factual slip in the same section: it says the collector runs for PRs that change "React, shared utilities, or the performance fixtures", but the paths filter in render-counts.yml also includes the root package.json and pnpm-lock.yaml.

Failure scenario: A contributor sees a render-count regression on their PR, has no documented way to see it locally, and either pushes speculative commits to iterate through CI or ignores the check.

Fix: Add a short "Running it locally" paragraph with the BENCHMARK_RENDER_COUNTS invocation and the two test/performance scripts, and mention dependency-manifest changes in the trigger list.

Verdict

Approve after nits - no user-facing or component behaviour is touched, but the publish-workflow ordering bug is a guaranteed failure on a routine push, and four load-bearing assumptions about @mui/internal-benchmark (findings 2, 3, 4, 7) are worth confirming before the first real /benchmark run.

---

🤖 Review generated with Claude Code · medium effort · 42 turns · 26m40s · $6.66 · run

[atomiks]

I went through each finding and applied the ones with confirmed current impact or a sufficiently cheap hardening path.

Addressed

• Finding 1: Fixed the stale-run ordering. Artifact identity is now validated against workflow_run.head_sha first; a closed or advanced PR is then skipped without a misleading tampering failure.
• Finding 6: Unexpected GitHub and CircleCI request failures now leave an explanatory PR reply.
• Finding 7: Added code-infra/install-pnpm before the benchmark job invokes pnpm.
• Finding 8: Changed render-count comment lookup to use github.paginate.
• Finding 9 (partial): Added the CircleCI pipeline link to the /benchmark acknowledgement. The trusted pipeline still intentionally runs from master.

Not changed

• Finding 2: Confirmed that the pinned @mui/internal-benchmark reporter supports BENCHMARK_OUTPUT_PATH.
• Finding 3: The current base config provides config.test and has no existing optimizeDeps configuration to preserve. This would only be future-proofing.
• Finding 4: Confirmed that commitSha comes from git rev-parse HEAD in each worktree, so the base and head identities remain distinct.
• Finding 5: CircleCI pipeline parameters are handled as configuration values, not reparsed as arbitrary YAML or shell input.
• Finding 10: Vitest produces progress during the suite; there is no demonstrated ten-minute period without output.
• Finding 11: Discontinuing automatic master timing uploads is intentional. Full timing comparisons now run only on request.
• Finding 12: startTime is derived partly from unfiltered mean gaps upstream, so outlier-adjusted weighting is not clearly more correct.
• Finding 13: The stub uses the public Microsoft Playwright image and does not require org-global.
• Tests, docs, and simplification: These are reasonable possible follow-ups, but they would materially expand this already large workflow PR. The package’s upload helpers are also not publicly exported.

Validation passed for workflow YAML parsing, embedded JavaScript syntax, Prettier, ESLint, and git diff --check.

---

🤖 Follow-up generated with Codex

[mnajdova]

@brijeshb42 can you check if it makes sense for this to be part of the mui-public so it can be reused across other repos too?