CI: Split ci:normal into ci:core and framework/builder split labels#35418
Open
Sidnioulz wants to merge 24 commits into
Open
CI: Split ci:normal into ci:core and framework/builder split labels#35418Sidnioulz wants to merge 24 commits into
Sidnioulz wants to merge 24 commits into
Conversation
…d.ts generation Single rolldown pass per package with the tsgo declaration emitter replaces the per-entry child-process rollup fan-out. A hybrid resolver keeps the fast oxc resolution but resolves bare, non-external specifiers with TypeScript's module resolution, because oxc does not fall back to @types/* packages and ignores typesVersions (sxzz/rolldown-plugin-dts#130, oxc-resolver#549). Core d.ts generation drops from ~2.2 min to ~5 s. - --dts-bundler flag: rolldown-tsgo (default), rolldown (tsc emitter), rollup (legacy fallback) - --dts-resolver flag: hybrid (default), tsc, oxc - generate-source-files: enumerate globalized-runtime exports statically via rolldown - export QueryFunctions/ServiceRegistryApi/BoxOptions/LogMessageOptions used in public signatures - annotate checklistData and the react-syntax-highlighter deep import so declaration emit stays portable - delete the dead rollup dts() helper in scripts/utils/tools.ts
…mpatibility The rolldown-bundled d.ts renames chunk-level enum declarations (e.g. SupportedRenderer -> SupportedRenderer$1) when the plain name is taken by another chunk's import binding. TypeScript's cross-declaration enum compatibility is name-based, so in-repo code that mixes source-relative enum imports with dist-typed values stops type-checking. Import enums via the package specifier so both sides resolve to the same declaration in dev and production checks. Also export QueryFunctions and ServiceRegistryApi, which appear in inferred public signatures of dependent packages.
tsgo declaration emit hangs for minutes to hours on this package (likely @typescript-eslint's recursive types). Add a per-package dtsBundler override to the build config and use the tsc emitter there; it finishes in seconds.
Two clean tsgo builds are not byte-identical: type aliases flap between the alias and its expansion (e.g. TestProviderId vs string), which cascades into different chunk hash names, and one observed build wrote entry files whose chunk imports did not match the emitted chunk files (silently broken types, masked by skipLibCheck). The tsc emitter bundled by rolldown is byte-deterministic across builds, still ~14x faster than the rollup path (core: 2.2 min -> 9.1 s), and avoids the tsgo hang on eslint-plugin.
Emit each package's declarations with a single program.emit() call (noCheck, repo-root rootDir) and feed the pre-emitted tree to rolldown-plugin-dts via dtsInput. TypeScript's declaration emit is check-order-dependent, so letting the plugin emit per module in rolldown's concurrent load order produced byte-different output across runs (type aliases flapped between alias and expansion); the plugin's own build mode fixes the order but cannot handle this repo's cross-package source imports. Handwritten typings.d.ts inputs are mirrored into the emitted tree so side-effect imports resolve, and shared chunks are named chunk-[hash] because the default [name]-[hash] inherits an unstable base name. Two clean full production compiles now produce byte-identical declarations (SHA-256 over all 169 files); the full production check suite passes for all 44 projects.
The compile task's --parallel flag was still derived from the old NX agents experiment (amountOfVCPUs - 1 = 1), so the 43-project compile ran fully serially on the 8-vCPU xlarge executors of build--linux / build--windows. Peak RSS of the largest package build (core) is ~750MB, so 8 concurrent workers fit comfortably into the executor's 16GB.
Each of the 43 compile tasks paid ~0.7s for the yarn exec shim plus jiti's transform pass before doing any work. Node 22 runs .ts files natively via type stripping, which needs explicit .ts extensions on relative imports (sanctioned by the AGENTS.md jiti-to-node migration note). The build-config imports in entry-configs.ts pointed one directory too high (../../../code resolves above the repo root); jiti silently rescued them by falling back to CWD-relative resolution, which also forced @ts-ignore on every line. With the correct depth and explicit extensions TypeScript can resolve them, so the @ts-ignore lines are gone. MODULE_TYPELESS_PACKAGE_JSON warnings are disabled because the build-config files live in packages whose package.json intentionally has no "type" field; module-syntax sniffing still works, the flag only silences the log noise.
The 'Persisting to workspace' step is bytes-bound: CircleCI gzips the layer single-threaded at ~20MB/s, so the ~2.5GB of node_modules dominated the 139s step regardless of file count (a plain uncompressed tar measurably changed nothing). Pre-compressing with gzip -1 shrinks the payload ~3x, which cuts CircleCI's own compression, the upload, and the download in every one of ~100 downstream jobs. gzip is the only compressor available on all executor images; zstd is on none of them (verified via step diagnostics on cimg/node, playwright and machine images). Per-package node_modules folders only exist for packages with unhoistable dependencies, so the pack step filters them at runtime; the three root trees are passed to tar unconditionally so a missing one fails the job loudly.
The Playwright e2e-sandbox specs import scripts/utils/constants.ts and Playwright transpiles it to CommonJS, where import.meta is a syntax error - every sandbox dev/e2e job failed with 'exports is not defined in ES module scope'. Restore __dirname there and compute the repo root locally in the two build modules instead, which run as native ESM and never load under Playwright.
The check task looped ~44 workspaces sequentially, each spawning 'yarn exec jiti' (~0.7s shim + transform overhead per spawn, the same cost the compile task shed in the native-node migration). The whole TypeCheck code step ran 291s on a medium+ executor while using one of its vCPUs. The task now runs the per-package checks through a p-limit pool and launches check-package.ts with native node. The job executor moves to xlarge so eight checks run concurrently; the slowest packages (core, vue3, svelte, angular) are scheduled first so they don't stretch the tail of the run. Per-package output is buffered and only printed on failure so the parallel log stays readable. check-package.ts computes the repo root locally instead of importing scripts/utils/constants.ts, which intentionally uses __dirname (it must stay loadable from CJS-transpiled contexts like the Playwright specs) and therefore cannot load as native ESM. The scripts 'check' script moves from jiti to native node for the same reason the compile launcher did.
Every sandbox E2E pass ran with workers: 1 on CI, leaving the 3-4 vCPU Playwright executors mostly idle: 460-484s per dev-mode pass and 223-239s per static pass for the same spec files. The specs are already independent (fullyParallel was on, each test drives its own page), with one exception: change-detection.spec.ts writes to the sandbox's source files, and the resulting dev-server invalidations can reload other tests' pages mid-assertion. That spec moves to a chromium-mutating project that runs serially after the parallel chromium pass. CI now runs 3 workers per E2E step (overridable via PLAYWRIGHT_WORKERS), and the dev-mode jobs move from medium+ to large so the dev server and the workers don't starve each other - the shorter runtime more than pays for the class.
Same treatment the node_modules workspace got: the create jobs persisted the raw sandbox directory (mostly node_modules), paying CircleCI's single-threaded workspace compression on persist (31-61s per template) and again on every downstream attach. Pre-compressing with gzip -1 shrinks the payload ~3x, which cuts the create job's persist - the handoff every build/dev/e2e/vitest/chromatic job in the template's flow waits on - and the attach in each of those jobs. Downstream jobs unpack the per-template archive right after the node_modules unpack; the Windows sandbox jobs (daily workflow) get the same step, since they consume the Linux-created sandbox from the shared workspace.
The first run of the sandbox tarball broke the dev jobs of every
change-detection template two ways:
- tar extraction as root (the Playwright image) preserved the create job's
uid, unlike attach_workspace which materializes files as the current
user. git then refused to operate on the sandbox repo ('dubious
ownership'), breaking the change-detection feature and its E2E tests.
--no-same-owner restores attach_workspace semantics.
- the default gnu tar format truncates mtimes to whole seconds, which
invalidated webpack's filesystem-cache snapshots and turned the next-js
webpack dev start into a full cold compile. The e2e task's 1s dev
readiness probe then timed out, spawned a second dev server on the same
port, and crashed with EADDRINUSE when that server finished compiling.
posix (pax) format keeps sub-second mtimes.
The dev readiness probe also gets a CI-sized timeout: when a server is
already listening but still compiling its first preview bundle, waiting is
correct and spawning a second server never is. Locally the probe stays at
1s (a dead port still fails fast with ECONNREFUSED either way).
ESLint (177s), Knip (109s) and format check (79s) each paid ~50s of spin-up, checkout and workspace restore per pipeline - and the format-check job additionally ran its own 46s yarn install because it never attached the workspace. None of the three is near the workflow critical path, so they now run as steps of one 'Static checks' job on the workspace, format check first for the fastest failure signal. The standalone fmt job remains for the docs workflow, which has no build job to restore a workspace from.
--skip-nx-cache on CI was a leftover of the concluded NX Cloud agents experiment (per its own comment) and fully disconnected the run from Nx Cloud - the remote cache was neither read nor written. Removing it lets unchanged packages restore their dist from the remote cache instead of recompiling on both build--linux and build--windows. Cache correctness rests on the d.ts determinism guarantee from the rolldown-dts work (byte-identical outputs across clean runs) and on the compile target's inputs: sharedGlobals covers scripts/**/* (the whole build toolchain) and code/tsconfig.json, production covers each project's sources. Untrusted runs without an Nx Cloud token degrade to a warning and an uncached compile.
The standalone (build) job per template was ~90s of which ~55s was fixed
setup (spin-up, checkout, workspace attach and unpack) around a ~25-45s
build - and it sat between (create) and the e2e/chromatic/vitest jobs on
the workflow critical path, adding a full job hop plus a workspace
round-trip to those chains.
The static build now runs at the end of the create job, before the sandbox
is packed, so storybook-static travels inside the existing sandbox tarball
and downstream jobs need no extra restore step. e2e, chromatic, vitest and
test-runner depend on create directly.
The Windows sandbox jobs and the daily test-runner job referenced flow
jobs by array index; they now use named references, since the build job no
longer exists. saveBench('build') records identical data - it runs inside
'yarn task build' regardless of which CI job hosts it, and no job in the
generated workflows consumed the bench task output.
Even with parallel Playwright workers, the dev-mode E2E jobs are the workflow's wall-clock tail (300-377s each, starting only after their template's create job). CircleCI parallelism 2 halves the spec list per container via the existing 'circleci tests run' scaffolding - the --index/--total flags were already in place, hardcoded to a single node. Each shard starts its own dev server; change-detection.spec.ts lands on exactly one shard, where the serial chromium-mutating project isolation still applies. The smoke-test dev variant (bench templates) stays unsharded - it runs no Playwright pass.
Playwright runs dependency projects unfiltered: on a sharded dev job whose file subset contained change-detection.spec.ts, the chromium-mutating project's dependency on chromium re-ran the entire chromium suite (105 tests instead of the shard's 41), erasing the sharding win on exactly one of the two containers. The e2e task now runs two sequential playwright invocations - the parallel chromium pass, then the serial chromium-mutating pass - which preserves the isolation ordering while respecting the shard's file filter. --pass-with-no-tests covers shards whose subset has no match for one of the projects, and the mutating pass writes its junit to a -mutating suffixed file so the two reports don't overwrite each other. chromium-mutating keeps only the cheap 'setup' project as a dependency.
Four independent trims, measured on a timestamped local reproduction of angular-vite sandbox creation (107s -> 56s) and a phase decomposition of the CI create jobs (83-106s 'Create Sandbox' step): - Yarn setup becomes two file writes. installYarn2 ran 'yarn set version berry' plus chained 'yarn config set' calls: ~12 yarn boots and two network downloads (corepack's bootstrap yarn, then the latest Berry) per sandbox. The sandbox now vendors the repository's own pinned yarn release via yarnPath and writes .yarnrc.yml directly. packageManager is pinned in the sandbox package.json to the same version: without the field, corepack auto-pins its classic bootstrap yarn, and JsPackageManagerFactory would read that field and misclassify the sandbox as Yarn 1 (caught by the local reproduction; installs kept working through yarnPath delegation while the CLI ran classic-syntax commands). - The final 'yarn install' no longer wipes node_modules first. The wipe forced a full re-extraction of the tree (~10-25s on CI) that yarn's incremental install makes redundant. - Extra sandbox deps carry explicit version ranges, so addExtraDependencies does not probe the registry (a subprocess each) to resolve 'latest' for packages whose major we already know - and sandbox contents stop drifting when a new major is published. - The executor images move to cimg/node:22.22.3, matching .nvmrc. That satisfies Angular's minimum Node version, so the per-job 'node/install' step (~9s on every angular create/dev job) is gone; the create jobs also pre-warm the npx cache for the pinned gitpick version the CLI spawns, so the template download does not pay a cold npx install. Rejected while implementing: passing skip-install to the init CLI step (would fold init's install into the final one, ~15-25s) silently no-ops all addon configuration, because postinstallAddon resolves each addon's postinstall hook from the sandbox's node_modules and returns quietly when resolution fails - addon-vitest's setup would disappear from sandboxes.
The readiness probe fetched /iframe.html and re-ran the dev task on any failure. But the fetch can fail while a dev server owns the port - a cold compile outlasting the timeout, or the server resetting connections before its middleware is up (seen on a sveltekit shard: the probe got a reset, the task spawned a second server, and that server crashed the job with EADDRINUSE once its own compile finished). A held port now counts as ready via a TCP-connect fallback; every consumer of the dev task performs its own HTTP wait before using the server, so this cannot mask a dead server. This also replaces the earlier CI-only 180s probe timeout with behavior that is correct locally too.
Sandbox creation spawned ~24 package-manager subprocesses just to resolve versions, measured via a timestamped local reproduction of angular-vite sandbox creation and confirmed in the CI create-job logs: - latestVersion() probed the registry for every storybook package even though the CLI ships the authoritative version map (versions.ts) and the sandbox registry (verdaccio) serves exactly those versions. Inside sandbox creation (IN_STORYBOOK_SANDBOX, set by the monorepo task runner and never by real user installs) the probe is now short-circuited to the versions map. On CI the resulting specifiers are byte-identical; in link-mode local sandboxes storybook packages get ^version instead of an exact pin, where the linked workspace overrides resolution anyway. - addDependencies(skipInstall) resolved a version for every dependency serially - including dependencies whose specifier already carried one, discarding the result. Lookups now only happen for unversioned specifiers and run concurrently, with insertion order kept deterministic. This also speeds up real 'storybook init' runs. - getInstalledVersion() spawned a full dependency-graph walk per package (yarn info --recursive), which exits 1 with usage output for packages that are not installed at all - addon-vitest probes vitest, msw and coverage packages this way on every sandbox. The module's own package.json is now resolved first (a plain fs read, PnP included), and the graph walk only runs as a fallback for declared dependencies that don't resolve directly (e.g. pnpm virtual store layouts). - The 'sb repro' command probed npm for latest/next before downloading the template; inside sandbox creation those two roundtrips only feed the 'you are behind' warning copy and are skipped. The proxy unit tests asserting registry-probe behavior now stub IN_STORYBOOK_SANDBOX off explicitly - the repo-level .env marks all vitest runs as sandbox context - and a new test pins the short-circuit contract. Measured locally (angular-vite, full creation): 24 -> 8 version-probe spawns; 56s -> 48s on top of the CI-side trims, 107s cumulative baseline.
Adds composable CI labels so PRs can run a much smaller, faster pipeline than ci:normal: - ci:core runs the internal Storybook e2e tests and every non-sandbox-specific job (build, unit/story tests, static checks, chromatic, benchmarks, test-storybooks, init-empty) - i.e. the normal workflow minus all sandbox jobs. - ci:<framework> (ci:react, ci:vue3, ci:angular, ci:svelte, ci:html, ci:preact, ci:web-components, ci:solid) runs ALL sandboxes for that framework, drawn from the complete (daily) template pool. - ci:<builder> (ci:vite, ci:webpack, ci:rsbuild) runs ALL sandboxes for that builder. Split labels compose: the trigger workflow joins them into a single `+`-separated workflow value (e.g. core+react) so one pipeline - and one completion status check - covers the union of the selected atoms. The legacy cadence labels (ci:normal/merged/daily/docs) still work unchanged and take precedence over split labels when both are present; the dangerfile now rejects that mix and otherwise accepts split labels in place of a cadence label. The CircleCI `workflow` pipeline parameter becomes a plain string (validated in scripts/ci/main.ts, which fails setup loudly on unknown atoms). Framework/builder atoms are derived from each sandbox template's expected renderer/builder metadata, with both Angular packages mapping to the single `angular` atom. Legacy workflows generate byte-identical configs apart from the parameter type change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDwFN1kYSqVTZah1BvKQKu
Sidnioulz
changed the title
|
View your CI Pipeline Execution ↗ for commit ab81b91
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.95 MB | 21.11 MB | 🎉 -835 KB 🎉 |
| Dependency size | 36.44 MB | 36.44 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/angular-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 32 | 32 | 0 |
| Self size | 22.38 MB | 2.40 MB | 🎉 -19.99 MB 🎉 |
| Dependency size | 20.07 MB | 40.06 MB | 🚨 +19.99 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 15 KB | 13 KB | 🎉 -2 KB 🎉 |
| Dependency size | 30.92 MB | 30.92 MB | 🚨 +812 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 533 | 533 | 0 |
| Self size | 663 KB | 640 KB | 🎉 -23 KB 🎉 |
| Dependency size | 62.29 MB | 62.26 MB | 🎉 -34 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 93 | 93 | 0 |
| Self size | 1.39 MB | 1.37 MB | 🎉 -22 KB 🎉 |
| Dependency size | 24.01 MB | 23.97 MB | 🎉 -38 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 122 | 122 | 0 |
| Self size | 30 KB | 29 KB | 🎉 -988 B 🎉 |
| Dependency size | 25.08 MB | 25.04 MB | 🎉 -38 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 83 | 83 | 0 |
| Self size | 36 KB | 32 KB | 🎉 -4 KB 🎉 |
| Dependency size | 21.78 MB | 21.75 MB | 🎉 -34 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 274 | 0 |
| Self size | 23 KB | 23 KB | 🎉 -777 B 🎉 |
| Dependency size | 48.07 MB | 48.04 MB | 🎉 -34 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 84 | 84 | 0 |
| Self size | 110 KB | 104 KB | 🎉 -5 KB 🎉 |
| Dependency size | 21.82 MB | 21.78 MB | 🎉 -38 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 821 KB | 821 KB | 🚨 +97 B 🚨 |
| Dependency size | 91.94 MB | 91.11 MB | 🎉 -835 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +79 B 🚨 |
| Dependency size | 90.42 MB | 89.59 MB | 🎉 -835 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🚨 +66 B 🚨 |
| Dependency size | 58.39 MB | 57.55 MB | 🎉 -835 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 59 | 59 | 0 |
| Self size | 1.52 MB | 1.49 MB | 🎉 -35 KB 🎉 |
| Dependency size | 12.45 MB | 12.45 MB | 🚨 +103 B 🚨 |
| Bundle Size Analyzer | Link | Link |
Contributor
📝 WalkthroughWalkthroughThis PR adds "split" CI label support (ci:core plus framework/builder atoms) alongside cadence labels across docs, GitHub Actions, CircleCI config, and dangerfile validation; reworks CI job workspace packing/restore and sandbox job dependencies; adds sandbox-environment fast-paths in package manager/CLI code; migrates type-declaration and manager-globals generation to rolldown; and applies various build-script import, dependency, and task-parallelism changes. ChangesSplit CI Label Workflow Support
CI Sandbox Job Rework
Sandbox Environment Fast-Paths
Rolldown-Based Build Tooling
CI Task Performance and Misc Cleanups
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions as GitHub Actions
participant CircleCI
participant MainTS as scripts/ci/main.ts
participant Sandboxes as scripts/ci/sandboxes.ts
GitHubActions->>GitHubActions: filter ci: labels, exclude cadence labels
GitHubActions->>GitHubActions: sort and join atoms with '+'
GitHubActions->>CircleCI: trigger pipeline with workflow param (e.g. core+react)
CircleCI->>MainTS: run config generator with --workflow param
MainTS->>MainTS: parseWorkflowAtoms(workflowParam)
loop for each atom
MainTS->>Sandboxes: getSplitSandboxes(atom) / getSandboxes()
Sandboxes-->>MainTS: job list for atom
end
MainTS->>MainTS: flatten and dedupe jobs via ensureRequiredJobs
MainTS-->>CircleCI: emit combined workflow config
sequenceDiagram
participant CreateJob as createJob
participant Workspace as workspace helper
participant DevJob as devJob/e2e/vitest/test-runner
participant Playwright
CreateJob->>CreateJob: Build storybook
CreateJob->>Workspace: packSandbox(id)
CreateJob->>Workspace: persist(sandboxArchive(id))
DevJob->>Workspace: restoreLinux({sandboxId: id})
Workspace->>Workspace: unpack node_modules archive
Workspace->>Workspace: unpackSandbox(id)
DevJob->>Playwright: getPlaywrightNodeSetupSteps(template)
Playwright->>Playwright: run chromium + chromium-mutating projects
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
code/core/src/shared/checklist-store/checklistData.tsx (1)
161-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant with trailing
as const satisfies ChecklistData.With the explicit
: ChecklistDataannotation, the value's exposed type is now always widened toChecklistData, making the trailingas const satisfies ChecklistDataassertion effectively redundant for type-checking purposes (though it likely remains needed for isolated-declaration-style explicit typing on the exported const). Consider dropping one of the two to avoid confusion about which is authoritative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/shared/checklist-store/checklistData.tsx` at line 161, The export of checklistData is using both an explicit ChecklistData annotation and a trailing as const satisfies ChecklistData, which is redundant and confusing about the source of truth. In checklistData, choose a single typing approach for the exported const and remove the other, keeping the remaining form consistent with how the object is meant to be inferred and checked.scripts/tasks/e2e-tests-build.ts (1)
45-50: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueStatic analysis flags shell-interpolated Playwright commands.
OpenGrep flags the dynamic
exec()command strings (built with template interpolation oftestFiles) as command-injection risk. In this dev/build-task contexttestFilescomes from the script's ownprocess.argv, not untrusted remote input, so exploitability is low — but the same interpolation is also fragile for correctness: any test path containing a space would break the shell-quoted command since it isn't quoted. Usingexeca's array-args form (orexecFile) would sidestep both concerns.Also applies to: 65-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tasks/e2e-tests-build.ts` around lines 45 - 50, The dynamic Playwright command built in e2e-tests-build.ts via exec() is shell-interpolating testFiles, which is both flagged by static analysis and fragile for paths with spaces. Update the command construction in the affected branch and the other matching block(s) so it uses a non-shell invocation style such as execa with argument arrays or execFile, and pass each test file as a separate argument instead of joining them into one string.Source: Linters/SAST tools
scripts/ci/utils/helpers.ts (1)
39-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pack/packSandboxrely on an implicit tar default archive target.Both
tar --create ... | gzip -1 > filecommands omit-f/--file, relying on tar writing to stdout by default. This works on Debian/Ubuntu-based images (likecimg/node) because their GNU tar build defaults the archive target to-(stdin/stdout) whenTAPEis unset, but this is a distro packaging convention, not documented/portable GNU tar behavior — vanilla GNU tar's compiled-in default is typically a tape device path, which would hang or error. Since this only matters if an executor image ever changes, it's low risk today, but adding an explicit-f -removes the implicit dependency on that convention.🔧 Make the archive target explicit
- `tar --create ${requiredPaths.join(' ')} $optional | gzip -1 > ${PACKED_NODE_MODULES_ARCHIVE}`, + `tar --create --file - ${requiredPaths.join(' ')} $optional | gzip -1 > ${PACKED_NODE_MODULES_ARCHIVE}`,- command: `tar --create --format=posix --pax-option=delete=atime,delete=ctime ${SANDBOX_DIR}/${id} | gzip -1 > ${sandboxArchive(id)}`, + command: `tar --create --file - --format=posix --pax-option=delete=atime,delete=ctime ${SANDBOX_DIR}/${id} | gzip -1 > ${sandboxArchive(id)}`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/utils/helpers.ts` around lines 39 - 92, The tar commands in pack and packSandbox rely on tar’s implicit stdout default instead of explicitly targeting standard output. Update the command construction in helpers.ts for pack and packSandbox to use an explicit tar archive target via the relevant tar invocation so the behavior does not depend on distro-specific GNU tar defaults. Keep the existing requiredPaths, optionalPaths, and sandboxArchive(id) flow unchanged; only make the archive destination explicit in those two helpers.scripts/ci/common-jobs.ts (1)
26-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile substring replace on
packageDirs; consider anchoring and deduplicating.
p.replace('src', 'node_modules')/p.replace('src', 'dist')replace only the first occurrence of the literal substring"src". SincepackageDirsentries come from a*/srcor*/*/srcglob,srcis always the trailing path segment — but this replaces wherever"src"first appears in the string, so a future package directory containingsrcelsewhere in its name (e.g.some-src-thing/src) would silently produce a wrong path instead of the intendednode_modules/distdirectory. This pattern is duplicated in bothbuild_linux(lines 73, 77) andbuild_windows(lines 122-125).♻️ Anchor the replacement to the trailing `src` segment and share a helper
+const toNodeModules = (p: string) => p.replace(/\/src$/, '/node_modules'); +const toDist = (p: string) => p.replace(/\/src$/, '/dist'); + const packageDirs = glob.sync(['*/src', '*/*/src'], { cwd: join(dirname, '../../code'), onlyDirectories: true, });- packageDirs.map((p) => `${WORKING_DIR}/code/${p.replace('src', 'node_modules')}`) + packageDirs.map((p) => `${WORKING_DIR}/code/${toNodeModules(p)}`) ), workspace.persist([ PACKED_NODE_MODULES_ARCHIVE, - ...packageDirs.map((p) => `${WORKING_DIR}/code/${p.replace('src', 'dist')}`), + ...packageDirs.map((p) => `${WORKING_DIR}/code/${toDist(p)}`),- ...packageDirs.flatMap((p) => [ - `code/${p.replace('src', 'dist')}`, - `code/${p.replace('src', 'node_modules')}`, - ]), + ...packageDirs.flatMap((p) => [`code/${toDist(p)}`, `code/${toNodeModules(p)}`]),Also applies to: 59-77, 122-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/common-jobs.ts` around lines 26 - 30, The path derivation for package directories is using fragile substring replacement on packageDirs, which can replace the wrong “src” segment and is duplicated across build_linux and build_windows. Update the logic in scripts/ci/common-jobs.ts to anchor replacements to the trailing src path segment instead of using a generic string replace, and extract a shared helper used by both build_linux and build_windows so the node_modules/dist path mapping is consistent and deduplicated.scripts/package.json (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
--disable-warning=MODULE_TYPELESS_PACKAGE_JSONfor consistency.
nx.json'scompiletarget's near-identical node-executed.tsinvocation includes--disable-warning=MODULE_TYPELESS_PACKAGE_JSON, but thischeckscript does not. Ifcheck-package.tsalso loads package.json files without a"type"field, this could emit the same experimental warning unsuppressed here.Proposed fix
- "check": "node ./check/check-package.ts", + "check": "node --disable-warning=MODULE_TYPELESS_PACKAGE_JSON ./check/check-package.ts",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/package.json` at line 10, The check script is missing the same Node warning suppression used by the compile target, so update the `check` entry in `scripts/package.json` to pass `--disable-warning=MODULE_TYPELESS_PACKAGE_JSON` when invoking `check-package.ts`. Keep the change aligned with the existing `nx.json` compile command so both Node-executed TypeScript paths behave consistently.code/core/scripts/generate-source-files.ts (1)
123-164: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the Rolldown bundle after generation.
bundle.generate()leaves the bundler open; wrap it intry/finallyand callawait bundle.close()so native resources are released and the script can exit cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/scripts/generate-source-files.ts` around lines 123 - 164, The Rolldown bundle created in the source-generation flow is never closed after calling bundle.generate(), so the script can leave resources open. Update the logic around the bundle variable in generate-source-files.ts to wrap generation in a try/finally and call await bundle.close() in the finally block, while keeping the existing export extraction and require() fallback behavior unchanged.code/lib/cli-storybook/src/sandbox.ts (1)
45-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
insideSandboxTaskfor the laterIN_STORYBOOK_SANDBOXcheck.Logic is correct. Minor nit:
insideSandboxTaskis computed here but the sameprocess.env.IN_STORYBOOK_SANDBOXcheck is re-evaluated later (line 238:!optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX)) instead of reusing this variable, causing minor duplication within the same function scope.♻️ Proposed fix
await initiate({ - dev: isCI() && !optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX), + dev: isCI() && !insideSandboxTask,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/cli-storybook/src/sandbox.ts` around lines 45 - 61, The sandbox version lookup logic is duplicating the `IN_STORYBOOK_SANDBOX` check within the same `sandbox.ts` flow. Reuse the existing `insideSandboxTask` variable wherever the later sandbox condition is needed instead of calling `optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX)` again, keeping the decision centralized in the same scope around the version-fetching code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/build-package.ts`:
- Around line 39-58: `--dts-bundler` is not validated, so invalid values can
silently fall back to the legacy rollup path instead of failing fast. Add an
explicit allowed-values check for `dtsBundler` in `build-package.ts`, mirroring
the existing `dtsResolver` validation, and throw a clear error when
`entry.dtsBundler` or the CLI flag is anything other than the supported bundler
names. Keep the validation close to the `dtsResolver` guard so the `switch` that
selects the bundler only runs after both options are confirmed valid.
In `@scripts/build/utils/generate-types-rolldown.ts`:
- Around line 168-177: The handwritten declaration copy loop in
generate-types-rolldown.ts silently falls back to an empty string when
ts.sys.readFile fails, which can create an empty .d.ts instead of surfacing a
failure. Update the file-copy logic in the parsed.fileNames loop to explicitly
handle unreadable/missing declaration files by throwing or logging a clear error
before writing, and keep the existing .d.ts detection and target path handling
intact.
- Around line 109-167: The declaration generation flow in
emitPackageDeclarations currently only prints diagnostics after program.emit()
and can still continue with partial .d.ts output, so update this function to
fail the build whenever result.diagnostics contains any errors. After collecting
the errors, throw an exception instead of just logging, keeping the existing
written-output check as a separate guard so compile stops on type issues.
In `@scripts/tasks/dev.ts`:
- Around line 68-75: The fallback socket in the connect check can still hang
during the TCP handshake because the current timeout approach does not cover
pre-connect stalls. Update the Promise in the connection helper to use a manual
timer around the socket created by connect, and on expiry destroy the socket and
resolve false; keep the existing connect and error handlers in the same helper
so the timeout is cleared when the socket connects or fails normally.
In `@scripts/tasks/e2e-tests-build.ts`:
- Around line 54-78: The sequential loop in e2e-tests-build.ts currently stops
at the first failing `exec()` call, so a failing `chromium` run prevents
`chromium-mutating` from running and hides its results. Update the `for (const
[project, junitSuffix] of ...)` flow to always execute both Playwright projects,
even if one fails, and then surface a failure at the end if either invocation
failed. Keep the existing `exec()` behavior in mind and add error aggregation or
per-iteration handling around the `exec` calls so both project results are
preserved.
---
Nitpick comments:
In `@code/core/scripts/generate-source-files.ts`:
- Around line 123-164: The Rolldown bundle created in the source-generation flow
is never closed after calling bundle.generate(), so the script can leave
resources open. Update the logic around the bundle variable in
generate-source-files.ts to wrap generation in a try/finally and call await
bundle.close() in the finally block, while keeping the existing export
extraction and require() fallback behavior unchanged.
In `@code/core/src/shared/checklist-store/checklistData.tsx`:
- Line 161: The export of checklistData is using both an explicit ChecklistData
annotation and a trailing as const satisfies ChecklistData, which is redundant
and confusing about the source of truth. In checklistData, choose a single
typing approach for the exported const and remove the other, keeping the
remaining form consistent with how the object is meant to be inferred and
checked.
In `@code/lib/cli-storybook/src/sandbox.ts`:
- Around line 45-61: The sandbox version lookup logic is duplicating the
`IN_STORYBOOK_SANDBOX` check within the same `sandbox.ts` flow. Reuse the
existing `insideSandboxTask` variable wherever the later sandbox condition is
needed instead of calling
`optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX)` again, keeping the
decision centralized in the same scope around the version-fetching code.
In `@scripts/ci/common-jobs.ts`:
- Around line 26-30: The path derivation for package directories is using
fragile substring replacement on packageDirs, which can replace the wrong “src”
segment and is duplicated across build_linux and build_windows. Update the logic
in scripts/ci/common-jobs.ts to anchor replacements to the trailing src path
segment instead of using a generic string replace, and extract a shared helper
used by both build_linux and build_windows so the node_modules/dist path mapping
is consistent and deduplicated.
In `@scripts/ci/utils/helpers.ts`:
- Around line 39-92: The tar commands in pack and packSandbox rely on tar’s
implicit stdout default instead of explicitly targeting standard output. Update
the command construction in helpers.ts for pack and packSandbox to use an
explicit tar archive target via the relevant tar invocation so the behavior does
not depend on distro-specific GNU tar defaults. Keep the existing requiredPaths,
optionalPaths, and sandboxArchive(id) flow unchanged; only make the archive
destination explicit in those two helpers.
In `@scripts/package.json`:
- Line 10: The check script is missing the same Node warning suppression used by
the compile target, so update the `check` entry in `scripts/package.json` to
pass `--disable-warning=MODULE_TYPELESS_PACKAGE_JSON` when invoking
`check-package.ts`. Keep the change aligned with the existing `nx.json` compile
command so both Node-executed TypeScript paths behave consistently.
In `@scripts/tasks/e2e-tests-build.ts`:
- Around line 45-50: The dynamic Playwright command built in e2e-tests-build.ts
via exec() is shell-interpolating testFiles, which is both flagged by static
analysis and fragile for paths with spaces. Update the command construction in
the affected branch and the other matching block(s) so it uses a non-shell
invocation style such as execa with argument arrays or execFile, and pass each
test file as a separate argument instead of joining them into one string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c78307ec-919f-409c-a99d-1c36fe244376
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (51)
.agents/skills/open-pr/SKILL.md.agents/skills/pr/SKILL.md.circleci/config.yml.github/PULL_REQUEST_TEMPLATE.md.github/workflows/trigger-circle-ci-workflow.yml.gitignorecode/core/package.jsoncode/core/scripts/generate-source-files.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/core/src/common/js-package-manager/NPMProxy.test.tscode/core/src/common/js-package-manager/PNPMProxy.test.tscode/core/src/common/js-package-manager/Yarn1Proxy.test.tscode/core/src/common/js-package-manager/Yarn2Proxy.test.tscode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/core-server/server-channel/file-search-channel.test.tscode/core/src/manager/globals/globals.tscode/core/src/node-logger/index.tscode/core/src/node-logger/logger/logger.tscode/core/src/shared/checklist-store/checklistData.tsxcode/core/src/shared/open-service/index.tscode/lib/cli-storybook/src/sandbox.tscode/lib/eslint-plugin/build-config.tscode/playwright.config.tsnx.jsonscripts/build/build-package.tsscripts/build/entry-configs.tsscripts/build/utils/dts-process.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-bundle.tsscripts/build/utils/generate-package-json.tsscripts/build/utils/generate-types-rolldown.tsscripts/build/utils/generate-types.tsscripts/check/check-package.tsscripts/ci/common-jobs.tsscripts/ci/main.tsscripts/ci/sandboxes.tsscripts/ci/utils/executors.tsscripts/ci/utils/helpers.tsscripts/ci/utils/parameters.tsscripts/ci/utils/types.tsscripts/dangerfile.jsscripts/package.jsonscripts/tasks/check.tsscripts/tasks/compile.tsscripts/tasks/dev.tsscripts/tasks/e2e-tests-build.tsscripts/tasks/sandbox-parts.tsscripts/tasks/sandbox.tsscripts/utils/constants.tsscripts/utils/tools.tsscripts/utils/yarn.ts
Comment on lines
39
to
+58
| options: { | ||
| prod: { type: 'boolean', default: false }, | ||
| production: { type: 'boolean', default: false }, | ||
| optimized: { type: 'boolean', default: false }, | ||
| watch: { type: 'boolean', default: false }, | ||
| cwd: { type: 'string' }, | ||
| // 'rolldown' bundles tsc-emitted declarations: ~14x faster than the old | ||
| // rollup path and byte-deterministic. 'rolldown-tsgo' is ~2x faster still, | ||
| // but tsgo declaration emit is not yet deterministic (type aliases flap, | ||
| // chunk references can go stale) and hangs on some packages; keep it | ||
| // opt-in until it stabilizes. | ||
| 'dts-bundler': { type: 'string', default: 'rolldown' }, | ||
| 'dts-resolver': { type: 'string', default: 'hybrid' }, | ||
| }, | ||
| allowNegative: true, | ||
| }); | ||
|
|
||
| if (dtsResolver !== 'tsc' && dtsResolver !== 'oxc' && dtsResolver !== 'hybrid') { | ||
| throw new Error(`Invalid --dts-resolver: ${dtsResolver} (expected 'hybrid', 'tsc' or 'oxc')`); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--dts-bundler isn't validated like --dts-resolver, so typos silently fall back to the legacy rollup path.
dtsResolver is validated against the three allowed values and throws on anything else (lines 56-58), but dtsBundler has no equivalent check. A typo such as --dts-bundler=roll-down (or a bad value coming from entry.dtsBundler) would silently hit the default case in the switch below and use the slower legacy rollup generator instead of failing fast, which could mask a misconfiguration for a long time.
🛡️ Proposed fix mirroring the dts-resolver validation
+if (dtsBundler !== 'rolldown' && dtsBundler !== 'rolldown-tsgo' && dtsBundler !== 'rollup') {
+ throw new Error(
+ `Invalid --dts-bundler: ${dtsBundler} (expected 'rolldown', 'rolldown-tsgo' or 'rollup')`
+ );
+}
+
if (dtsResolver !== 'tsc' && dtsResolver !== 'oxc' && dtsResolver !== 'hybrid') {
throw new Error(`Invalid --dts-resolver: ${dtsResolver} (expected 'hybrid', 'tsc' or 'oxc')`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| options: { | |
| prod: { type: 'boolean', default: false }, | |
| production: { type: 'boolean', default: false }, | |
| optimized: { type: 'boolean', default: false }, | |
| watch: { type: 'boolean', default: false }, | |
| cwd: { type: 'string' }, | |
| // 'rolldown' bundles tsc-emitted declarations: ~14x faster than the old | |
| // rollup path and byte-deterministic. 'rolldown-tsgo' is ~2x faster still, | |
| // but tsgo declaration emit is not yet deterministic (type aliases flap, | |
| // chunk references can go stale) and hangs on some packages; keep it | |
| // opt-in until it stabilizes. | |
| 'dts-bundler': { type: 'string', default: 'rolldown' }, | |
| 'dts-resolver': { type: 'string', default: 'hybrid' }, | |
| }, | |
| allowNegative: true, | |
| }); | |
| if (dtsResolver !== 'tsc' && dtsResolver !== 'oxc' && dtsResolver !== 'hybrid') { | |
| throw new Error(`Invalid --dts-resolver: ${dtsResolver} (expected 'hybrid', 'tsc' or 'oxc')`); | |
| } | |
| options: { | |
| prod: { type: 'boolean', default: false }, | |
| production: { type: 'boolean', default: false }, | |
| optimized: { type: 'boolean', default: false }, | |
| watch: { type: 'boolean', default: false }, | |
| cwd: { type: 'string' }, | |
| // 'rolldown' bundles tsc-emitted declarations: ~14x faster than the old | |
| // rollup path and byte-deterministic. 'rolldown-tsgo' is ~2x faster still, | |
| // but tsgo declaration emit is not yet deterministic (type aliases flap, | |
| // chunk references can go stale) and hangs on some packages; keep it | |
| // opt-in until it stabilizes. | |
| 'dts-bundler': { type: 'string', default: 'rolldown' }, | |
| 'dts-resolver': { type: 'string', default: 'hybrid' }, | |
| }, | |
| allowNegative: true, | |
| }); | |
| if (dtsBundler !== 'rolldown' && dtsBundler !== 'rolldown-tsgo' && dtsBundler !== 'rollup') { | |
| throw new Error( | |
| `Invalid --dts-bundler: ${dtsBundler} (expected 'rolldown', 'rolldown-tsgo' or 'rollup')` | |
| ); | |
| } | |
| if (dtsResolver !== 'tsc' && dtsResolver !== 'oxc' && dtsResolver !== 'hybrid') { | |
| throw new Error(`Invalid --dts-resolver: ${dtsResolver} (expected 'hybrid', 'tsc' or 'oxc')`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build/build-package.ts` around lines 39 - 58, `--dts-bundler` is not
validated, so invalid values can silently fall back to the legacy rollup path
instead of failing fast. Add an explicit allowed-values check for `dtsBundler`
in `build-package.ts`, mirroring the existing `dtsResolver` validation, and
throw a clear error when `entry.dtsBundler` or the CLI flag is anything other
than the supported bundler names. Keep the validation close to the `dtsResolver`
guard so the `switch` that selects the bundler only runs after both options are
confirmed valid.
Comment on lines
+109
to
+167
| function emitPackageDeclarations(wrapperTsconfig: string, outDir: string): void { | ||
| const parsed = ts.getParsedCommandLineOfConfigFile(wrapperTsconfig, undefined, { | ||
| ...ts.sys, | ||
| onUnRecoverableConfigFileDiagnostic: (diagnostic) => { | ||
| // eslint-disable-next-line local-rules/no-uncategorized-errors | ||
| throw new Error(ts.flattenDiagnosticMessageText(diagnostic.messageText, '\n')); | ||
| }, | ||
| }); | ||
| if (!parsed) { | ||
| // eslint-disable-next-line local-rules/no-uncategorized-errors | ||
| throw new Error(`Unable to parse ${wrapperTsconfig}`); | ||
| } | ||
|
|
||
| const program = ts.createProgram({ | ||
| rootNames: parsed.fileNames, | ||
| options: { | ||
| ...parsed.options, | ||
| noEmit: false, | ||
| noCheck: true, | ||
| declaration: true, | ||
| emitDeclarationOnly: true, | ||
| declarationMap: false, | ||
| sourceMap: false, | ||
| composite: false, | ||
| incremental: false, | ||
| skipLibCheck: true, | ||
| noEmitOnError: false, | ||
| stripInternal: true, | ||
| // Source files import with explicit .ts extensions; emitted d.ts must | ||
| // reference .js so that consumers (and the bundling pass) resolve them. | ||
| allowImportingTsExtensions: true, | ||
| rewriteRelativeImportExtensions: true, | ||
| outDir, | ||
| rootDir: DIR_ROOT, | ||
| }, | ||
| }); | ||
|
|
||
| let written = 0; | ||
| const result = program.emit(undefined, (fileName, text, writeByteOrderMark) => { | ||
| written++; | ||
| ts.sys.writeFile(fileName, text, writeByteOrderMark); | ||
| }); | ||
| const errors = result.diagnostics.filter((d) => d.category === ts.DiagnosticCategory.Error); | ||
| if (errors.length > 0) { | ||
| console.error( | ||
| ts.formatDiagnosticsWithColorAndContext(errors, { | ||
| getCurrentDirectory: () => DIR_ROOT, | ||
| getCanonicalFileName: (fileName) => fileName, | ||
| getNewLine: () => ts.sys.newLine, | ||
| }) | ||
| ); | ||
| } | ||
| // `emitSkipped` is unreliable with `noCheck` (it reports true even though | ||
| // every file was written), so gate on actual output instead. | ||
| if (written === 0) { | ||
| // eslint-disable-next-line local-rules/no-uncategorized-errors | ||
| throw new Error(`Declaration emit produced no output for ${wrapperTsconfig}`); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and the legacy generator.
ast-grep outline scripts/build/utils/generate-types-rolldown.ts --view expanded || true
ast-grep outline scripts/build/utils/generate-types.ts --view expanded || true
# Read the relevant sections with line numbers.
sed -n '1,240p' scripts/build/utils/generate-types-rolldown.ts | cat -n
printf '\n--- legacy ---\n'
sed -n '1,240p' scripts/build/utils/generate-types.ts | cat -n
# Find where these generators are invoked and whether typecheck is enforced elsewhere.
rg -n "generate-types-rolldown|generate-types\.ts|typecheck|tsc --noEmit|noEmitOnError|emitDeclarationOnly|rewriteRelativeImportExtensions" scripts package.json .github . -g '!**/node_modules/**'Repository: storybookjs/storybook
Length of output: 29204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' scripts/build/build-package.ts | cat -n
printf '\n--- dts-process ---\n'
sed -n '1,220p' scripts/build/utils/dts-process.ts | cat -n
printf '\n--- rolldown tail ---\n'
sed -n '179,360p' scripts/build/utils/generate-types-rolldown.ts | cat -nRepository: storybookjs/storybook
Length of output: 13360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Root and package scripts.
sed -n '1,260p' package.json | cat -n
printf '\n--- build-package references ---\n'
rg -n "build-package\.ts|generateTypesFilesRolldown|generateTypesFiles\(" scripts package.json .github -g '!**/node_modules/**'
printf '\n--- build/typecheck scripts ---\n'
rg -n "\"build\"|\"typecheck\"|\"check\"" package.json code/**/package.json scripts/**/package.json nx.json .github/workflows -g '!**/node_modules/**'Repository: storybookjs/storybook
Length of output: 5361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' scripts/package.json | cat -n
printf '\n--- nx targets ---\n'
sed -n '1,180p' nx.json | cat -nRepository: storybookjs/storybook
Length of output: 16903
Fail the build on declaration-emit errors
program.emit() can log errors and still write output here, which lets compile succeed with partial or incorrect .d.ts files. Throw when errors.length > 0 so production builds stop on type errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build/utils/generate-types-rolldown.ts` around lines 109 - 167, The
declaration generation flow in emitPackageDeclarations currently only prints
diagnostics after program.emit() and can still continue with partial .d.ts
output, so update this function to fail the build whenever result.diagnostics
contains any errors. After collecting the errors, throw an exception instead of
just logging, keeping the existing written-output check as a separate guard so
compile stops on type issues.
Comment on lines
+168
to
+177
| // Handwritten declaration files (e.g. `typings.d.ts`) are program inputs, | ||
| // not outputs, so tsc does not place them in outDir; copy them so that | ||
| // side-effect imports like `import './typings.d.ts'` resolve in the tree. | ||
| for (const fileName of parsed.fileNames) { | ||
| if (/\.d\.[cm]?ts$/.test(fileName)) { | ||
| const target = join(outDir, relative(DIR_ROOT, fileName)); | ||
| ts.sys.writeFile(target, ts.sys.readFile(fileName) ?? ''); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent empty-file fallback when copying handwritten .d.ts files.
ts.sys.readFile(fileName) ?? '' writes an empty file if the handwritten declaration can't be read, instead of failing. This could silently produce an empty .d.ts (breaking import './typings.d.ts' side-effect imports) rather than surfacing a clear error about a missing/unreadable source file.
🐛 Proposed fix
for (const fileName of parsed.fileNames) {
if (/\.d\.[cm]?ts$/.test(fileName)) {
const target = join(outDir, relative(DIR_ROOT, fileName));
- ts.sys.writeFile(target, ts.sys.readFile(fileName) ?? '');
+ const content = ts.sys.readFile(fileName);
+ if (content === undefined) {
+ // eslint-disable-next-line local-rules/no-uncategorized-errors
+ throw new Error(`Unable to read handwritten declaration file: ${fileName}`);
+ }
+ ts.sys.writeFile(target, content);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handwritten declaration files (e.g. `typings.d.ts`) are program inputs, | |
| // not outputs, so tsc does not place them in outDir; copy them so that | |
| // side-effect imports like `import './typings.d.ts'` resolve in the tree. | |
| for (const fileName of parsed.fileNames) { | |
| if (/\.d\.[cm]?ts$/.test(fileName)) { | |
| const target = join(outDir, relative(DIR_ROOT, fileName)); | |
| ts.sys.writeFile(target, ts.sys.readFile(fileName) ?? ''); | |
| } | |
| } | |
| } | |
| // Handwritten declaration files (e.g. `typings.d.ts`) are program inputs, | |
| // not outputs, so tsc does not place them in outDir; copy them so that | |
| // side-effect imports like `import './typings.d.ts'` resolve in the tree. | |
| for (const fileName of parsed.fileNames) { | |
| if (/\.d\.[cm]?ts$/.test(fileName)) { | |
| const target = join(outDir, relative(DIR_ROOT, fileName)); | |
| const content = ts.sys.readFile(fileName); | |
| if (content === undefined) { | |
| // eslint-disable-next-line local-rules/no-uncategorized-errors | |
| throw new Error(`Unable to read handwritten declaration file: ${fileName}`); | |
| } | |
| ts.sys.writeFile(target, content); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build/utils/generate-types-rolldown.ts` around lines 168 - 177, The
handwritten declaration copy loop in generate-types-rolldown.ts silently falls
back to an empty string when ts.sys.readFile fails, which can create an empty
.d.ts instead of surfacing a failure. Update the file-copy logic in the
parsed.fileNames loop to explicitly handle unreadable/missing declaration files
by throwing or logging a clear error before writing, and keep the existing .d.ts
detection and target path handling intact.
Comment on lines
+68
to
+75
| return await new Promise<boolean>((resolve) => { | ||
| const socket = connect({ port, host: '127.0.0.1' }); | ||
| socket.once('connect', () => { | ||
| socket.destroy(); | ||
| resolve(true); | ||
| }); | ||
| socket.once('error', () => resolve(false)); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js net.Socket connect no timeout hangs when never emits connect or error
💡 Result:
In Node.js, net.Socket does not have a built-in connection timeout [1][2][3]. When you call socket.connect(), the operation depends on the underlying operating system's TCP stack to either establish a connection, refuse it, or reach its own internal timeout, which is often very long [4][2]. If the connection attempt is "hanging" (e.g., due to packet loss or an unreachable destination that does not actively reject the connection), the net.Socket instance will wait indefinitely because it does not have a native "connection timeout" mechanism [2][5]. It is important to distinguish this from socket.setTimeout(), which is documented to control the timeout for socket inactivity once a connection is established, not the duration of the initial connection attempt [1][3][6]. To handle this behavior and prevent indefinite hangs, you must implement your own connection timeout logic [2][7][5]. The standard approach is to use a manual timer: 1. Create a setTimeout timer immediately before or during the socket.connect() call [2]. 2. If the 'connect' or 'error' event emits before the timer expires, clear the timer using clearTimeout() [2]. 3. If the timer expires first, the connection attempt is considered timed out. You should then manually destroy the socket using socket.destroy() and handle the failure appropriately (e.g., by emitting a custom timeout error or closing the socket) [2][7][5]. Example implementation pattern: const net = require('net'); const socket = new net.Socket; const timeout = 5000; // 5 seconds // Set a manual connection timer const timer = setTimeout( => { socket.destroy; // Terminate the connection attempt console.error('Connection timed out'); }, timeout); socket.connect(port, host, => { clearTimeout(timer); // Connection successful, clear the timer console.log('Connected'); }); socket.on('error', (err) => { clearTimeout(timer); // Connection failed, clear the timer console.error('Connection error:', err); });
Citations:
- 1: https://nodejs.org/api/net.html
- 2: net.createConnection does not have a connection timeout. nodejs/node#39256
- 3: http://nodejs.org/docs/latest/api/net.html
- 4: https://stackoverflow.com/questions/71374789/node-js-net-socket-connect-hangs-and-doesnt-connect-to-server
- 5: https://stackoverflow.com/questions/25669094/node-js-hanging-tcp-connecting-attempt
- 6: https://nodejs.org/docs/latest-v23.x/api/net.html
- 7: net.Socket has no setConnectionTimeout(timeout) method nodejs/node#5757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file around the reported lines.
wc -l scripts/tasks/dev.ts
sed -n '1,180p' scripts/tasks/dev.ts
# Find the ready()/connect fallback usage in the repo for context.
rg -n "connect\\({ port, host: '127\\.0\\.0\\.1' }\\)|ready\\(" scripts/tasks/dev.ts scripts -SRepository: storybookjs/storybook
Length of output: 4804
Add a real connect timeout to the fallback socket. socket.setTimeout() only applies after a connection is established, so the fallback can still hang on a stalled TCP handshake. Use a manual timer that destroys the socket and resolves false on expiry.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tasks/dev.ts` around lines 68 - 75, The fallback socket in the
connect check can still hang during the TCP handshake because the current
timeout approach does not cover pre-connect stalls. Update the Promise in the
connection helper to use a manual timer around the socket created by connect,
and on expiry destroy the socket and resolve false; keep the existing connect
and error handlers in the same helper so the timeout is cleared when the socket
connects or fails normally.
Source: Linters/SAST tools
Comment on lines
+54
to
+78
| // The sandbox-mutating specs run in a second, serial invocation so their | ||
| // dev-server invalidations cannot reload the parallel pass's pages. Two | ||
| // invocations instead of a Playwright project dependency: dependency | ||
| // projects run unfiltered, which would re-run the whole chromium suite on | ||
| // any CI shard whose file subset contains a mutating spec. | ||
| // --pass-with-no-tests covers shards whose subset has no match for one of | ||
| // the projects. | ||
| for (const [project, junitSuffix] of [ | ||
| ['chromium', ''], | ||
| ['chromium-mutating', '-mutating'], | ||
| ]) { | ||
| await exec( | ||
| `yarn playwright test --project=${project} --pass-with-no-tests ${testFiles.join(' ')}`, | ||
| { | ||
| env: { | ||
| ...baseEnv, | ||
| ...(junitFilename && { | ||
| PLAYWRIGHT_JUNIT_OUTPUT_NAME: junitFilename.replace(/\.xml$/, `${junitSuffix}.xml`), | ||
| }), | ||
| }, | ||
| cwd: codeDir, | ||
| }, | ||
| cwd: codeDir, | ||
| }, | ||
| { dryRun, debug } | ||
| ); | ||
| { dryRun, debug } | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
First-project failure aborts the run before chromium-mutating executes, hiding its results.
exec() throws on a non-zero exit (test failures included), and the loop awaits each invocation with no error handling. If the chromium project fails, the loop exits immediately and chromium-mutating never runs — so any regression in the mutating specs won't surface in that CI run, requiring an extra push/fix cycle to discover. Previously (single invocation covering all projects) both projects' results were visible in one run.
🛠️ Run both projects and surface either failure
+ let firstError: unknown;
for (const [project, junitSuffix] of [
['chromium', ''],
['chromium-mutating', '-mutating'],
]) {
- await exec(
- `yarn playwright test --project=${project} --pass-with-no-tests ${testFiles.join(' ')}`,
- {
- env: {
- ...baseEnv,
- ...(junitFilename && {
- PLAYWRIGHT_JUNIT_OUTPUT_NAME: junitFilename.replace(/\.xml$/, `${junitSuffix}.xml`),
- }),
- },
- cwd: codeDir,
- },
- { dryRun, debug }
- );
+ try {
+ await exec(
+ `yarn playwright test --project=${project} --pass-with-no-tests ${testFiles.join(' ')}`,
+ {
+ env: {
+ ...baseEnv,
+ ...(junitFilename && {
+ PLAYWRIGHT_JUNIT_OUTPUT_NAME: junitFilename.replace(/\.xml$/, `${junitSuffix}.xml`),
+ }),
+ },
+ cwd: codeDir,
+ },
+ { dryRun, debug }
+ );
+ } catch (err) {
+ firstError ??= err;
+ }
}
+ if (firstError) {
+ throw firstError;
+ }Want me to open an issue to track this if you'd rather defer the fix?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The sandbox-mutating specs run in a second, serial invocation so their | |
| // dev-server invalidations cannot reload the parallel pass's pages. Two | |
| // invocations instead of a Playwright project dependency: dependency | |
| // projects run unfiltered, which would re-run the whole chromium suite on | |
| // any CI shard whose file subset contains a mutating spec. | |
| // --pass-with-no-tests covers shards whose subset has no match for one of | |
| // the projects. | |
| for (const [project, junitSuffix] of [ | |
| ['chromium', ''], | |
| ['chromium-mutating', '-mutating'], | |
| ]) { | |
| await exec( | |
| `yarn playwright test --project=${project} --pass-with-no-tests ${testFiles.join(' ')}`, | |
| { | |
| env: { | |
| ...baseEnv, | |
| ...(junitFilename && { | |
| PLAYWRIGHT_JUNIT_OUTPUT_NAME: junitFilename.replace(/\.xml$/, `${junitSuffix}.xml`), | |
| }), | |
| }, | |
| cwd: codeDir, | |
| }, | |
| cwd: codeDir, | |
| }, | |
| { dryRun, debug } | |
| ); | |
| { dryRun, debug } | |
| ); | |
| } | |
| // The sandbox-mutating specs run in a second, serial invocation so their | |
| // dev-server invalidations cannot reload the parallel pass's pages. Two | |
| // invocations instead of a Playwright project dependency: dependency | |
| // projects run unfiltered, which would re-run the whole chromium suite on | |
| // any CI shard whose file subset contains a mutating spec. | |
| // --pass-with-no-tests covers shards whose subset has no match for one of | |
| // the projects. | |
| let firstError: unknown; | |
| for (const [project, junitSuffix] of [ | |
| ['chromium', ''], | |
| ['chromium-mutating', '-mutating'], | |
| ]) { | |
| try { | |
| await exec( | |
| `yarn playwright test --project=${project} --pass-with-no-tests ${testFiles.join(' ')}`, | |
| { | |
| env: { | |
| ...baseEnv, | |
| ...(junitFilename && { | |
| PLAYWRIGHT_JUNIT_OUTPUT_NAME: junitFilename.replace(/\.xml$/, `${junitSuffix}.xml`), | |
| }), | |
| }, | |
| cwd: codeDir, | |
| }, | |
| { dryRun, debug } | |
| ); | |
| } catch (err) { | |
| firstError ??= err; | |
| } | |
| } | |
| if (firstError) { | |
| throw firstError; | |
| } |
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 65-77: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tasks/e2e-tests-build.ts` around lines 54 - 78, The sequential loop
in e2e-tests-build.ts currently stops at the first failing `exec()` call, so a
failing `chromium` run prevents `chromium-mutating` from running and hides its
results. Update the `for (const [project, junitSuffix] of ...)` flow to always
execute both Playwright projects, even if one fails, and then surface a failure
at the end if either invocation failed. Keep the existing `exec()` behavior in
mind and add error aggregation or per-iteration handling around the `exec` calls
so both project results are preserved.
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
3 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.
What I did
Splits the monolithic
ci:normallabel into smaller, composable CI labels so PRs can run a much faster, targeted pipeline:ci:core— runs the internal Storybook e2e tests and every non-sandbox-specific job (build, unit/story tests, static checks, TypeScript validation, Chromatic, benchmarks, test-storybooks, init-empty). This is thenormalworkflow minus all sandbox jobs: 19 jobs instead of 86.ci:<framework>(ci:react,ci:vue3,ci:angular,ci:svelte,ci:html,ci:preact,ci:web-components,ci:solid) — runs all sandboxes for that framework, drawn from the complete (daily) template pool. Atoms are derived from each template'sexpected.renderer, so new templates join the right label automatically. Both Angular packages map to the singleangularatom.ci:<builder>(ci:vite,ci:webpack,ci:rsbuild) — runs all sandboxes for that builder, viaexpected.builder.Split labels compose: the trigger workflow joins them into one
+-separated CircleCIworkflowparameter (e.g.core+react), so a single pipeline — and a single completion status check for branch protection — covers the union of the selected atoms. The legacy cadence labels (ci:normal/ci:merged/ci:daily/ci:docs) behave exactly as before and take precedence when both kinds are present; the dangerfile now accepts split labels in place of a cadence label and rejects mixing the two.Under the hood, the CircleCI
workflowpipeline parameter changes from an enum to a plain string, validated byscripts/ci/main.ts, which fails pipeline setup loudly (listing all valid atoms) on anything unknown.Stacked on @valentinpalkovic's CI stack
This branch deliberately includes the CI-timing stack (#35341, #35343, #35346, #35347, #35348, #35349, #35350, #35352, #35353, #35355, #35356, cherry-picked from #35358's combined branch) because the split rewrites the same generator files and would otherwise conflict. The split itself is the single top commit (
ab81b9176, 10 files, +192/−34). Once the stack PRs merge, a rebase shrinks this PR to just that commit.The labels must be created on the repo:
ci:core,ci:react,ci:vue3,ci:angular,ci:svelte,ci:html,ci:preact,ci:web-components,ci:solid,ci:vite,ci:webpack,ci:rsbuild. The code validates atoms but cannot create labels.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
The config generator was validated directly (see manual testing): all 18 workflow shapes generate valid configs, legacy workflows (
normal/merged/daily/docs) produce byte-identical output apart from the intentional parameter-type change, and unknown atoms fail setup with a clear error.Manual testing
yarn workspaces focus @storybook/scriptsyarn dlx jiti ./scripts/ci/main.ts --workflow=core+react, then check.circleci/config.generated.yml— it should contain the core jobs plus every react sandbox flow, deduplicated, with a singlecircleci-completionjob.--workflow=normal) and confirm the job set matches whatnextgenerates today.--workflow=bogus) and confirm the generator throws, listing all valid atoms.ci:core+ci:reactto a test PR and confirm a single CircleCI pipeline namedcore-react-generatedis triggered.Documentation
.agents/skills/prand.agents/skills/open-prlabel guidance)MIGRATION.MD
Checklist for Maintainers
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.ts—ci:normalapplied: it exercises the legacy path end-to-end through the new string-parameter plumbingqa:neededorqa:skip—qa:skip: CI-infrastructure only, no user-facing behaviorbuild🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>🤖 Generated with Claude Code
https://claude.ai/code/session_01JDwFN1kYSqVTZah1BvKQKu
Summary by CodeRabbit
New Features
Bug Fixes
Chores