Add PR bundle-size comments workflow#27527
Open
ChumpChief wants to merge 50 commits into
Open
Conversation
This is the CI-side counterpart to `check bundleSize`: it takes two commit SHAs, downloads both ADO bundle-size artifacts, and emits the per-package, per-bundle diff as JSON. Intended for the upcoming PR comment workflow; `check bundleSize` remains the inner-dev-loop entry point that compares locally-collected artifacts against an ADO baseline. Shared ADO pipeline constants are lifted into `pipelineConstants.ts` so both commands stay in sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Triggers on pull_request_target, checks out the PR's base branch (never PR HEAD), fetches the PR head as a git object to compute merge-base, installs build-tools, and runs `flub report comparePipelineBundleArtifacts` to emit a JSON bundle-size comparison for the PR. No commenting, labeling, or check publishing yet — those land in follow-up commits. Single-workflow design (not the repo's pr-check/X-reporter split) since this workflow never executes PR-authored code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Move the header comment block below the `name:` field to match the changeset-reporter / pr-check-changeset style - Drop references to development history (skeleton commit, follow-up commits) so the comment describes durable behavior - Drop the `next` branch from the trigger filter (no longer relevant) - Sync the release-notes links to match the actually-pinned SHAs: actions/checkout v6.0.0 -> v6.0.2, pnpm/action-setup v4.2.0 -> v5.0.0, actions/setup-node v6.0.0 -> v6.3.0 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Several workflow files had `# release notes:` URLs that pointed to an older release tag than the SHA they actually pinned, making it hard to read what the pinned version contains. Resolve each pinned SHA to its matching release tag and update the link: - actions/checkout SHA de0fac2e... -> v6.0.2 (was v6.0.0) - pnpm/action-setup SHA fc06bc12... -> v5.0.0 (was v4.2.0) - actions/setup-node SHA 53b83947... -> v6.3.0 (was v6.0.0) devcontainer-prebuild-check.yml is intentionally left alone — it pins an older actions/checkout SHA (1af3b93b...) that actually does correspond to v6.0.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-runs the bundle-size comparison when ADO publishes the `Build - Client bundle size artifacts` check on a commit. ADO publishes checks but not commit statuses, so check_run is the only viable rerun trigger. Split into two jobs: - route: handles pull_request_target directly; for check_run events, maps the check's SHA back to affected open PRs by either PR-head SHA match or merge-base match. - compare: fans out via strategy.matrix over route's output, runs the per-PR comparison (same flow as before, parameterized on the matrix entry instead of github.event.pull_request). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The route job hard-coded `--base main` on `gh pr list` and computed
merge-base against main's tip after checking out main, which would have
silently skipped release-branch PRs and produced wrong merge-bases for
any PR not targeting main.
Drop the checkout entirely (no local git workspace is needed any more),
drop the `--base main` filter, and use `gh api .../compare/{base}...{head}`
to compute merge-base server-side against each PR's actual base ref.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two branches of the route job's collect step were producing the same shape of `prs=...` GITHUB_OUTPUT line but via different tools: printf for the pull_request_target branch, jq for the check_run branch. The printf version skipped JSON escaping and emitted the PR number as a printf %d, which would have desync'd from the check_run path if anything ever needed escaping or if the number's JSON type drifted. Use jq -n --argjson/--arg in both branches so the output shape is generated by the same code path with proper escaping in both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`route` was opaque jargon (borrowed from web framework routers); the job's actual function is to identify which PRs the incoming event should fan out to. Rename consistently across the job ID, the `needs:` dependency, the `needs.<id>.outputs.prs` references, and the top-of-file comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, the compare job re-derived the merge-base locally via `actions/checkout` + `git fetch refs/pull/N/head` + `git merge-base`. Three race windows: a normal push to the PR could give compare a stale SHA; a force-push could leave the recorded SHA unreachable; main moving under the PR could change the locally-computed merge-base out from under the SHA that identify-targets evaluated against. Compute the merge-base once in identify-targets via gh's compare API and pass it through the matrix as `mergeBase`. compare then only needs the build-tools source to build flub and forwards the SHAs straight through. No PR-head fetch, no local merge-base, no full-history checkout. Also note inside the parenthetical why PR HEAD is never checked out (PR-authored code isn't trusted under pull_request_target). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Batched correctness/robustness fixes from the rubber-duck review: - Add concurrency: group keyed on PR number (or check-SHA), cancel-in-progress true, so rapid synchronizes / repeated check_run completions don't race for the sticky comment. - Collapse compute_mb's "null" output (from GitHub returning success with merge_base_commit: null on disjoint history) into the same empty-string failure shape as a real API error, and drop PRs from AFFECTED when MB is empty so flub never gets called with --base "" / --base null. - Switch the check_run branch's matrix-entry construction from raw shell interpolation into a jq filter to the same --argjson/--arg form the pull_request_target branch uses, so refs containing quote/backslash chars no longer break the filter. - Replace `while ... done < <(...)` with a here-string read of a captured variable so `set -euo pipefail` actually catches producer-pipeline failures (gh CLI errors, jq parse errors). - Bump `gh pr list --limit` from 200 to 500 and warn on cap-hits, so PRs beyond the prior cap are no longer silently dropped. - Filter check_run-derived candidates to PRs whose baseRef matches the pull_request_target trigger whitelist (main + release/...), so the two trigger surfaces agree on which PRs are in scope. - Add `set -euo pipefail` to the compare step's run block so a failing `flub` exit no longer gets masked by `tee`'s success. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Smaller-but-real review cleanups:
- Drop the dead `this.log("Compared base=... head=...")` line from
comparePipelineBundleArtifacts.ts — oclif suppresses `log` under
`--json`, which is the only invocation that path is meant to
receive, so the line is invisible in CI and just noisy
preamble in any non-JSON local debug run.
- Reuse the newly-introduced `fluidframeworkAdoOrgUrl` constant in
codeCoverage.ts where the same URL was inlined twice as a literal.
- Fix the pipelineConstants.ts JSDoc and comparePipelineBundleArtifacts.ts
command description to spell the ADO pipeline name with capital
`Client`, matching both the live ADO check name and the workflow's
exact-string `check_run` filter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Investigation found that the public ADO project's bundle-publishing pipelines have diverged based on commit type: - Main commits: `Build - Client bundle size artifacts` (def 48) is the only public-project pipeline that publishes `bundleAnalyzerJson`. The public-project `Build - client packages` (def 11) stopped running on main; the version that fires on main lives in the internal ADO project and isn't anonymously accessible. - PR commits (fork or not): all checks fire from the public project, including `Build - client packages` (def 11) which publishes `bundleAnalyzerJson` for the PR head. So the workflow needs two pipeline definitions, one per side, both anonymous-accessible from the public project. Add a second constant `bundleSizeArtifactsPrPipeline` (def 11) alongside the renamed `bundleSizeArtifactsBaselinePipeline` (def 48). `check bundleSize` keeps using the baseline pipeline as before (inner-dev-loop case); `report comparePipelineBundleArtifacts` now uses each side's appropriate pipeline. Def 11's PR builds set `sourceVersion` to the GitHub-generated test-merge SHA, not the actual PR HEAD SHA — the actual PR HEAD lives in `triggerInfo['pr.sourceSha']`. Extend `findBuildIdForCommit` to match either field so the same lookup works for both main builds (def 48, no triggerInfo) and PR builds (def 11, triggerInfo present). Workflow's check_run `if:` filter now matches either check name. End-to-end verified: `flub report comparePipelineBundleArtifacts --base <recent-main-sha> --head <recent-pr-head-sha> --json` returns real bundle-size deltas. `flub check bundleSize` still works against the baseline pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`pull_request_target` events can't actually run the comparison because the PR-side ADO build will have just kicked off — only check_run completions signal that an artifact is available. Drop the pull_request_target branch from `identify-targets`'s script, narrow its `if:` to check_run, and let `compare` auto-skip on pull_request_target via GitHub Actions' default `needs:` dependency behavior. Add an `acknowledge-pr` job that fires on pull_request_target events and currently just logs an "intent" line — the actual marocchino sticky posting step is checked in commented-out, ready to be enabled once the comment-posting wiring lands. Until then the job at least gives the workflow a visible signal per pull_request_target event so the trigger is observably firing. Net: -15 lines in identify-targets's shell script, no more event-type branching, +1 small stub job. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Symmetric with the acknowledge-pr stub: add a logging step that acknowledges where the bundle-size-results sticky would go, and check in the marocchino post step in commented-out form so the intended wiring shape is visible. The marocchino step uses `path:` rather than `message:` because the final comment will be a formatted comparison rendered from the JSON that the prior step writes — whether via a new `flub` subcommand or inline jq, that's a later decision. Also note the `pull-requests: write` permission upgrade the compare job will need, also commented out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Print the base SHA, head SHA, and pretty-printed comparison JSON in the stub step so action logs show what the (eventual) sticky would contain. Easier to eyeball whether the comparison ran correctly without the comment-posting wiring being live. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the previous "cat the JSON" stub with a jq-based renderer that produces the actual markdown body the sticky comment would post: per-package sections, bundle-name + parsed/gzip delta lines with +/- sign formatting, "added" / "removed" labels for one-sided bundles, and "No bundle-size changes." when nothing changed. The renderer writes to bundle-comparison.md (which the commented-out marocchino step is already set up to consume via `path:`) and also echoes the body to the action log so we can iterate on the format by reading workflow runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Once this workflow is on main, exercising it organically requires
waiting for ADO check_runs to fire and routing through identify-targets.
That's slow to iterate on. Add a workflow_dispatch trigger with a
single `pr_number` input so a maintainer can fire the compare flow
against a chosen PR on demand.
Dispatch routes through a new dispatch-target job (parallel to
identify-targets) that resolves the PR number into the same
{number, head, baseRef, mergeBase} matrix-entry shape. compare's
needs become [identify-targets, dispatch-target] with an `if:` that
runs when either upstream succeeded; the matrix expression uses a
short-circuit `||` to pick whichever produced output.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the workflow is invoked via `workflow_dispatch` from a fork (e.g. ChumpChief/FluidFramework) but the maintainer wants to exercise the compare flow against a real microsoft/FluidFramework PR, the default `gh pr view <num>` resolves to the fork's PR space — which doesn't contain the upstream PR — and fails. Add an optional `repo` input (default: current repo) and thread it through `gh pr view --repo` and `gh api repos/<repo>/compare/...`. Maintainers can now dispatch from their fork with `repo: microsoft/FluidFramework` to test against an upstream PR without first having to merge this workflow to the upstream main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`workflow_dispatch.inputs.<x>.default` does not allow ${{ github.* }}
expressions — GitHub Actions rejects the workflow file at load time
with `Unrecognized named-value: 'github'`. Drop the default and have
the dispatch-target script fall back to $GITHUB_REPOSITORY when the
input is blank.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The compare job was checking out matrix.pr.baseRef (= main) to source build-tools. That fails for workflow_dispatch testing on a fork: the fork's main doesn't have the workflow's new flub commands yet, and running flub from stale build-tools surfaces as `report comparePipelineBundleArtifacts is not a flub command`. Use the workflow's own ref (github.ref_name) on workflow_dispatch events, so the dispatched ref's build-tools matches the workflow file that's being run. For organic check_run / pull_request_target events, keep using matrix.pr.baseRef — production landings have the workflow file on main / release branches alongside the matching build-tools. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In --json mode, oclif catches thrown Errors and serializes them as
`{"error":{}}` to stdout — message lost — and writes nothing to stderr.
When the compare step fails the action log was therefore useless for
debugging.
On a non-zero exit, re-run flub without --json so the human-readable
error message ends up in the action log. The re-run is cheap because
the failure case here (build not yet completed, in-progress build,
no build found, …) errors during the ADO build lookup, before any
artifact download.
Follow-up: fix at the oclif / command level so the JSON path emits
the message too (adjacent to AB#74372 — BaseCommand.error() losing
under --quiet, this is the same shape of bug under --json).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`name: pr-bundle-size-report` matched the file slug, so GitHub showed the file path in the Actions UI rather than a readable title. Rename to `"PR Bundle Size Comments"` so once the workflow lands on the default branch, runs surface with that name everywhere (web UI, gh output, status badges). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop manual-test scaffolding and rephrase as a deliberate design, so the only remaining work before flipping comment-posting on is a small "uncomment marocchino + write permissions" change in two jobs. - Drop workflow_dispatch trigger + dispatch-target job; compare now only takes input from identify-targets. - Drop the event-type conditional on compare's checkout ref — just matrix.pr.baseRef. - Drop "(stub)" suffixes from step names. - Symmetrize acknowledge-pr with compare: both render their would-be comment bodies to a file (acknowledge.md / bundle-comparison.md) and echo them to the action log. Both commented-out marocchino steps use path: to consume the rendered file. - Comment out acknowledge-pr's `pull-requests: write` permissions to match compare. Enabling comment posting means uncommenting both the marocchino step AND the permissions block in each job — kept consistent across both. - Reword the --json re-run fallback as a deliberate workaround for an upstream oclif bug (no implied fix). - Rewrite the top-of-file Triggers block as production design (drop "kept as a trigger so a future…" framing). - Add a short one-liner above each commented marocchino step: "Uncomment to enable comment posting after bake-in." 35 added, 93 removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The upstream bug now has a tracking issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The acknowledge sticky was firing on pull_request_target without any
signal of whether the PR actually triggers a Build - client packages
run. Server-only and other non-client PRs would receive a "Waiting for
the ADO bundle-size build to complete…" comment that the build will
never complete, and would also fail at the compare step when the
baseline check fires.
Use ADO's own check publication as the source of truth instead.
GitHub publishes a check_run.created event when ADO queues a check
(verified: started_at to completed_at runs ~34 minutes on a typical
build). Listen for both lifecycle events:
check_run.created on `Build - client packages` -> acknowledge-build
posts the initial "pending" sticky for the matching
PR. Non-client PRs never get a check, so they never
get a sticky — exactly the desired behavior.
check_run.completed (success) on either bundle-publishing pipeline ->
identify-targets + compare matrix (unchanged).
Drop the pull_request_target trigger entirely. acknowledge-pr is
replaced by acknowledge-build, which resolves the affected PR via
head-SHA match against `gh pr list`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Split identify-targets into two jobs, one per check_run.name. Both still run the same enumeration logic in the original split, but the structure is now ready for surgical strategy changes per trigger. Then apply the obvious win: for PR-head events (both acknowledge-build on `Build - client packages` created, and identify-from-pr-build on its completion), use `gh pr list --search "head-sha:<SHA>"` to find the one matching PR server-side, instead of iterating up to 500 open PRs and filtering client-side. This is 1 API call instead of as many as 500. The baseline path (identify-from-baseline-build) still enumerates, because a main commit can be the merge-base of multiple PRs and we'd need an explicit per-PR merge-base check regardless. Optimization for that path is tracked as a follow-up. Note: GitHub's check_run object has a `pull_requests` field that *could* tell us this directly, but Azure Pipelines' GitHub App doesn't populate it (verified empty for both fork and non-fork PRs in this repo). So we have to look the PR up ourselves; the `head-sha:` search qualifier is the cleanest server-side way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`github.run_id` was the fallback for workflow_dispatch when the workflow had three triggers. After the redesign the only trigger is check_run, where check_run.head_sha is always populated, so the fallback is dead code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The filter was a "scope safety net" to keep the workflow consistent with the original `pull_request_target.branches: [main, "release/**/*"]` restriction. Now that pull_request_target is gone, the safety net buys us very little: most PRs target main or release/* anyway, and PRs against other bases would just naturally fail downstream when the ADO artifact lookup misses (which is already the gap we're tracking as follow-up work for non-client PRs). In the baseline path the filter was also redundant — `Build - Client bundle size artifacts` only runs on main / release / lts, so the SHA is always on one of those branches; a PR against `feature/foo` would have its merge-base computed against `feature/foo`, not against the check SHA, so it could never match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The display name is "PR Bundle Size Comments" but the file was pr-bundle-size-report.yml — a leftover from when this was framed as a "report" rather than a sticky-comment workflow. Rename the file to pr-bundle-size-comments.yml so the file slug, the display name, and the actual purpose all agree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
It's purely informational — the SHA didn't match an open PR head, which is a normal expected case (e.g. PR was closed, SHA isn't on a PR). Not an error or warning, just stdout narration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Matches acknowledge-build's pattern. Bare `.[0]` on `[]` emits the literal string "null", which we then had to special-case; switching to `.[0] // empty` makes the empty-result case produce no output, so a plain `-z $PR_JSON` check is sufficient. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`2>/dev/null` was cosmetic — `|| true` already kept the script going on non-zero exit. Hiding gh's error message just meant rate-limit hits, API outages, or auth failures would show up only as repeated "Skipping PR #N: merge-base lookup failed" lines with no root cause. Drop the redirect so the actual HTTP/network error text shows up in the log alongside each failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `--jq '.merge_base_commit.sha' || true` pattern silently let HTTP failures through. On 404 / rate limit / transient 5xx, gh writes the raw error response body to stdout (the --jq filter is bypassed); the old code captured that body into `mb` as if it were a SHA. Downstream saw e.g. `mergeBase: <404 JSON body>` and forwarded it to flub, which would later fail with a misleading error. Gate on the gh exit code instead: if gh exited zero, `mb` holds the SHA (or empty via `// empty` on disjoint history); if gh exited non-zero, the function returns empty and the caller hits the existing "skip this PR" path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
compute_mb now lets gh's exit code flow through unchanged. `// empty` still handles the legitimate "no shared history" case at the jq layer (stdout becomes empty on disjoint history while exit code stays 0). Callers can now tell whether they got "no shared history" (legitimate state, workflow stays green) or "HTTP/network/rate-limit failure" (actionable problem). identify-from-pr-build: a gh failure is fatal — the job had exactly one PR to handle and we lost the only signal it could produce. `exit 1` so the workflow run goes red. Disjoint history stays soft: log + emit no matrix entry, exit 0. identify-from-baseline-build: a gh failure on one PR shouldn't abort the whole scan — log + continue. Disjoint history naturally falls through the match check; we keep a safety guard on `[ -n "$MB" ]` for the rare edge case where PR_HEAD == SHA on a PR with disjoint history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same triggers, concurrency, job structure, and matrix shape. Octokit's github.paginate also closes the --limit 500 silent-truncation risk in identify-from-baseline-build. The compare job's checkout / pnpm setup / flub invocation stay as bash since they're inherently shell-bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace `commit: string` with `match: { kind: "commit" | "prHead", sha: string }`
so callers say which field they're keying on instead of relying on a hidden `||`
in the filter. Error messages also identify the kind, so a missing PR build no
longer says "No build found for commit ...".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tighten tsdoc and inline comments to keep what's load-bearing (Cancelling carve-out, scan-all-matches rationale, triggerInfo cast) and drop preamble and details that are inferrable from the code or duplicated in BuildMatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reframe the trigger-section bullets to acknowledge that the current pipeline set is contingent on today's bundle-artifact producers. Rewrite the concurrency block's comment to lead with the goal (`completed` wins over `created`) and call out where cross-SHA staleness is actually handled. Add an inline note in identify-from-pr-build pointing back to its role as the cross-SHA staleness guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the (commented-out) sticky-post step's gate from re-checking
find_pr's output to `steps.render.outcome == 'success'` — so the chain
reads as "post if render ran" and tracks any future change to render's
conditions automatically.
Reframe the acknowledge-build job comment to lead with the *why* of the
filter ("client packages are the only place we measure bundle size
today"). Add an honest framing of the same-head-SHA limitation to both
identify-from-pr-build and acknowledge-build (two open PRs can share a
head SHA; only the first matched gets a sticky).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch acknowledge-build's PR-resolution step from REST search to GraphQL so it also returns headRefOid and baseRefName, then call compareCommits to get the merge-base. Render both SHAs in the pending sticky's body so it matches the results sticky's layout — and users can verify the build context before results arrive. Normalize the three merge-base lookups to `?? undefined` (vs `?? null`) and trim the "because this job had exactly one PR to handle" qualifier on the API-failure comments — the throw is right there in the code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the nested `if (pr) { ... }` block with early-exit guards for
the no-PR and no-merge-base cases. The happy path now drops out the
bottom without an extra indentation level.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove the `pr.head.sha === sha` arm of the baseline-build match. Whenever it would fire, the merge-base lookup already returns the same SHA (so `mb === sha` covers it), or the PR's HEAD lives on a different ref where comparing X against X is a zero-delta no-op. Drop the `mb &&` guard too — `mb === sha` already excludes `undefined`. Also drop the stale comment about a `--limit 500` silent-truncation risk from a former bash version; the current paginate() version doesn't carry that risk and the bash variant is long gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `# release notes: ...` line for the commented-out marocchino step sat at the step-list level, breaking the file's convention. For multi-property steps (`name:`/`id:`/`if:` before `uses:`) the release-notes comment goes inside the step body, right above `uses:`. Move it into position so an uncomment-by-strip-# produces a properly nested annotation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add `(tracked: AB#75349)` to the four "Uncomment to enable comment posting after bake-in" comments so a reader can find the work-item that owns the eventual flip. Reword the compare job's `needs:` comment from "Either upstream produces the matrix" to "One of the identify-* jobs produces..." — clearer at a glance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`set +e` is already active at this point, so a non-zero exit from the re-run can't kill the script — execution falls through to `exit "$EC"` either way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The comparison JSON omits absent sides via spread, so after JSON.parse the fields are `undefined`, never `null` — the loose-equality idiom isn't earning anything. Strict undefined checks read closer to the actual contract and match the project's `?? undefined` preference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Render changed bundles as `parsed 123 → 133 (+10), gzip 55 → 60 (+5)` instead of just the delta — useful for judging proportional impact at a glance. Use → over -> for the transition arrow. Apply consistently to the GH workflow renderer and the flub local formatter so PR comments and local CLI output match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ADO org URL isn't bundle-size-specific — codeCoverage.ts importing from `library/bundleSize` purely for it was an awkward cross-namespace dependency. Move to `library/azureDevops/constants.ts` alongside `IAzureDevopsBuildCoverageConstants` (its semantic home) and update all three consumers to import from there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (723 lines, 18 files), I've queued these reviewers:
How this works
|
Contributor
🔭 PR Review Fleet ReportNote This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement. Verdict: 0 Spicy, 0 Pungent, 1 Smelly Findings
|
The bulleted continuation lines were indented under the bullet, which the eslint `jsdoc/check-indentation` rule rejects. Collapse each bullet onto a single line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated artifact via `oclif readme`. Missed in the original commit; CI flags it as stale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a new GitHub Actions workflow (
pr-bundle-size-comments.yml) that listens for ADOcheck_runevents on the bundle-publishing pipelines, runs a per-PR bundle-size comparison via flub, and (eventually) posts the result as a sticky PR comment.The workflow ships in bake-in mode: the four sticky-comment post steps are commented out. After merge, the workflow runs end-to-end on every PR-build event, logs the would-be comment body in the action log, and writes nothing back to the PR. Turning on actual comment posting is a follow-up, tracked in AB#75349.
Supporting changes:
flub report comparePipelineBundleArtifactsthat drives the actual comparison: fetches both sides' bundle artifacts from ADO, runscompareJsonReportsByPackage, prints JSON or a human-readable summary.BuildMatchrefactor inlibrary/azureDevops/getArtifactForCommit.tsso callers explicitly state whether they're looking up a build bycommit(main-side) orprHead(PR-side, viatriggerInfo['pr.sourceSha']sincesourceVersionis the GitHub-generated test-merge SHA).fluidframeworkAdoOrgUrlinlibrary/azureDevops/constants.tsand use it from both bundleSize commands andcodeCoverage.ts(which previously had the literal URL inlined twice).# release notescomments in other.github/workflows/*files. The comments claimed versions that no longer matched the pinned SHAs, which had been bumped without the URLs being updated.Resolves AB#74457.
Reviewer Guidance
The review process is outlined on this wiki page.
Start at
.github/workflows/pr-bundle-size-comments.yml. The header comment explains the trigger model (check_run created/completed lifecycle), the concurrency strategy (within-SHA dedupe + cross-SHA staleness handled separately in the identify-* jobs), and the bake-in posture.