perf(ui): bundle both ui roots in one compilation, 24MB to 16MB - #10629
Merged
Conversation
PR Summary by QodoBundle workspace + scope UI roots in a single rspack compilation (24MB → 16MB)
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Stale builds lack root HTML
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+114
to
+116
| // one html per entry, each injecting only its own entry's chunks. `index.html` is no longer | ||
| // emitted: with two roots in one compilation there is no single default document, so the ui | ||
| // server falls back to `<entry>.html` for the root it is serving. |
There was a problem hiding this comment.
1. Workbox fallback references index 🐞 Bug ≡ Correctness
The browser build no longer emits a shared index.html (it emits <entry>.html per root), but Workbox is still configured with navigateFallback: 'public/index.html', so service-worker navigation fallback can target a document that isn’t produced by this build.
Agent Prompt
### Issue description
The rspack browser build now emits one HTML per entry (`<entry>.html`) and explicitly stops emitting `index.html`, but Workbox `GenerateSW` still uses `navigateFallback: 'public/index.html'`. This leaves the generated service worker with a navigation fallback URL that won’t exist for either UI root.
### Issue Context
- The server history fallback was updated to serve `<root>.html`, so the runtime “online” routing path is correct, but the service worker’s navigation fallback is now out-of-sync.
- Because this is a shared multi-entry bundle, a single static `navigateFallback` cannot be correct for both roots unless you deliberately keep a shared fallback document.
### Fix Focus Areas
- scopes/ui-foundation/ui/rspack/rspack.browser.config.ts[113-143]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+75
to
+78
| await pMapSeries(Object.values(UIROOT_ASPECT_IDS), async (uiRootAspectId) => { | ||
| const maybeUiRoot = this.ui.getUi(uiRootAspectId); | ||
| if (!maybeUiRoot) throw new Error(`no uiRoot found for ${uiRootAspectId}`); | ||
| const [, uiRoot] = maybeUiRoot; |
There was a problem hiding this comment.
2. Hash generation hard-fails 🐞 Bug ☼ Reliability
BundleUiTask.generateHash() iterates the hardcoded UIROOT_ASPECT_IDS list and throws when a root isn’t registered, causing bundle generation to fail in runtimes that legitimately register only one UI root (e.g. scope-only).
Agent Prompt
### Issue description
`BundleUiTask.generateHash()` currently throws when `this.ui.getUi(uiRootAspectId)` returns undefined. This contradicts the intended behavior documented in the comment (missing root should simply mean “no pre-bundle for that root”) and can fail bundle generation in contexts where only one root is registered.
### Issue Context
Scope and workspace register their UI roots independently, so a runtime can naturally have only one of them registered.
### Fix Focus Areas
- scopes/ui-foundation/ui/bundle-ui.task.ts[68-84]
- scopes/scope/scope/scope.main.runtime.ts[1520-1527]
- scopes/workspace/workspace/workspace.main.runtime.ts[273-274]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+30
| export function getUiRootEntryName(uiRootAspectId: string): string { | ||
| // a root outside the two bit ships still gets a usable entry name rather than failing the build. | ||
| return BUNDLE_UIROOT_DIR[uiRootAspectId] || uiRootAspectId.replace(/[^a-zA-Z0-9-]+/g, '-'); | ||
| } |
There was a problem hiding this comment.
3. Entry name collisions possible 🐞 Bug ≡ Correctness
For non-core UI roots, getUiRootEntryName() uses lossy sanitization without collision detection (e.g. foo/bar and foo@bar both become foo-bar), so Object.fromEntries() can silently overwrite one entry and produce wrong/missing <entry>.html + manifest data.
Agent Prompt
### Issue description
`getUiRootEntryName()` sanitizes arbitrary aspect IDs into entry names but doesn’t guarantee uniqueness. When two IDs sanitize to the same string, the later entry overwrites the earlier one in `Object.fromEntries`, causing one root to disappear from the compilation output (and its HTML/manifest to be wrong).
### Issue Context
The function explicitly aims to support roots beyond the two shipped ones, which makes collisions plausible.
### Fix Focus Areas
- scopes/ui-foundation/ui/bundle-ui.task.ts[22-34]
- scopes/ui-foundation/ui/ui.main.runtime.ts[254-260]
- scopes/ui-foundation/ui/rspack/rspack.browser.config.ts[54-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
guysaar223
approved these changes
Aug 18, 2026
This was referenced Aug 18, 2026
Base automatically changed from
fix/ui-ssr-bundle-size-and-invalid-tag
to
master
August 18, 2026 21:25
GiladShoham
enabled auto-merge (squash)
August 19, 2026 04:58
GiladShoham
force-pushed
the
perf/ui-bundle-single-compilation
branch
from
August 19, 2026 05:02
9659e1a to
abfe40d
Compare
Comment on lines
+720
to
+721
| const outputPath = pathResolve(uiRoot.path, await this.publicDir(uiRoot)); | ||
| if (fs.pathExistsSync(outputPath)) return false; |
There was a problem hiding this comment.
1. Stale builds lack root html 🐞 Bug ≡ Correctness
buildIfNoBundle() treats any existing public output directory as a valid build, even though the server now requires <root>.html and pre-PR local builds contain only index.html. When the path-based build hash is unchanged, startup skips rebuilding and client-side routes return 404 because the required root document is absent.
Agent Prompt
## Issue description
Existing local UI output from the previous layout may contain only `index.html`. The new server falls back to a root-specific HTML file, but `buildIfNoBundle()` skips building whenever the output directory exists, allowing stale output to cause client-side route failures.
## Issue Context
The normal build cache hashes resolved aspect paths rather than the browser configuration/layout, so this PR's document-name change does not necessarily invalidate an existing local build. Validate the expected root-specific HTML document, not merely its parent directory, before deciding that a usable build exists.
## Fix Focus Areas
- scopes/ui-foundation/ui/ui.main.runtime.ts[718-724]
- scopes/ui-foundation/ui/ui-server.ts[286-291]
- scopes/ui-foundation/ui/bundle-ui.task.ts[27-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit abfe40d |
GiladShoham
added a commit
that referenced
this pull request
Aug 19, 2026
…10628/#10629 (#10631) Sanity e2e for `bit start` itself, covering both UI roots. Part 3 of #10596 follow-up work. > Rebased onto `master` now that #10628 and #10629 have merged; targets `master` directly. Until now nothing exercised `bit start` end to end. That is how the scope SSR bundle managed to throw on every request for months (#10628), and the layout change in #10629 has a matching failure mode: name the fallback document wrong and every *client-side* route 404s while SSR-rendered ones keep working. `e2e/harmony/ui-start.e2e.ts` starts a real server per root and asserts, over http: - startup writes nothing matching `/error|exception|unhandled/i` to stderr — a server can listen fine with an aspect that failed to load - the served document has a react root - **every script and stylesheet the document references actually resolves 200** — this is the one that catches assets emitted under a path the server does not expose - a deep client-side route returns a document (the history-api fallback) - `/graphql` answers without errors - workspace only: the document loads the *workspace* entry and not the scope one — both roots are entries of one bundle now, so serving the wrong document would still look like a working page, just booting the other root's app - scope only: the markup is server-rendered and contains the exported component 12 assertions, ~1 min. All `--rebuild`, so they describe this repo's code rather than whichever bit release is installed. ## Two supporting changes **`HttpHelper` can start either root.** It was hardcoded to the bare scope (`scopes.remotePath`, and a ready-message string naming `teambit.scope/scope`). It now takes `{ extraArgs, uiRootAspectId }`, derives the cwd from the root, and builds the ready message per root. Existing callers use the unchanged two-arg form. It also records stderr so tests can assert on a clean startup. **`portHolders()` now filters to listening sockets** (`lsof -ti tcp:PORT -sTCP:LISTEN`). Without it lsof also reports processes holding a *client* socket to the port — including the mocha process itself, since node keeps connections alive after a test fetches from the server. `waitForPortToBeFree` read that as a foreign process squatting the port and refused to continue, failing the `after` hooks. Only a listener can actually hold a port. This was a latent bug in the helper; the new tests hit it because they fetch every referenced asset. --- ## Also: Qodo review findings from #10628 / #10629 Both of those merged before their review findings were addressed, so the actionable ones land here. Each was verified against the code rather than taken on trust. **`bit start` 404s on an existing local UI build (from #10629) — the important one.** `buildIfNoBundle()` treated *any* existing `public/bit` directory as a valid build, but the server now falls back to `<root>.html`, which a build made before #10629 does not contain. Reproduced end to end: with the pre-fix check the whole UI returns **404** on `/` and on deep routes; with the fix it detects the missing document, rebuilds, and serves 200. This would have hit every user upgrading past #10629 with a previously-built local UI. It now checks for the root's document rather than the directory. **Hash written for roots that were never built (from #10629).** `generateHash()` walked a hardcoded root list and threw when one was not registered. Beyond failing in a scope-only runtime, it could record a hash for a root whose document was never emitted — which reads at startup as "a pre-bundle exists" and then 404s, the same failure as above. It now walks the same registered roots `build()` turns into entries, via a new `UiMain.getUiRoots()`. **Service worker bound to a document that is not emitted (from #10629).** Confirmed in the built artifact: `service-worker.js` contained `createHandlerBoundToURL("public/index.html")` while the build emits only `scope.html` / `workspace.html`. With an entry per root there is no single app shell, so `navigateFallback` is removed — the express history-api fallback already serves the right document. Verified the built service worker no longer contains that binding. **Entry name collisions (from #10629).** `Object.fromEntries` would silently keep only the last of two entries sharing a sanitized name, leaving a root with no chunks and no document while still looking built. Now throws instead. **Stats filename could break (from #10628).** `writeBundleStats` interpolated an unsanitized name into a path, so a root name containing `/` would fail with ENOENT into a swallowed debug log. Now sanitized. Not changed: the "ad-hoc chalk in `writeStats`" rule violation. That line matches the surrounding `[Rspack]` log statements in the same file; the style guide it cites covers section titles and symbols in command output, not diagnostic log lines. Happy to switch it if you'd rather be strict. The `preview/bundle-stats.ts` copy of the sanitization fix lands with #10632, which is where that file lives.
GiladShoham
added a commit
that referenced
this pull request
Aug 19, 2026
…10628/#10629 (#10631) Sanity e2e for `bit start` itself, covering both UI roots. Part 3 of > Rebased onto `master` now that #10628 and #10629 have merged; targets `master` directly. Until now nothing exercised `bit start` end to end. That is how the scope SSR bundle managed to throw on every request for months (#10628), and the layout change in #10629 has a matching failure mode: name the fallback document wrong and every *client-side* route 404s while SSR-rendered ones keep working. `e2e/harmony/ui-start.e2e.ts` starts a real server per root and asserts, over http: - startup writes nothing matching `/error|exception|unhandled/i` to stderr — a server can listen fine with an aspect that failed to load - the served document has a react root - **every script and stylesheet the document references actually resolves 200** — this is the one that catches assets emitted under a path the server does not expose - a deep client-side route returns a document (the history-api fallback) - `/graphql` answers without errors - workspace only: the document loads the *workspace* entry and not the scope one — both roots are entries of one bundle now, so serving the wrong document would still look like a working page, just booting the other root's app - scope only: the markup is server-rendered and contains the exported component 12 assertions, ~1 min. All `--rebuild`, so they describe this repo's code rather than whichever bit release is installed. **`HttpHelper` can start either root.** It was hardcoded to the bare scope (`scopes.remotePath`, and a ready-message string naming `teambit.scope/scope`). It now takes `{ extraArgs, uiRootAspectId }`, derives the cwd from the root, and builds the ready message per root. Existing callers use the unchanged two-arg form. It also records stderr so tests can assert on a clean startup. **`portHolders()` now filters to listening sockets** (`lsof -ti tcp:PORT -sTCP:LISTEN`). Without it lsof also reports processes holding a *client* socket to the port — including the mocha process itself, since node keeps connections alive after a test fetches from the server. `waitForPortToBeFree` read that as a foreign process squatting the port and refused to continue, failing the `after` hooks. Only a listener can actually hold a port. This was a latent bug in the helper; the new tests hit it because they fetch every referenced asset. --- Both of those merged before their review findings were addressed, so the actionable ones land here. Each was verified against the code rather than taken on trust. **`bit start` 404s on an existing local UI build (from #10629) — the important one.** `buildIfNoBundle()` treated *any* existing `public/bit` directory as a valid build, but the server now falls back to `<root>.html`, which a build made before #10629 does not contain. Reproduced end to end: with the pre-fix check the whole UI returns **404** on `/` and on deep routes; with the fix it detects the missing document, rebuilds, and serves 200. This would have hit every user upgrading past #10629 with a previously-built local UI. It now checks for the root's document rather than the directory. **Hash written for roots that were never built (from #10629).** `generateHash()` walked a hardcoded root list and threw when one was not registered. Beyond failing in a scope-only runtime, it could record a hash for a root whose document was never emitted — which reads at startup as "a pre-bundle exists" and then 404s, the same failure as above. It now walks the same registered roots `build()` turns into entries, via a new `UiMain.getUiRoots()`. **Service worker bound to a document that is not emitted (from contained `createHandlerBoundToURL("public/index.html")` while the build emits only `scope.html` / `workspace.html`. With an entry per root there is no single app shell, so `navigateFallback` is removed — the express history-api fallback already serves the right document. Verified the built service worker no longer contains that binding. **Entry name collisions (from #10629).** `Object.fromEntries` would silently keep only the last of two entries sharing a sanitized name, leaving a root with no chunks and no document while still looking built. Now throws instead. **Stats filename could break (from #10628).** `writeBundleStats` interpolated an unsanitized name into a path, so a root name containing `/` would fail with ENOENT into a swallowed debug log. Now sanitized. Not changed: the "ad-hoc chalk in `writeStats`" rule violation. That line matches the surrounding `[Rspack]` log statements in the same file; the style guide it cites covers section titles and symbols in command output, not diagnostic log lines. Happy to switch it if you'd rather be strict. The `preview/bundle-stats.ts` copy of the sanitization fix lands with (cherry picked from commit 59bd5c2)
GiladShoham
added a commit
that referenced
this pull request
Aug 19, 2026
…SSR gap Rebuilt the UI/preview pre-bundle from current source and refreshed .bundle-cache/ - UI artifact 80 MB -> 16 MB, matching upstream #10629's single-compilation dedupe now that it's reflected on this branch. Verified end to end against a real `npm run bundle` build: 16/16 UI-bundling sanity tests passing, including SSR. Total shipped distribution 216 MB / 2,933 files -> 160 MB / 2,839 files. Also documents a scope-UI SSR crash found while validating (window is not defined in useUserAgent), confirmed scoped to local --rebuild mode only - the shipped, forPreBundle-filtered pre-bundle is unaffected. Not fixed this session; tracked as a known gap. See PRs #10628, #10629, #10631 for the upstream work behind the numbers.
GiladShoham
added a commit
that referenced
this pull request
Aug 19, 2026
Adds `docs/ui-bundle-size-analysis.md` — where the remaining bundle size sits after #10628 and #10629, what I measured, and what I tried that did not work. Written to be picked up cold in a later session. > Rebased onto `master` now that #10628, #10629 and #10631 have merged; targets `master` directly. Env preview duplication is deliberately excluded — the core envs are being removed, which takes it along. ## Also in here - **Preview bundle stats.** `BIT_UI_BUNDLE_STATS=1` now covers the preview pre-bundle too, so one build produces `browser`, `scope-ssr` and `preview` stats together. It is a small copy of the UI helper rather than an import: `@teambit/ui`'s index is imported by browser code, and re-exporting a node-only module through it pulled `fs` polyfills into the UI bundle (caught by the build failing on `Can't resolve 'constants'`). - **Stats were being under-reported.** rspack's `toJson` groups assets and modules into summary rows ("assets by status") that carry a size but no name. That showed up as a single unattributable 3.3 MB / 17.9% bucket, and `assets: 0`. All `groupModulesBy*` / `groupAssetsBy*` flags are now off. - **`analyze-bundle.mjs` crashed** on assets without a `name` (those same grouped rows). - **`code-view.tsx`** imported `createElement` from the `react-syntax-highlighter` package root, which defeats its own `prism-light` import two lines later. Changed to the deep path. Behaviour-neutral, and worth stating plainly: **it saves nothing today** — see below. ## Headline findings **The two eagerly-loaded syntax highlighting registries are the biggest single item** — `highlight.js` (1.34 MB, every language) plus `refractor` (0.85 MB, every Prism language), in both the browser and ssr bundles. Neither is imported directly anywhere in this repo. They come in through `react-syntax-highlighter`'s package root, which re-exports every build including the full-language ones. The blocker is that the remaining root imports are in *published* `@teambit` components in `node_modules` (`api-reference.renderers.schema-node-member-summary`, `documenter.ui.code-snippet`), whose source is not in this repo. I verified this: fixing the in-repo imports changes the artifact by **0 bytes**. Filed separately as #10633. I tried the bundler-level workaround (alias `lowlight` → `lowlight/lib/core`, `refractor` → `refractor/core`) and **rejected it**: bit fails the build because both are transitive and would have to be declared dependencies of `@teambit/ui`, and it silently degrades any consumer relying on auto-registered languages to plain text. That is a product call, not a build one. **`lodash` is the best effort-to-reward item.** It is CJS-only (no `module` field), so it cannot be tree-shaken, and the repo has 280 `from 'lodash'` imports and zero cherry-picked ones. It is 0.52 MB of the 1.96 MB preview bundle — 28% — for six functions. Fixing it pays out in the browser, ssr and preview bundles at once. Also documented: `graphql` shipping whole into preview (28%), `sucrase` (0.47 MB) arriving via `react-live` and never lazy-loaded, `date-fns` at 302 modules, and the fact that one 6.23 MB chunk is the entire eager payload — which is what makes the cold-cache first paint slower than client-only rendering (measured in #10628). `@shikijs/langs` is 1.45 MB but already lazy-loaded per language, and is called out as the pattern the rest of the UI should copy. ## Update after the stack merged Rebased onto `master`. Two follow-ups folded in: - **`preview/bundle-stats.ts` gets the filename sanitization** that #10631 applied to its UI twin (a Qodo finding from #10628). This file only exists on this branch, which is why it was carried over rather than fixed there. - **The doc records that the service worker no longer claims navigations.** #10631 removed the `navigateFallback` that still pointed at an `index.html` the multi-entry build stopped emitting, so the analysis notes that re-adding an offline shell now has to answer what that means for two roots. Qodo reviewed this PR and found no issues. Re-validated on the rebased branch: full compile, fresh-capsule build of both bundles with `BIT_UI_BUNDLE_STATS=1`, and `analyze-bundle.mjs` reproducing the figures the doc quotes — 6.23 MB eager chunk, `@shikijs/langs` 1.45 MB / 9.5%, `highlight.js` 1.34 MB / 8.7%, and in preview `graphql` 0.52 MB / 27.9% next to `lodash` 0.52 MB / 27.7%.
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
2 participants
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.
Part 2 of #10596 — the workspace/scope dedupe. Builds both UI roots in one rspack compilation with an entry each, so the chunks they share are emitted once. Takes the shipped
@teambit/uipre-bundle from 24 MB to 16 MB.The roots are the same app
The issue estimated a small win from file-level comparison ("only 25 files / 1.9 MB byte-identical"). At the module level they are all but identical:
The one differing module is the generated root entry (
ui.root<hash>.js) — same aspect graph, different root aspect id baked in. Both theworkspaceandscopeaspects are already in both bundles; the root id only selects which one renders. So the duplicate is essentially total, and so is the saving.The 6.53 MB vendor chunk and 0.53 MB CSS are now shared; each root adds ~40 KB of its own. Combined with #10628 this is 58 MB → 16 MB.
It also lowers peak memory during
bit build— the concern raised on #10612 — since there is one module graph instead of two, and theBundleUItask got slightly faster (17s vs 18-19s).Layout
Three things had to change to support it:
.hashis now a map.shouldServeBundleUicompares a per-root hash, but there is now one bundle..hashholds one hash per root aspect id andreadBundleUiHashlooks up the root being served. A root missing from the map — or a bundle in the old layout — reads as "no pre-bundle" and falls back to a local build.index.html. With two roots in one compilation there is no single default document, so the server falls back to<root>.html. This is the sharpest edge in the PR: get the name wrong and every client-side route 404s while the SSR-rendered ones keep working. There is a test for exactly that (below).generateAssetManifesthardcodedentrypoints.main, and it is shared with the preview aspect's rspack config, so renaming was not an option. It now also emitsentrypointsByName;entrypointskeeps its old meaning for single-entry compilations, so the preview side is untouched. The SSR middleware prefers its root's entry and falls back toentrypoints.build()now builds every registered root rather than one, souiRootAspectIdOrNameonly selects the output location. That is nearly free — the module graph is shared — and it keeps one layout everywhere instead of one for the build task and another forbit start. A bare scope registers only the scope root, so it emits justscope.html.bit start --devis unaffected; the dev config is separate and still single-entry.Deletes machinery
Because the two roots bundled concurrently, #10612 had to defer closing the compilers (
openBuildCompilers+ adeferCloseoption onUiMain.build) instead of closing insidebuild(). With one compiler there is no concurrent sibling, and it collapses into a single close inbuild()'sfinally.buildIfNoBundlealso stopped constructing a whole rspack config — resolving every root's aspects — just to readoutput.path.Testing
Both roots verified in a real browser, on both paths that can serve them:
bit start --rebuild): bare scope emitsscope.htmland SSRs every route; workspace emits both htmls with the 6.53 MB vendor chunk present once.bit startloggedreturned from ui bundle cacheandbundle will be served from …/ui-bundle/public/bit, with no local build directory created. Scope SSR renders (/27 KB,/ui/button52 KB); workspace servesworkspace.htmlthrough the fallback for client-side routes.Workspace and scope home pages, component page, code tab and API reference all render with no console errors.
New e2e assertion covers change 2 —
?rendering=clientmakes the SSR middleware callnext(), which is the only way to reach the history-api fallback. Verified it catches the bug: pointing the fallback back atindex.htmlfails that test and only that test, which is precisely the failure mode described above.