perf(admin): defer icon and chart bundles - #2516
Conversation
🦋 Changeset detectedLatest commit: 08002e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right change for the right reason. Deferring the full Phosphor catalog and the chart renderer out of the admin entry chunk matches the approved Discussion and follows EmDash’s existing code-splitting patterns. The deterministic icon buckets, alias-preserving loader, and production-fixture bundle guard are well-scoped and the snapshot reuse in the query-counts workflow avoids a redundant build.
I reviewed the diff, the generator/loader logic, the bundle-guard script, the Vite test/dev resolution plugins, the chart lazy-loading path, and the new tests against AGENTS.md. No security issues, data-integrity regressions, or broken contracts were found. The only issues are test/assertion quality and comment discipline:
- The new chart-failure test and the new chart-height test assert exact Tailwind class strings back at the implementation. Per AGENTS.md these are tautological tests that cannot fail on a real regression.
- The bundle-guard script opens with several paragraphs justifying the threshold constants, which AGENTS.md treats as reviewer-facing justification rather than durable code context.
Once the class-string assertions are removed/rewritten and the rationale comments are trimmed, this is good to land.
| }); | ||
|
|
||
| const placeholder = container.querySelector<HTMLElement>('[aria-hidden="true"]'); | ||
| expect(placeholder?.className).toContain("rounded-lg border border-kumo-line p-4"); |
There was a problem hiding this comment.
[needs fixing] This assertion repeats the exact Tailwind class string from ChartPlaceholder:
expect(placeholder?.className).toContain("rounded-lg border border-kumo-line p-4");It is a tautological test: the only way it fails is if someone intentionally changes those classes. Per AGENTS.md, that makes it worse than no test. The useful behavior here is that a placeholder is rendered and the configured height is preserved. Drop the class assertion and keep the height check.
| expect(placeholder?.className).toContain("rounded-lg border border-kumo-line p-4"); | |
| expect(placeholder).toBeTruthy(); |
| expect(fallback?.classList).toContain("rounded-lg"); | ||
| expect(fallback?.classList).toContain("border"); | ||
| expect(fallback?.classList).toContain("border-kumo-line"); | ||
| expect(fallback?.classList).toContain("p-4"); |
There was a problem hiding this comment.
[needs fixing] These four assertions pin the implementation detail (the exact Tailwind classes added in this PR) straight back:
expect(fallback?.classList).toContain("rounded-lg");
expect(fallback?.classList).toContain("border");
expect(fallback?.classList).toContain("border-kumo-line");
expect(fallback?.classList).toContain("p-4");Like the chart-failure test, this restates the diff and can only fail on an intentional style change. Remove them and exercise the observable behavior instead: the placeholder exists before the chart resolves and the configured height is applied. The later assertions that the chart appears and the placeholder disappears already cover the real behavior.
| // The entry and closure ceilings retain roughly 400 KB of normal bundle drift, | ||
| // but less than one deferred chart payload. The definition cap is over twice | ||
| // the optimized fixture count while remaining far below the full icon catalog. | ||
| const MAX_PLUGIN_REGISTRY_BYTES = 2_250_000; | ||
| const MAX_INITIAL_CLOSURE_BYTES = 3_150_000; | ||
| const MAX_INITIAL_PHOSPHOR_DEFS = 350; | ||
| // Feature chunks may carry a focused icon set; namespace imports exceed both caps by an order of magnitude. | ||
| const MAX_NON_BUCKET_LAZY_PHOSPHOR_DEFS = 32; | ||
| const MAX_TOTAL_NON_BUCKET_LAZY_PHOSPHOR_DEFS = 96; | ||
| // A deferred bucket has about 25% growth room before one icon request becomes too large. |
There was a problem hiding this comment.
[needs fixing] The constant block opens with reviewer-facing justification rather than durable context for future readers:
// The entry and closure ceilings retain roughly 400 KB of normal bundle drift,
// but less than one deferred chart payload. The definition cap is over twice
// the optimized fixture count while remaining far below the full icon catalog.
// Feature chunks may carry a focused icon set; namespace imports exceed both caps by an order of magnitude.
// A deferred bucket has about 25% growth room before one icon request becomes too large.AGENTS.md says comments should not justify decisions, narrate alternatives, or address reviewers. These thresholds are either self-explanatory (the constants are right above them) or belong in the changeset/PR description. Delete this comment block; if the history really matters, capture it in the changeset instead.
| plugins: [linguiMacroPlugin()], | ||
| // @tiptap/suggestion is intentionally bundled (devDependency) | ||
| inlineOnly: false, | ||
| // Generated icon buckets are package-owned chunks; normal root imports stay consumer-owned. |
There was a problem hiding this comment.
[suggestion] This comment justifies the noExternal regex rather than explaining a non-obvious invariant to future readers:
// Generated icon buckets are package-owned chunks; normal root imports stay consumer-owned.
noExternal: /^@phosphor-icons\/react\//,AGENTS.md discourages decision-justification comments. The regex itself already encodes the rule (deep @phosphor-icons/react/* imports are bundled; the bare import is listed in external). Either delete the comment or replace it with a short note about why a bare root import must remain external (consumer-side deduplication / initial icon set).
github-actions
Bot
added
review/awaiting-author
Scope checkThis PR changes 2,550 lines across 54 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
Pull request overview
This PR improves admin/perf-site bundle performance by code-splitting uncommon Phosphor navigation icons into deterministic lazy buckets and lazy-loading the Block Kit chart renderer, plus adding a CI guard to detect bundle-regression chunk graphs during the query-counts workflow.
Changes:
- Introduces generated Phosphor icon bucket modules + a runtime loader/fallback, and updates admin navigation icon resolution to use lazy buckets (with an explicit Phosphor version pin).
- Lazily loads the Block Kit chart renderer with a height-preserving placeholder and a failure fallback that keeps routes mounted; adds targeted tests.
- Adds a production-fixture bundle metadata emitter and a
bundle:checkguard script, wired into the Query Counts workflow between SQLite and D1 builds.
Reviewed changes
Copilot reviewed 20 out of 54 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/check-admin-client-bundle.mjs | Emits/validates fixture client chunk graph and enforces bundle ceilings (icons/charts). |
| pnpm-workspace.yaml | Pins @phosphor-icons/react exactly to keep generated buckets deterministic. |
| pnpm-lock.yaml | Updates catalog specifier to match the exact Phosphor pin. |
| packages/core/tests/unit/astro/vite-config.test.ts | Verifies dev-time Phosphor deep-import resolver behavior under source aliasing. |
| packages/core/src/astro/integration/vite-config.ts | Adds a Vite plugin to resolve Phosphor deep imports when serving raw admin source. |
| packages/blocks/tests/renderer.test.tsx | Updates chart mocks and adds a test ensuring height is reserved during chart lazy-load. |
| packages/blocks/tests/renderer-chart-failure.test.tsx | Adds coverage for chart chunk failure: logs error and keeps placeholder mounted. |
| packages/blocks/src/renderer.tsx | Switches chart block rendering to React.lazy + Suspense with failure fallback. |
| packages/admin/vitest.config.ts | Adds a Vite resolve plugin so Phosphor deep imports resolve correctly in browser tests. |
| packages/admin/tsdown.config.ts | Bundles Phosphor deep imports for bucket chunks while keeping root import external. |
| packages/admin/tests/lib/phosphor-icon-loader-failure.test.ts | Ensures icon-bucket load failures log and fall back to Plug. |
| packages/admin/tests/components/Sidebar.test.tsx | Adds assertions for name/alias/icon-module-alias resolution via the bucket loader. |
| packages/admin/src/lib/phosphor-icon-loader.ts | Implements deterministic bucket selection + lazy loading + Plug fallback on chunk failure. |
| packages/admin/src/generated/phosphor-icon-buckets/module-aliases.ts | Generated mapping of exported aliases to owning icon modules. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-00.ts | Generated Phosphor icon bucket 00 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-01.ts | Generated Phosphor icon bucket 01 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-02.ts | Generated Phosphor icon bucket 02 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-03.ts | Generated Phosphor icon bucket 03 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-04.ts | Generated Phosphor icon bucket 04 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-05.ts | Generated Phosphor icon bucket 05 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-06.ts | Generated Phosphor icon bucket 06 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-07.ts | Generated Phosphor icon bucket 07 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-08.ts | Generated Phosphor icon bucket 08 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-09.ts | Generated Phosphor icon bucket 09 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-10.ts | Generated Phosphor icon bucket 10 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-11.ts | Generated Phosphor icon bucket 11 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-12.ts | Generated Phosphor icon bucket 12 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-13.ts | Generated Phosphor icon bucket 13 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-14.ts | Generated Phosphor icon bucket 14 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-15.ts | Generated Phosphor icon bucket 15 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-16.ts | Generated Phosphor icon bucket 16 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-17.ts | Generated Phosphor icon bucket 17 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-18.ts | Generated Phosphor icon bucket 18 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-19.ts | Generated Phosphor icon bucket 19 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-20.ts | Generated Phosphor icon bucket 20 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-21.ts | Generated Phosphor icon bucket 21 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-22.ts | Generated Phosphor icon bucket 22 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-23.ts | Generated Phosphor icon bucket 23 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-24.ts | Generated Phosphor icon bucket 24 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-25.ts | Generated Phosphor icon bucket 25 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-26.ts | Generated Phosphor icon bucket 26 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-27.ts | Generated Phosphor icon bucket 27 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-28.ts | Generated Phosphor icon bucket 28 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-29.ts | Generated Phosphor icon bucket 29 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-30.ts | Generated Phosphor icon bucket 30 exports. |
| packages/admin/src/generated/phosphor-icon-buckets/bucket-31.ts | Generated Phosphor icon bucket 31 exports. |
| packages/admin/src/components/admin-navigation-icons.ts | Switches plugin nav icon lazy import path to bucket loader + fallback. |
| packages/admin/scripts/generate-phosphor-icon-buckets.js | Generates deterministic icon bucket modules + module-alias mapping; supports --check. |
| packages/admin/package.json | Adds icon generation/check scripts and wires them into build/dev/typecheck. |
| package.json | Adds bundle:check and bundle:check:ci scripts. |
| fixtures/perf-site/astro.config.mjs | Adds bundle-metadata emitter plugin to the perf fixture’s Vite build. |
| .oxfmtrc.json | Excludes generated icon buckets from formatting. |
| .github/workflows/query-counts.yml | Runs bundle:check:ci after SQLite snapshot regeneration and before D1. |
| .changeset/small-icons-load.md | Changeset describing deferred icon/chart payloads and failure behavior. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the automated review in
Reverification:
The first Browser Tests run failed only in unchanged |
github-actions
Bot
added
review/needs-rereview
|
CI lint exposed four
|
|
Retriggering CI after unrelated runner flakes and a transient pkg.pr.new 500; no code change. |
There was a problem hiding this comment.
This is the right change for the right reason: it defers the full Phosphor catalog and the Block Kit chart renderer out of the admin entry chunk, matching the approved Discussion and EmDash's existing code-splitting patterns. The deterministic FNV buckets, alias-preserving loader, and production-fixture bundle guard are well scoped.
Both prior-review issues are resolved in this revision:
- The chart failure and chart height tests no longer assert exact Tailwind class strings; they now check the Suspense placeholder through the aria-hidden container and the rendered chart's
data-height. - The bundle-guard script no longer opens with long rationale comments about threshold constants.
I reviewed the diff, the generator/loader logic, the bundle-guard script, the Vite dev/test resolution plugins, the lazy chart path, and the new tests against AGENTS.md. No security issues, DB regressions, localization leaks, or API contract breaks. There is one remaining robustness issue in the icon loader.
Remaining issue: loadPhosphorIcon does an own-property check before returning a bucket member. Because the bucket object inherits from Object.prototype, a name like "toString" would return Object.prototype.toString instead of undefined, and isIconComponent would then accept it as a valid icon. The normal caller (resolveNavIcon) PascalCases the name first, so the current UI path avoids this, but the exported loader's contract is to return undefined for unknown names and should not be sensitive to prototype properties.
| const loadBucket = PHOSPHOR_ICON_BUCKETS[getBucket(moduleName)]; | ||
| if (!loadBucket) return undefined; | ||
| try { | ||
| return (await loadBucket())[name]; |
There was a problem hiding this comment.
[suggestion] The bucket access return (await loadBucket())[name]; does not verify that name is an own property of the module namespace. Since the namespace inherits from Object.prototype, names like "toString" or "constructor" return prototype methods, pass isIconComponent, and get rendered as bogus icons instead of falling back to Plug.
The surrounding code already returns undefined for truly unknown icons, so the intent is clearly to fall back; prototype pollution breaks that intent. Add an own-property check:
| return (await loadBucket())[name]; | |
| const bucket = await loadBucket(); | |
| if (!Object.prototype.hasOwnProperty.call(bucket, name)) return undefined; | |
| return bucket[name]; |
|
Addressed the final loader robustness issue in
Verification:
|
|
Retriggering the unchanged head once more for the detached Chromium frame flake in unchanged hooks.test.tsx; all changed-code, lint, typecheck, bundle, preview, visual, and E2E gates pass. |
There was a problem hiding this comment.
This is the right change for the right reason. It defers the full Phosphor catalog and the Block Kit chart renderer out of the admin entry chunk, matching the approved performance Discussion and EmDash’s existing client-only architecture (the admin shell renders with client:only="react", so Suspense/lazy icons do not affect SSR). The deterministic FNV buckets, alias-preserving loader, production-fixture bundle guard, and dev/test deep-import resolvers are all well-scoped.
I reviewed the diff and the full changed files statically — I did not re-run pnpm test, lint, or the bundle guard in this environment, so I’m relying on the author’s verification checklist for those.
Headline checks:
- The prior-review prototype-property issue is resolved:
loadPhosphorIconnow usesObject.hasOwnboth on the alias map and on the bucket module, andSidebar.test.tsxadds a regression test for"constructor". - The chart Suspense/fallback preserves the configured height on load and on chunk failure; the tests now assert observable behavior (
data-height,aria-hiddenplaceholder) rather than brittle Tailwind class strings. - The bundle-guard script no longer opens with long threshold-rationale comments.
- No new user-facing strings are added, so the Lingui/Done column is correctly n/a.
- No DB, auth, API-route, or locale-filter changes are introduced.
- The changeset is present and describes the user-visible effect, not implementation internals.
I didn’t find any blocking issues, regressions, or AGENTS.md violations in the changed code. LGTM.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
github-actions
Bot
added
review/approved
What does this PR do?
Defers uncommon plugin navigation icons into 32 deterministic Phosphor buckets and lazy-loads Block Kit chart rendering. Existing icon names and aliases remain supported; unknown or failed icon chunks fall back to
Plug, while failed chart chunks retain the configured-height placeholder instead of unmounting the route.Adds a production-fixture bundle guard that checks the emitted chunk graph, rejects eager/full-catalog regressions and stale CI builds, and runs between the SQLite and D1 query-count builds.
Approved performance Discussion: #2486
Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (verified by upstream CI; local Oxlint aborts inoxc_allocator)pnpm testpasses (targeted tests for this change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
No persistent visual change after lazy chunks load. The chart fallback reserves the configured height while loading and on chunk failure.
Matched production-site build measurements:
Current-upstream fixture guard:
Verification:
pnpm typecheckpnpm bundle:checkpnpm formatandpnpm format:checkgit diff --check