fix(ci): publish the bundle-size baseline instead of tracking it in git - #526
fix(ci): publish the bundle-size baseline instead of tracking it in git#526RaananW wants to merge 10 commits into
Conversation
The "PR Validation → Bundle Size" job was constantly red for reasons unrelated to the PR being validated. `validate:bundle-manifest` hard-failed whenever the freshly measured per-scene manifest differed from the one committed at `git HEAD`, compared byte-exactly and including content-hashed chunk filenames. Because a change to any shared module moves bytes in most of the ~230 scenes (#451 touched 200 manifest files, #521 215, #450 189), the moment one shared-code PR merged, every other open PR's committed manifest was stale and its job turned red — curable only by a rebase plus a very slow full local rebuild, and then again on the next merge to master. The manifest is a generated baseline and CI already measures it, so CI now owns it: - `scripts/commit-bundle-manifest.ts` (new) pushes the freshly measured per-scene files back to the PR branch as a bot commit, so the size deltas still land in the diff for review with no author busywork. An Azure PR build checks out the PR *merge* commit, so it commits in a detached worktree at the PR head rather than pushing that merge onto the contributor's branch. It skips fork PRs, refuses to mirror an incomplete build, stops after two consecutive bot commits instead of ping-ponging on a non-deterministic measurement, and warns rather than failing on every error path. - `validate:bundle-manifest` now reports drift instead of failing. `--strict` (`pnpm validate:bundle-manifest:strict`) keeps the old behaviour for local use. - Runtime chunk sets are compared with the Rollup content hash stripped. The hash moves whenever any module in the chunk changes — including from an unrelated PR or a bundler upgrade — so comparing it reported churn carrying no size information. The hash-stripped names still catch a scene starting or stopping to pull in a module. Bundle-size regressions remain gated by the per-scene `maxRawKB` ceilings, which `pnpm build:bundle-scenes` enforces byte-exactly and which fail the job before the auto-commit step runs. That check is deterministic and can never go stale. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR stabilizes CI bundle-size validation by making the per-scene bundle-size manifest baseline CI-owned (auto-refreshed by the Bundle Size job) instead of author-maintained, while keeping size regressions gated by scene-config.json ceilings.
Changes:
- Add a CI script (
commit-bundle-manifest.ts) that pushes refreshed per-scene manifest files back to the PR branch (with safeguards for forks, incomplete builds, and non-converging measurements). - Update
validate-bundle-manifestto report drift by default (exit 0) and add a strict mode that preserves the old hard-fail behavior. - Update docs/tests/pipeline wiring to reflect the new CI-owned baseline workflow and hash-stripped chunk-set comparisons.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/lite/unit/validate-bundle-manifest.test.ts | Adds unit + CLI coverage for drift reporting, strict mode, and hash-stripped chunk comparisons. |
| tests/lite/unit/commit-bundle-manifest.test.ts | Adds end-to-end tests covering the worktree-based commit/push behavior and safety rails. |
| TESTING.md | Documents that CI refreshes the per-scene manifest and how to validate drift locally (report vs strict). |
| scripts/validate-bundle-manifest.ts | Converts validation into report-by-default, adds strict mode, and ignores chunk content hashes when comparing chunk sets. |
| scripts/commit-bundle-manifest.ts | New script to refresh/push per-scene manifest files from CI without pushing the PR merge commit. |
| package.json | Adds validate:bundle-manifest:strict and commit:bundle-manifest scripts. |
| GUIDANCE.md | Updates the workflow guidance to reflect CI-owned manifest refresh and non-gating drift. |
| docs/lite/architecture/38-bundle-size-tooling.md | Updates architecture documentation to describe the CI-owned baseline and the new scripts. |
| azure-pipelines.yml | Wires the pipeline to report drift, then refresh the manifest on the PR branch (continueOnError). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lite Playground - Static SiteBuild 20260804.4 - merge @ 4dd5ad8 |
Measured by CI build 20260804.4. Per-scene bundle sizes are a generated baseline; CI refreshes them so they never go stale when another PR merges first.
Lite Playground - Static SiteBuild 20260804.5 - merge @ 34ae187 |
Lab - Static SiteBuild 20260804.5 - merge @ 34ae187 |
`warn()` interpolated arbitrary text into `##vso[task.logissue ...]`. Two call sites forward raw git stderr, which is routinely multi-line, so a failed push would truncate the annotation and leave the remaining lines to be interpreted as output of their own — including any that begin with `##vso[`. Flatten the annotation to a single bounded line and neutralize embedded `##vso[` on both output streams. The console stream keeps its line breaks so a push failure is still debuggable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Lite Playground - Static SiteBuild 20260804.7 - merge @ 0a589ae |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/validate-bundle-manifest.ts:298
- This log message promises that a CI auto-commit step will refresh the manifest, but in some CI contexts (e.g. fork PRs or missing GITHUB_TOKEN) the refresh step cannot push and the manifest will remain stale. That can mislead readers of the build log and complicate debugging.
Consider wording this as an attempt/conditional refresh rather than a guarantee.
console.log(`Bundle manifest drift detected — the CI auto-commit step will refresh it.\n${detail}`);
scripts/commit-bundle-manifest.ts:138
countLeadingAutocommits()treats any commit subject that starts with the bot subject as an autocommit. That can misclassify a human commit like "chore(bundle): refresh per-scene bundle-size manifest (manual)" and trigger the non-convergence loop guard earlier than intended.
Since this script controls the exact subject it emits, an exact equality check is safer and clearer.
if (!subject.startsWith(COMMIT_SUBJECT)) break;
scripts/commit-bundle-manifest.ts:197
- This warning tells fork-PR authors to run a full
pnpm build:bundle-scenesand commit the entire manifest directory. That’s both heavier than necessary and inconsistent with the updated workflow where the manifest is generally CI-owned.
Since fork PRs are the special case where CI cannot push, it’s clearer to say CI cannot refresh it and that the author should regenerate (filtered scenes is fine) and commit the affected per-scene manifest files if they want the baseline updated in the diff.
warn("Fork pull request — cannot push the refreshed bundle-size manifest. Run 'pnpm build:bundle-scenes' and commit lab/public/bundle/manifest/ yourself.");
Lab - Static SiteBuild 20260804.7 - merge @ 0a589ae |
| | `lab/public/bundle/manifest/<scene>.json` | `pnpm build:bundle-scenes` | Yes | Current/local per-scene runtime measurements — the tracked repository bundle baseline. Commit the regenerated files for any scene whose size or runtime chunks moved. | | ||
| | `lab/public/bundle/manifest.json` | `pnpm build:bundle-scenes` (aggregated from the per-scene files) | No | Generated aggregate of all per-scene files, consumed at runtime. Gitignored — never commit it. | | ||
| | `lab/public/bundle/master-manifest.json` | `pnpm build:bundle-scenes` and `pnpm build:bundle-master-info` | No | Ignored generated aggregate reconstructed from the selected master ref's per-scene files (with a legacy single-file fallback for pre-migration refs). Used as the master baseline. | | ||
| | File | Generated by | Tracked? | Meaning | |
There was a problem hiding this comment.
Leaving a comment here to avoid unwanted merging. This is a change we can discuss!
The per-scene bundle-size manifest is a generated file tracked in git, and until now every PR author was expected to regenerate and commit all ~227 of them. That produced two distinct failures with the same trigger: - red CI, because any merge to master made every other open PR's manifest stale and the validation step rejected it; and - merge conflicts, because two branches rewriting the same ~200 generated files conflict at the git level (PR 523: 169 conflicting paths, 168 of them manifest files, 0 source files). The first can be fixed by having CI regenerate the manifest. The second cannot -- as long as PR branches carry the manifest at all, they will keep colliding on it. Nothing on a PR branch actually needs the manifest: the size gate reads scene-config.json (maxRawKB), and the master baseline is reconstructed from origin/master via git ls-tree, never from the working tree. So the manifest only ever has to be correct on master. Master therefore becomes its single writer: - PR builds measure, enforce ceilings and report drift, but never write the manifest. Manifest files stop appearing in PR diffs, so the conflicts become structurally impossible rather than merely less frequent. - A new master-triggered pipeline refreshes and commits the manifest after each merge. Its path trigger excludes the manifest directory so the bot commit cannot re-trigger it, and the demos and playground pipelines exclude it too so the bot commit does not pay for their deploys. The push is applied through a detached worktree on the freshly fetched tip rather than the build's own checkout, so a merge landing mid-measurement is never discarded, and a rejected push is retried onto the moved tip. Each bot commit records the revision it measured in a Measured-from trailer. The anti-ping-pong guard only counts leading bot commits from that same revision, so overlapping master builds -- which routinely stack bot commits from different revisions -- do not trip a guard meant to catch a non-deterministic measurement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The doc claimed the per-scene split stopped PRs colliding. It does for scene-local changes, but not for the shared-module changes that dominate: those move bytes in ~200 scenes, so two branches still rewrite the same files. Say that plainly, since it is the reason master now owns the files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (4)
azure-pipelines-bundle-manifest.yml:40
- The path filter excludes
lab/public/bundle/manifestas a literal path, which does not reliably exclude files under that directory in Azure Pipelines. That can let the bot’s commit (touchinglab/public/bundle/manifest/*.json) retrigger this pipeline, defeating the “single writer / no self-trigger” safety property.
paths:
exclude:
- lab/public/bundle/manifest
azure-pipelines-demos.yml:16
- This trigger exclusion has the same issue as the bundle-manifest pipeline: excluding
lab/public/bundle/manifestmay not exclude changes to files inside that folder, so the manifest bot commit can still trigger a demos deploy.
exclude:
- lab/public/bundle/manifest
azure-pipelines-playground.yml:29
- This trigger exclusion may not match files under
lab/public/bundle/manifest/unless it’s written as a folder glob. If it doesn’t, the manifest bot commit can still trigger the playground deploy/purge pipeline.
exclude:
- lab/public/bundle/manifest
scripts/validate-bundle-manifest.ts:271
- This error message still instructs contributors to regenerate + commit the per-scene manifests. With the new master-owned baseline design, that guidance is misleading: PR branches should restore the baseline from master, not commit regenerated manifests from the branch.
if (committed === null) {
console.error(`No committed manifest found under ${MANIFEST_DIR_REL_PATH}/ at HEAD.\nRun 'pnpm build:bundle-scenes' and commit the generated per-scene manifest files.`);
process.exit(1);
}
Lite Playground - Static SiteBuild 20260804.12 - merge @ 81d65fe |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
azure-pipelines-playground.yml:29
- Same as the demos pipeline: exclude
lab/public/bundle/manifest/**(recursive) so the manifest-refresh bot commit does not trigger a full playground build + deploy on every merge.
paths:
# The bundle-manifest bot (azure-pipelines-bundle-manifest.yml) pushes a
# commit touching only this directory after every merge. It cannot change
# the playground, so exclude it rather than paying for a full build and
# CDN deploy/purge on each one.
exclude:
- lab/public/bundle/manifest
azure-pipelines-demos.yml:16
- Same as the bundle-manifest pipeline: the trigger path exclusion should exclude files under
lab/public/bundle/manifest/, not just the directory path. Otherwise the manifest-refresh bot commit can still trigger an expensive demos deploy.
paths:
# The bundle-manifest bot (azure-pipelines-bundle-manifest.yml) pushes a
# commit touching only this directory after every merge. Nothing here
# affects the demos, so exclude it rather than paying for a full build
# and CDN deploy on each one.
exclude:
- lab/public/bundle/manifest
azure-pipelines-bundle-manifest.yml:40
- Azure Pipelines path filters are glob-based; excluding
lab/public/bundle/manifestmay not exclude changes under that directory. If the bot commit modifieslab/public/bundle/manifest/<scene>.json, this pipeline can still be retriggered on every refresh commit, defeating the “single writer” loop-safety goal. Use a recursive glob to exclude all files under the directory.
paths:
exclude:
- lab/public/bundle/manifest
Lab - Static SiteBuild 20260804.12 - merge @ 81d65fe |
The `PR Validation → Bundle Size` job was failing on PRs that had nothing to do with bundle size, and the same mechanism was the repo's dominant source of merge conflicts. Both came from one root cause: the baseline was ~227 generated JSON files tracked in git and refreshed by PR authors. Nearly every shared-module change moves bytes in most of the ~230 scenes, so: - as soon as any shared-code PR merged, every other open PR's committed manifest was stale and its Bundle Size job went red — curable only by a rebase plus a very slow full local rebuild, then again on the next merge; - two branches that both regenerated the manifest rewrote the same ~200 files and collided. In PR 523, 168 of the 169 conflicting paths were manifest files and zero were source files. The baseline is now measured once per master build and published to public storage; PR builds and local runs fetch it over HTTPS. Nothing is tracked, so there is no file to go stale and nothing for two branches to collide on. This also keeps CI out of the repository's write path, which matters because master is protected and the CI identity holds no bypass. Safe because the baseline gates nothing: size regressions are caught by the absolute `scene-config.json` ceilings, enforced byte-exactly by `build:bundle-scenes`. A missing or unreachable baseline degrades to "no delta report" and never fails a build. - `bundle-scenes-core.ts`: resolve the baseline from file → HTTPS → git ref (the last for pre-migration refs), with a shape guard so a CDN serving HTML or wrong-shaped JSON is not adopted as a baseline. - `azure-pipelines-bundle-manifest.yml`: master-triggered; enforces ceilings first, then uploads the manifest and purges the CDN. Refuses to publish an empty manifest. - `azure-pipelines.yml`: drop the git baseline fetch and the drift gate. The ceiling checks and the delta PR comment are unchanged. - Delete `validate-bundle-manifest.ts` and `commit-bundle-manifest.ts`; untrack `lab/public/bundle/manifest/`. - Drop the device-dependent write suppression, which existed only to protect tracked files. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 240 out of 241 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
lab/vite.config.ts:244
- This comment still claims a “fresh checkout” can populate the Bundle tab without a full build by synthesizing from per-scene files. After this PR, the per-scene files are also gitignored, so on a fresh checkout neither aggregate nor per-scene manifests exist. Consider rewording to the narrower (still true) case: synthesize only when per-scene files exist but the aggregate hasn’t been generated (e.g. partial/failed build).
azure-pipelines-bundle-manifest.yml:145 - The script says it “refuses to publish an empty baseline”, but
test -sonly checks non-zero file size. A broken build could still produce{}(non-empty file, 0 scenes) and this step would publish it, causing all consumers to treat the baseline as missing (shape guard rejects empty manifests) until the next successful run. Add aSCENES==0guard before uploading.
if ! test -s "$MANIFEST"; then
echo "No aggregate manifest at $MANIFEST — refusing to publish an empty baseline."
exit 1
fi
SCENES="$(node -e 'process.stdout.write(String(Object.keys(require(process.argv[1])).length))' "$MANIFEST")"
echo "Publishing bundle-size baseline for $SCENES scene(s)."
Resolved the 128 modify/delete conflicts by keeping this branch's deletion of lab/public/bundle/manifest/*.json — those files are exactly what this PR stops tracking, and every conflicting path was one of them. Follow-up fixes the merge exposed: * tests/lite/unit/bundle-content-no-f64.test.ts and npe-particle-bundle-content.test.ts read the per-scene manifests, which are now build output rather than tracked source. They self-skip when the bundle has not been built (CI's Unit Tests job runs vitest before any build), and azure-pipelines.yml re-runs both in the Bundle Size job after `pnpm build:bundle-scenes` so coverage is preserved rather than dropped. * readMasterBundleManifestFromRef spawned one `git show` per scene, so the legacy-ref fallback cost ~15s for ~230 files — long enough to time out the new baseline unit tests. Read the blobs with a single `git cat-file --batch` instead (15.5s -> 1.5s, byte-identical result). * README: the per-scene manifests are no longer tracked. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Lite Playground - Static SiteBuild 20260805.15 - merge @ 0ab9381 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 243 out of 244 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/lite/unit/bundle-master-baseline.test.ts:75
expect(promise).resolves.not.toThrow()is not a valid Vitest/Jest assertion becausetoThrow()expects a function, not a resolved value. This will error even when the promise resolves. If the intent is just “promise does not reject”, assert that it resolves (e.g.toBeDefined()), while still allowing anullbaseline.
tests/lite/unit/bundle-content-no-f64.test.ts:86HAS_BUILT_CHUNKSis documented as implyingHAS_MANIFEST, but the boolean expression doesn’t enforce that. If chunk files exist whilemanifest/is missing (e.g. partial cleanup / stale local state), tests guarded byHAS_BUILT_CHUNKSwill still run and then throw whenloadManifest()readsMANIFEST_DIR. MakeHAS_BUILT_CHUNKSdepend onHAS_MANIFEST(or guard the relevant tests with both).
…the playground The baseline was being overlaid onto litePlayground/, so a CI-only artifact was served from the playground's user-facing product domain. Move it to the storage account already used for the per-build Playwright reports and lab sites: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/lite/bundle-baseline/manifest.json Everything else on that host is scoped by $(Build.BuildNumber) and therefore never overwritten; the baseline is deliberately the one stable path, so readers need no build number to find it. That host is not cached, so the post-deploy CDN purge is gone along with the cdnEndpoint/cdnProfile variables. STORAGE_ACCOUNT comes from BabylonJS-Deployment, which the pipeline already uses, so BabylonJS-CI-Infrastructure (TOOLS_STORAGE_ACCOUNT, CDN_PROFILE_TOOLS) and BabylonJS-BrowserStack (never referenced) are dropped — two fewer variable groups to authorize in the ADO UI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`.resolves` already fails the test when the promise rejects, but `.not.toThrow()` is then applied to the resolved value (null) rather than a function, so that half of the assertion checked nothing. Await the call directly — a rejection fails the test on its own, which is exactly what this case is asserting. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 243 out of 244 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/lite/unit/bundle-master-baseline.test.ts:76
await expect(resolveMasterBundleManifest()).resolves.not.toThrow()is not a valid Vitest/Jest pattern:toThrowexpects a function, so this assertion can fail even when the promise resolves successfully. This test should simply await the promise (and then assert on observable side effects likerequestCount).
tests/lite/unit/bundle-content-no-f64.test.ts:81HAS_MANIFESTis true if any JSON exists in the manifest directory, but the manifest assertions assume at leastscene2.jsonexists. If someone runs this after a filtered bundle build that didn’t include scene2, the test will run and fail with a confusing "scene2 missing" error instead of self-skipping as intended.
docs/lite/architecture/38-bundle-size-tooling.md:92- This section says the new baseline pipeline "purges the CDN", but
azure-pipelines-bundle-manifest.ymldoes not actually run a purge step (unlike the playground/demos pipelines). Either add the purge step or adjust this documentation so it doesn’t promise behavior the pipeline doesn’t implement.
`azure-pipelines-bundle-manifest.yml` re-measures on each push to master and
uploads `manifest.json` to the deployment server, then purges the CDN so the new
baseline is visible promptly. It refuses to publish an empty or missing manifest,
so a broken build cannot blank the baseline, and the ceiling check runs first so a
breach can never be published as the new normal.
Lite Playground - Static SiteBuild 20260805.17 - merge @ 0b9464e |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 243 out of 244 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/lite/unit/bundle-content-no-f64.test.ts:86
HAS_BUILT_CHUNKSis documented as implyingHAS_MANIFEST, but the current definition does not includeHAS_MANIFEST. If a developer has stale chunk files underlab/public/bundle/but the manifest directory is missing/cleaned,HAS_BUILT_CHUNKScan be true and tests like${HPM_OFF_SCENE}: no runtime chunk contains the F64 build tagwill callloadManifest()and throw onreaddirSync(MANIFEST_DIR). TieHAS_BUILT_CHUNKStoHAS_MANIFEST(or guard the tests that callloadManifest) to avoid this crash in partial-build states.
Lab - Static SiteBuild 20260805.17 - merge @ 0b9464e |
Problem
The
PR Validation → Bundle Sizejob fails on PRs that have nothing to do with bundle size, and the same mechanism is the repo's dominant source of merge conflicts.Both symptoms have one root cause: the bundle-size baseline was ~227 generated JSON files tracked in git and refreshed by PR authors. Nearly every change to a shared module moves bytes in most of the ~230 scenes, so:
Fix
Stop putting a generated file in git. The baseline is measured once per master build and published to public storage; PR builds and local runs fetch it over HTTPS.
There is no tracked file to go stale, and nothing for two branches to collide on. Both failure modes become structurally impossible rather than merely less frequent.
Publishing rather than pushing to master also keeps CI out of the repository's write path — which matters, because
masteris protected (required review + a ruleset with required status checks) and the CI identity deliberately holds no bypass.Why this is safe
The baseline gates nothing. Every consumer was audited:
bundle-size.spec.ts:139console.warnonly. The real gate is line 158,rawKB <= scene.maxRawKBfromscene-config.json.report-bundle-size-deltas.ts:133POST_BUNDLE_COMMENT=false, exits 0.lab/vite.config.ts:382Size regressions are still gated by the absolute per-scene
maxRawKBceilings, whichbuild:bundle-scenesenforces byte-exactly and which this PR does not touch. A missing or unreachable baseline degrades to "no delta report" and can never fail a build.Reporting is unchanged
The PR diff contains zero changes to any reporting step. Reviewers still get:
report-bundle-size-deltas.ts+GitHubComment@0),Changes
scripts/bundle-scenes-core.tsBUNDLE_MASTER_MANIFEST_FILE/BUNDLE_MASTER_MANIFEST_URLoverrides.azure-pipelines-bundle-manifest.ymlmanifest.jsonand purges the CDN. Refuses to publish an empty manifest, so a broken build cannot blank the baseline.azure-pipelines.ymlscripts/validate-bundle-manifest.ts,scripts/commit-bundle-manifest.tslab/public/bundle/manifest/bundle-scenes-core.tsGUIDANCE.md,TESTING.md,docs/lite/architecture/38-bundle-size-tooling.md.Validation
tsc, eslint, prettier clean; all four pipeline YAMLs parse.azure-pipelines-bundle-manifest.ymlas a pipeline.BabylonJS-DeploymentandBabylonJS-CI-Infrastructurevariable groups for it.Until then the baseline is simply never published — PR builds skip the delta comment and keep gating on ceilings, so nothing breaks.
Accepted trade-off
Per-scene size history is no longer in git — you can no longer
git loga single scene's size over time. In exchange, the two failure modes above disappear entirely.