test(ui): bit start sanity e2e, plus fixes for the qodo findings on #10628/#10629 - #10631
Merged
Conversation
PR Summary by QodoAdd bit start sanity E2E coverage for both UI roots
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. Root aliases force rebuilds
|
guysaar223
approved these changes
Aug 18, 2026
GiladShoham
force-pushed
the
perf/ui-bundle-single-compilation
branch
from
August 19, 2026 05:02
9659e1a to
abfe40d
Compare
GiladShoham
force-pushed
the
test/ui-start-e2e
branch
from
August 19, 2026 05:46
28f6b6e to
c8748e6
Compare
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| * version, so the assertions would describe that release instead of the code under test. | ||
| */ | ||
| (IS_WINDOWS ? describe.skip : describe)('bit start', function () { | ||
| this.timeout(0); |
There was a problem hiding this comment.
1. Startup failure hangs suite 🐞 Bug ☼ Reliability
The suite disables Mocha's timeout entirely, while HttpHelper.start() has no timeout and only settles after the exact ready message or process exit. A live process that never reaches readiness—or a stalled fetch—therefore hangs the test job indefinitely instead of producing a bounded failure.
Agent Prompt
## Issue description
The new `bit start` suite uses an unlimited Mocha timeout even though startup can remain pending forever if the child process stays alive without emitting the expected ready message. This can hang CI rather than fail the test.
## Issue Context
`HttpHelper.start()` resolves on a matching stdout message and rejects on process close, but does not enforce a startup deadline. The suite also performs multiple HTTP requests under the same unlimited timeout.
## Fix Focus Areas
- e2e/harmony/ui-start.e2e.ts[31-32]
- e2e/http-helper.ts[49-83]
ⓘ 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 c8748e6 |
GiladShoham
changed the title
| // roots shared one compilation left an `index.html` here and no `<root>.html`, and the server | ||
| // now falls back to the latter - so "the directory exists" would skip the rebuild and every | ||
| // client-side route would 404 against an output this bit can no longer serve. | ||
| if (fs.pathExistsSync(join(outputPath, getUiRootHtmlFilename(uiRootAspectId)))) return false; |
There was a problem hiding this comment.
1. Root aliases force rebuilds 🐞 Bug ➹ Performance
When --ui-root-name matches UIRoot.name, getUi() preserves that name as uiRootAspectId, but the bundle emits HTML using the registered aspect ID. The new document check therefore searches for a nonexistent name-derived file and forces a costly Rspack rebuild on every start despite a valid local build.
Agent Prompt
## Issue description
Selecting a UI root through its supported human-readable name retains the name as the tuple key. Bundle documents are named from registered aspect IDs, so `buildIfNoBundle()` never recognizes the emitted document and rebuilds on every invocation.
## Issue Context
`getUi()` should return the canonical slot registration ID even when lookup succeeds through `UIRoot.name`. Use that canonical ID consistently for document names, hashes, server fallback, and build selection.
## Fix Focus Areas
- scopes/ui-foundation/ui/ui.main.runtime.ts[534-539]
- scopes/ui-foundation/ui/ui.main.runtime.ts[723-736]
ⓘ 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 4dbb550 |
GiladShoham
enabled auto-merge (squash)
August 19, 2026 08:30
guysaar223
approved these changes
Aug 19, 2026
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.
Sanity e2e for
bit startitself, covering both UI roots. Part 3 of #10596 follow-up work.Until now nothing exercised
bit startend 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.tsstarts a real server per root and asserts, over http:/error|exception|unhandled/ito stderr — a server can listen fine with an aspect that failed to load/graphqlanswers without errors12 assertions, ~1 min. All
--rebuild, so they describe this repo's code rather than whichever bit release is installed.Two supporting changes
HttpHelpercan start either root. It was hardcoded to the bare scope (scopes.remotePath, and a ready-message string namingteambit.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.waitForPortToBeFreeread that as a foreign process squatting the port and refused to continue, failing theafterhooks. 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 start404s on an existing local UI build (from #10629) — the important one.buildIfNoBundle()treated any existingpublic/bitdirectory 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 rootsbuild()turns into entries, via a newUiMain.getUiRoots().Service worker bound to a document that is not emitted (from #10629). Confirmed in the built artifact:
service-worker.jscontainedcreateHandlerBoundToURL("public/index.html")while the build emits onlyscope.html/workspace.html. With an entry per root there is no single app shell, sonavigateFallbackis 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.fromEntrieswould 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).
writeBundleStatsinterpolated 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.tscopy of the sanitization fix lands with #10632, which is where that file lives.