Repository navigation
feat(ci): PR size report measures per-import JS and CSS and flags touch-only budget overruns - #8296
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
|
… touch-only budget overruns analyze-pr.js measures each changed component's import: the gzip JavaScript it pulls in (transitive, minified, other packages external) and the gzip CSS of the stylesheet rules that JavaScript references, plus each changed package stylesheet's gzip size, at head and at the merge base. The PR comment shows before, after, and delta, and flags a modified component whose JavaScript grew more than 2 KB or CSS more than 1 KB (INV6, AST-080 FR8).
ad0b46c to
7851d61
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the per-component JavaScript figure bundles each import with packages: 'external', so modules reached through package imports (Core modules used by Lab, Charts and Rich Text, d3, intl-messageformat) never count, while architecture:component-style-authoring INV6 defines it as one bundle holding the import and every module it reaches. For example, a Lab component whose entry starts importing Core's BottomSheet moves the reported figure by 23 B while the code it reaches grows by about 18 KB gzip, so the 2 KB flag never fires. Please let esbuild resolve package imports (leaving out React and StyleX only if INV6 is amended to say so), add a test that counts a module reached through a package import, and keep the @keyframes blocks that kept rules name, which the CSS figure currently drops.
[Automated review]
| minify: true, | ||
| format: 'esm', | ||
| platform: 'browser', | ||
| packages: 'external', |
There was a problem hiding this comment.
packages: 'external' keeps every module reached through a package specifier out of the figure: Core modules imported by Lab, Charts and Rich Text, d3, and intl-messageformat. INV6 defines the figure as one bundle holding the import and every module it reaches. For example, a Lab component whose entry starts re-exporting @astryxdesign/core/BottomSheet moves this figure by 23 B while the code it reaches grows by about 18 KB gzip, so the 2 KB flag never fires. Letting esbuild resolve package imports (leaving out React and StyleX only if INV6 says so) would close the gap.
| let out = ''; | ||
| for (const node of nodes) { | ||
| if (node.children) { | ||
| if (/^@keyframes\b/.test(node.prelude)) continue; |
There was a problem hiding this comment.
Skipping every @keyframes block leaves out the keyframes the kept rules name (165 B gzip for Button, 174 B for Switch), which belong to the rules FR8 counts for the component. Keeping each block whose name appears in a kept rule's animation-name would fix it.
…rts included; CSS keeps named keyframes
There was a problem hiding this comment.
Requesting changes: the JavaScript figure now bundles every module an import reaches and the named @keyframes are kept, but the CSS figure takes class names from that whole bundle, so a Lab, Rich Text or Charts component is charged for its package stylesheet's rules that merely share an atomic class with a Core module it uses. For example, Lab InfoTip whose entry gains a Core Button re-export reads +1,166 B CSS and is flagged, while lab.css is unchanged and InfoTip's own modules' rules stay at 458 B, which spec:AST-080 FR8 (“the rules its modules contribute to the package stylesheet”) does not count. Please collect class names only from the reached modules inside the component's own package (keeping the whole bundle for the JavaScript figure), add a test for it, and resolve Lab ChatReasoning's module, which has no index.js and so gets no figure.
[Automated review]
| const js = bundleImport(entry); | ||
| if (js === null) return {importGzipBytes: null, cssGzipBytes: null}; | ||
| const css = stylesheetText | ||
| ? rulesForClasses(stylesheetText, classNamesIn(js)) |
There was a problem hiding this comment.
The class names come from the whole bundle, Core modules included, but the rules come from this component's own package stylesheet. For a Lab, Rich Text or Charts component, rules that merely share an atomic class with a reached Core module are counted, though no module the import reaches contributes them (Core's rules live in astryx.css). Lab InfoTip whose entry gains a Core Button re-export reads +1,166 B CSS and is flagged, while lab.css is byte-identical and InfoTip's own modules' rules stay at 458 B (spec:AST-080 FR8: “the rules its modules contribute to the package stylesheet”). Collecting class names only from the reached modules under the component's own package (esbuild's metafile lists them) would fix it, with the JS figure keeping the whole bundle.
| const dist = path.join(process.cwd(), pkgDist(pkg)); | ||
| return pkg.layout === 'flat' | ||
| ? path.join(dist, `${componentName}.js`) | ||
| : path.join(dist, componentName, 'index.js'); |
There was a problem hiding this comment.
Lab ChatReasoning has no index.ts (the barrel exports it from ./ChatReasoning/ChatReasoning), so this path does not exist and both its figures read N/A on both sides, INV6's “A changed component with no base-against-head gzip JavaScript or CSS figure”. Resolving the module the barrel exports for the component, or falling back to dist/<Name>/<Name>.js, would cover it.
| /** | ||
| * @file component-size.test.mjs | ||
| * Pins how the PR size report measures one component import: the JavaScript | ||
| * it pulls in transitively (other packages external) and the stylesheet rules |
There was a problem hiding this comment.
Nit: the header still says “other packages external”, while the first case now pins that package imports are bundled.
…ference; resolve folders without an index
There was a problem hiding this comment.
Requesting changes: the import and CSS figures now measure what architecture:component-style-authoring INV6 and spec:AST-080 FR8 describe, cross-package imports included, but they are computed only for components whose directory holds exactly one documented component, so most changed components get no figure and can never be flagged. For example, a simulated change confined to Core BottomSheet that grows its import by 5,665 B and its CSS by 1,434 B gzip renders “No new or modified components detected” with no budget section, and the same holds for 113 of Core's 180 and 27 of Lab's 41 canonical components, Selector, Drawer, MobileNav and Dialog among them. Please measure every canonical component whose barrel export lives in a changed directory, at head and at base, and let componentExistsInBase recognise a folder without index.ts such as ChatReasoning, which is currently reported as new with no before figure or flag.
[Automated review]
| // What the component's import costs: the gzip JavaScript it pulls in, | ||
| // transitive modules included, and the gzip CSS of the stylesheet rules | ||
| // that JavaScript references (spec:AST-080 FR8, INV6). | ||
| const importSize = measureComponentImport( |
There was a problem hiding this comment.
Only a component that passes the single-component directory check (line 87) reaches this measurement; a changed Selector/, BottomSheet/, Dialog/ or Table/ directory is recorded as unresolved at line 636 and gets no figure. That leaves 113 of Core's 180 and 27 of Lab's 41 canonical components unmeasured: a simulated change confined to BottomSheet that grows its import by 5,665 B and its CSS by 1,434 B gzip renders “No new or modified components detected” and no budget flag. INV6 and FR8 ask for a figure for each changed component; measuring every canonical component whose barrel export lives in a changed directory, at head and at base, would cover it.
| const dist = path.join(process.cwd(), pkgDist(pkg)); | ||
| if (pkg.layout === 'flat') return path.join(dist, `${componentName}.js`); | ||
| const index = path.join(dist, componentName, 'index.js'); | ||
| return fs.existsSync(index) ? index : path.join(dist, componentName, `${componentName}.js`); |
There was a problem hiding this comment.
This fallback gives ChatReasoning a head figure, but componentExistsInBase (line 194) still asks the base for ChatReasoning/index.ts, so a pull request that changes ChatReasoning lists it under New Components: no before figure, no delta and no flag. Accepting <Name>/<Name>.tsx there too would make it a modified component.
…ers included; a folder without an index exists in base
There was a problem hiding this comment.
Requesting changes: the report now sizes every changed component, family folders included, as architecture:component-style-authoring INV6 and spec:AST-080 FR8 ask, but the Import figure counts modules a component loads with import(), which INV6 says the import does not carry. Without code splitting, esbuild inlines every import() target, so DropdownMenu reads 96.5KB where its import loads 65.8KB, and moving touch-only code behind import() (the remedy the budget section itself suggests) does not lower the figure or clear the import()-only module; separately, ContextMenuItem, an alias of DropdownMenuItem, reads N/A because the alias is bundled from a module that does not export it.
[Automated review]
| resolveDir: path.dirname(path.resolve(entry)), | ||
| loader: 'js', | ||
| }, | ||
| bundle: true, |
There was a problem hiding this comment.
Without code splitting, esbuild inlines every import() target into this one output, so the figure counts code the component loads lazily: DropdownMenu measures 98,812 B where its import loads 67,374 B eagerly, and a fixture component reads 3,895 B with its touch table imported eagerly and 4,175 B with the table behind import(). INV6 says lazily loaded JavaScript is not carried by the import, and the budget section tells authors to lazy-load, so the JS flag cannot clear that way. Counting only the entry chunk and the chunks it imports statically (code splitting), or leaving dynamic-import resolutions external, would fix it; the CSS figure can keep every reached module, since those rules still ship in the stylesheet.
| path.join(dist, target.distModule), | ||
| packageStylesheet(target.pkg), | ||
| dist, | ||
| target.name, |
There was a problem hiding this comment.
ContextMenuItem is exported as DropdownMenuItem as ContextMenuItem (packages/core/src/ContextMenu/index.ts:21), so this passes a name DropdownMenu/DropdownMenuItem.js does not export; esbuild fails with “No matching export” and the row reads N/A in all six columns on every DropdownMenu change. Bundling the name the module exports (the sourceName that namedExportItems parses), or falling back to export *, would give it figures.
… are sized through the package entry
There was a problem hiding this comment.
Requesting changes: the report now sizes each changed component's eagerly loaded JavaScript and its CSS as architecture:component-style-authoring INV6 and spec:AST-080 FR8 ask, and the import() and ContextMenuItem gaps are closed, but the CSS figure reads class names only from eagerly loaded chunks, so touch-only CSS placed behind import() is never counted or flagged even though it still ships in the package stylesheet. For example, 128 coarse-pointer rules that grow astryx.css by 1,557 B gzip read +1,581 B DropdownMenu.tsx, but 0 B with no flag when added to its lazily loaded MenuBottomSheetActionList.tsx. Please collect class names from every output chunk (keeping the eager chunks for the Import figure) and add a test whose stylesheet class lives only in an import() target; separately, drop the “No new or modified components detected” line when the Component Size table has rows, and size Vega's VegaChart from its ESM entry rather than the CommonJS dist/index.js.
[Automated review]
| const inputs = new Set(); | ||
| let text = ''; | ||
| for (const key of eager) { | ||
| text += byPath.get(key) ?? ''; | ||
| for (const input of Object.keys(outputs[key].inputs || {})) { | ||
| inputs.add(path.resolve(workingDir, input)); | ||
| } | ||
| } | ||
| inputs.delete(entryFile); |
There was a problem hiding this comment.
The CSS class scan in measureComponentImport reuses this eager set, so a module the component reaches only through import() never contributes classes, though its rules still ship in the package stylesheet (spec:AST-080 FR8: “the rules its modules contribute to the package stylesheet”; INV6: flagged CSS “stays in the package stylesheet”). The same 128 coarse-pointer rules read +1,581 B DropdownMenu.tsx, and 0B with no flag when added to its lazily loaded MenuBottomSheetActionList.tsx, while astryx.css grows 1,557 B both times. Collecting inputs from every output chunk while keeping text to the eager set would fix it (+1,686 B for the lazy case).
| sizeSection += `| ${inline(key)} | ${sizeText(base.importGzipBytes)} | ${sizeText(head.importGzipBytes)} | ${signedSizeText(importDelta)}${jsOver ? ' ⚠️' : ''} | ${sizeText(base.cssGzipBytes)} | ${sizeText(head.cssGzipBytes)} | ${signedSizeText(cssDelta)}${cssOver ? ' ⚠️' : ''} |\n`; | ||
| } | ||
| sizeSection += | ||
| '\nSizes are gzip. Import counts every module the import reaches; CSS counts the package stylesheet rules its own package modules reference.\n\n'; |
There was a problem hiding this comment.
Nit: the Import figure now leaves out modules reached only through import(), so “every module the import reaches” overstates it; “every module the import loads eagerly” would match the code. The component-size.js header (lines 7-8) says the same, and component-size.test.mjs:6 still says “other packages external”.
|
|
||
| ${diffModeCaveat}${previewAvailabilitySection}${storybookSection}${sandboxSection}${componentSection || '_No new or modified components detected._\n\n'} | ||
| ${bundleSection} | ||
| ${sizeSection}${budgetSection}${bundleSection} |
There was a problem hiding this comment.
When only a folder that holds several components changes (DropdownMenu, BottomSheet, Selector, Table), the line above still renders “No new or modified components detected.” and this Component Size table follows it with that folder's components, sizeKeys is non-empty would keep the report consistent.
| // measure it through the package entry instead. | ||
| if (size.importGzipBytes === null) { | ||
| size = measureComponentImport( | ||
| path.join(dist, 'index.js'), |
There was a problem hiding this comment.
In a package that is not "type": "module", index.js is the CommonJS build: Vega's VegaChart, which has no per-component module, is measured from it at 278,537 B, while the ESM dist/index.mjs a product's import resolves gives 267,580 B. resolveEntries(dist, target.pkg).esmPath picks the right entry.
…e the ESM entry; no empty-components line beside the size table
There was a problem hiding this comment.
Requesting changes: the report now counts CSS in lazily loaded modules, sizes Vega's chart from its ESM entry and drops the empty-components line, but the CSS figure reads class names from every esbuild output chunk, including chunks for import() calls that tree shaking removed, so components are charged for CSS they never load (spec:AST-080 FR8: “the rules its modules contribute”). For example, adding 128 coarse-pointer rules to DropdownMenu's lazily loaded touch action list flags ContextMenuItem at +1.5KB DropdownMenuItem, the component it aliases, reads 0B. Please collect class names only from the chunks the entry reaches through static and dynamic imports, add a test whose import() sits in an unused export, and update the footnote and file headers that say the Import figure counts every module the import reaches.
[Automated review]
| // CSS ships in the package stylesheet whether its module loads eagerly or | ||
| // through import(), so class names come from every chunk. | ||
| const inputs = new Set(); | ||
| for (const key of Object.keys(outputs)) { | ||
| for (const input of Object.keys(outputs[key].inputs || {})) { | ||
| inputs.add(path.resolve(workingDir, input)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This scans every esbuild output, including the chunks esbuild emits for an import() that sits in code tree shaking removed; no output imports those chunks, yet their classes count. ContextMenuItem, measured through the package entry, gets 22 chunks of which its entry reaches 9, so its CSS reads 14,337 B against DropdownMenuItem's 5,050 B, and adding coarse-pointer rules to MenuBottomSheetActionList.tsx flags it +1,532 B DropdownMenuItem reads 0B (spec:AST-080 FR8: “the rules its modules contribute”). Walking the outputs from entryOutput through both import-statement and dynamic-import edges, and collecting inputs only from those, keeps DropdownMenu at +1,586 B
| sizeSection += `| ${inline(key)} | ${sizeText(base.importGzipBytes)} | ${sizeText(head.importGzipBytes)} | ${signedSizeText(importDelta)}${jsOver ? ' ⚠️' : ''} | ${sizeText(base.cssGzipBytes)} | ${sizeText(head.cssGzipBytes)} | ${signedSizeText(cssDelta)}${cssOver ? ' ⚠️' : ''} |\n`; | ||
| } | ||
| sizeSection += | ||
| '\nSizes are gzip. Import counts every module the import reaches; CSS counts the package stylesheet rules its own package modules reference.\n\n'; |
There was a problem hiding this comment.
Nit: the Import figure leaves out modules reached only through import(), so “every module the import reaches” overstates it; “every module the import loads eagerly” would match the code, here and in the component-size.js header (line 8).
| /** | ||
| * @file component-size.test.mjs | ||
| * Pins how the PR size report measures one component import: the JavaScript | ||
| * it pulls in transitively (other packages external) and the stylesheet rules |
There was a problem hiding this comment.
Nit: “other packages external” is stale; the first case pins that package imports are bundled.
…note describe eager JS and loadable CSS
There was a problem hiding this comment.
The size report now measures each changed component's eagerly loaded JavaScript and its loadable CSS as architecture:component-style-authoring INV6 and spec:AST-080 FR8 describe, and coarse-pointer CSS in DropdownMenu's lazily loaded touch list flags only the components that can load it. One change is still needed: components are chosen by the folder that changed (.github/scripts/analyze-pr.js:536-555), so a touch-only change confined to a shared module, such as a tuning table in hooks/useLongPress.ts or coarse-pointer rules in utils/interactionOverlay.stylex.ts, pushes many components past the budget with no size row and no flag. Choosing components by what their bundles reach (or sizing every component of a changed package and listing the ones that moved), with a test where only a shared hook changes, would close it.
- The Evidence figures in the description (Button 41.4 KB and 7.0 KB, Selector 90.7 KB and 13.7 KB) no longer match what this head computes (about 39.9 KB and 8.9 KB, and 74.1 KB and 14.4 KB).
[Automated review]
| const file = exports.get(name); | ||
| if (!file) continue; | ||
| const relative = path.relative(sourceDir, file); | ||
| if (!changed.has(relative.split(path.sep)[0])) continue; |
There was a problem hiding this comment.
Only a component whose defining module sits in a changed folder gets a row, so a change confined to a shared module selects none. With this head's code over a built Core, a touch tuning table added to hooks/useLongPress.ts grows 12 components' imports by 3,576 to 3,811 B, and 85 coarse-pointer rules in utils/interactionOverlay.stylex.ts grow 175 components' CSS by 1,085 to 1,117 B; for both changes this loop returns no target and the report reads “No new or modified components detected.”, the case INV6 and FR8 say is flagged. Choosing targets by what each component's bundle reaches (size every canonical component of a changed package and keep the rows that moved, or keep the components whose esbuild inputs include a changed file), plus an analyze-pr.test.mjs case where only a shared hook changes, would close it.
…shared hooks and style modules included
There was a problem hiding this comment.
Requesting changes: the report now measures each component's eagerly loaded JavaScript and loadable CSS as architecture:component-style-authoring INV6 and spec:AST-080 FR8 describe, and FR8's 0.9 KB and 1.1 KB CSS pair falls on the right side of the flag. One gap remains: CI measures the merge base in the size-base job without building the head, but sizeTargets takes bundle reach from the head build (.github/scripts/analyze-pr.js:777), so components reached through a changed module read N/A and are never flagged; a touch tuning table in hooks/useLongPress.ts lists 12 components at N/A with no budget section, although each import grows by 3.6 to 3.8 KB. Deciding reach in the merge-base build as well (for example by running sizeTargets inside --base-root and measuring the union) and adding an analyze-pr.test.mjs case that runs --base-only with only the merge base built would close it.
[Automated review]
| // and package lists stay the head's; only the stats are read in the base tree. | ||
| let baseComponentStats = null; | ||
| let baseBundlePackages = null; | ||
| const targets = sizeTargets(changedFiles); |
There was a problem hiding this comment.
In CI this list is also what the merge base is measured for, and the size-base job never builds the head (.github/workflows/ci.yml:1122-1147 builds only .size-base), so bundleInputsByEntry finds no entries here and only folder matches survive. Every component reached through a changed module then has no baseComponentSizes entry, reads N/A and cannot be flagged: on a Core build of this pull request, a touch table in hooks/useLongPress.ts gives 12 head rows and 0 base figures. Running sizeTargets in the --base-root tree too and measuring the union would close it.
There was a problem hiding this comment.
Requesting changes: the report now measures, base against head, every component a change reaches, including through shared modules in CI's base-only job, and its import and CSS figures, flags and stylesheet rows match architecture:component-style-authoring INV6 and spec:AST-080 FR8. One gap remains: unionTargets keeps the head tree's module path when the merge base is measured (.github/scripts/analyze-pr.js:590-594), so a component whose defining module the pull request renames is measured there through the package entry and its growth can read as shrinkage; renaming Center/Center.tsx to CenterView.tsx with a 2.2 KB tuning table reports -3.2KB and no flag. Measuring each tree with its own defining module (for example, letting the merge-base targets win in unionTargets before measuring there) and adding an analyze-pr.test.mjs case with a renamed defining module would close it.
[Automated review]
| getPackageBundleStats(p), | ||
| ); | ||
| targets = unionTargets(targets, sizeTargets(changedFiles)); | ||
| baseComponentSizes = measureSizes(targets); |
There was a problem hiding this comment.
In the merge-base tree this measures each shared key with the head tree's distModule, because unionTargets keeps the first tree's target (line 592). When the pull request renames the module that defines a component, that path does not exist in the base build, so measureSizes falls back to the package entry, whose bundle keeps modules the component's import never loads: renaming Center/Center.tsx to CenterView.tsx with a 2.2 KB tuning table gives a base of 9,632 B instead of Center's own 4,131 B, so the row reads -3.2KB and nothing is flagged. Measuring here with the merge-base targets first, for example measureSizes(unionTargets(sizeTargets(changedFiles), targets)), would use the base tree's own module.
There was a problem hiding this comment.
This looks ready to merge: the size report now measures, base against head, each affected component's eagerly loaded JavaScript and its package-stylesheet CSS, and flags growth past 2 KB or 1 KB per component as architecture:component-style-authoring INV6 and spec:AST-080 FR8 ask, renamed defining modules included. One small fix: the description says a change to hooks/useLongPress.ts sizes 13 components, but on this head it sizes 12. Worth a thought for a follow-up: a component new at head shows N/A deltas and is never flagged although its CSS joins the package stylesheet, and a CSS change to a shared module such as Button is listed against every component that renders it.
[Automated review]
Builds on #8277 (base and head size measurement), now on
main.Maintainer impact
Reviewers cannot see what a change adds to the code and CSS a product downloads for one component, so touch-only growth that ships to every desktop product lands unnoticed.
spec:AST-080FR8 andarchitecture:component-style-authoringINV6 require each pull request to report it and flag touch-only growth over budget.Intended invariant
architecture:component-style-authoringINV6: every pull request reports, base against head, the gzip JavaScript each changed component's import pulls in (transitive modules included) and its gzip CSS delta; touch-only growth over 2 KB JavaScript or 1 KB CSS per component is flagged.spec:AST-080FR8: each changed package stylesheet's gzip size is reported base against head.Change
lib/component-size.jsbundles one component import with esbuild (minified for production, ESM, every module it reaches including package imports such as Core from Lab, React, and third-party libraries), gzips it at the default level, collects the StyleX class names the reached modules of the component's own package reference, and gzips the package stylesheet rules whose selectors name those classes, keeping their at-rule wrappers and the @Keyframes they name.analyze-pr.jssizes every public component whose folder changed or whose bundle reaches a changed module of its package (family folders and shared hooks or style modules included) from the dist module that defines its export, recordscomponentSizesandstylesheetGzipBytesper package at head and, through--base-root, at the merge base, and recognises a component folder without an index barrel in base.Evidence
hooks/useLongPress.tssizes the 13 components whose bundles reach it; the comment tests render the flagged and unflagged rows.Scope
Testing
vitest run .github/scripts/lib/component-size.test.mjs .github/scripts/generate-pr-comment.test.mjs .github/scripts/analyze-pr.test.mjs .github/scripts/lib/pr-preview.test.mjs .github/scripts/ci-*.test.mjs@astryxdesign/core.