CircleCI: Dynamic config setup#32925
Conversation
|
View your CI Pipeline Execution ↗ for commit b9aa2ad
☁️ Nx Cloud last updated this comment at |
…mline node installation steps. The command for generating config has been modified to use yarn dlx for improved clarity and efficiency in the CI process.
…ecutor settings, enhancing clarity in the CI setup.
…for package installation and remove redundant shallow clone step, improving clarity and efficiency in the CI process.
… installation, enhancing consistency in the CI process and streamlining the command for generating configuration.
…n step after Corepack setup, ensuring proper package management with PNPM.
…Yarn, adjusting the resource class and command for generating configuration to enhance clarity and efficiency in the CI process.
…ult and add continuation step for configuration generation, improving CI workflow management.
…esource_class for improved resource allocation during CI jobs.
…fault to node/default, ensuring compatibility with the current CI environment.
…hs, changing from /tmp to relative paths for improved clarity and consistency in CI jobs.
…OT_DIR for consistency across executors and CI jobs.
…ttings to use /tmp for improved isolation and consistency across CI jobs.
…path settings, replacing specific directories with relative paths for improved clarity and consistency across CI jobs.
…y removing unnecessary prefixes, enhancing clarity and consistency across CI jobs.
…nd various other packages, enhancing compatibility and performance. Adjust execa usage to utilize ResultPromise for improved type safety in command execution.
…onsOrPrompt function in options.ts for improved command handling.
…nsOrPrompt function in options.ts for improved flexibility in command handling.
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
Review: CircleCI Dynamic Config Setup
Thanks for the detailed walkthrough in the video! The architecture is clean and the TypeScript approach is much more maintainable than 50+ YAML files.
Question: Is all old functionality ported?
I noticed two things that appear to be missing from the new implementation:
1. Discord Notifications
Old (.circleci/src/commands/report-workflow-on-failure.yml):
- discord/status:
only_for_branches: main,next,next-release,latest-release
fail_only: true
failure_message: $(yarn get-report-message << pipeline.parameters.workflow >> << parameters.template >>)New (scripts/ci/utils/helpers.ts:207-224):
reportOnFailure: () => {
return [
// ...
{
run: {
name: 'Report failure',
when: 'on_fail',
command: 'echo "Workflow failed"', // ← Just echoes to stdout
},
},
];
},The Discord orb is still defined in orbs.ts, but not used. Is this intentional (deferred to M6: Reporting) or should it be restored?
2. Coverage Job
The old coverage.yml job with codecov integration appears to be removed entirely. The tracking issue mentions "Code coverage reporting on PR" under M6 - is this intentionally deferred?
If these are intentionally deferred to M6, it might be worth adding a comment in the code or tracking issue to make that clear.
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
I absolutely agree that this PR improves our current setup. The features it adds are valuable - sandboxes running independently so one failure doesn't block others, the ability to filter jobs for debugging, and better visibility in the CircleCI graph.
That said, as you know, I still believe using a dedicated task runner is the right direction long-term. Here's why:
1. CI Config Generated from Task Graph
With NX Agents, this entire PR essentially becomes unnecessary - the CI configuration is automatically derived from the task graph. No custom code to generate YAML, no manual job dependencies to maintain. NX Agents dynamically distributes tasks across CI agents at runtime - it automatically determines which tasks can run concurrently and assigns them to available agents to minimize idle time.
(Turborepo doesn't have this level of automation yet - you'd still need to set things up manually.)
2. This is a Lot of Custom Code
This PR adds ~1500 lines of custom infrastructure that we need to maintain. With NX Agents, we get the same features (and more) out of the box.
3. Change Detection and Remote Caching
Both NX and Turborepo offer built-in change detection and remote caching. We're planning to implement change detection ourselves, but these tools already have it - they know which projects are impacted by a change and only run those tasks. Remote caching means tasks that haven't changed don't re-run, even across different PRs.
4. Breaking the Homebrew Pattern
Historically, the team has invested a lot of time in homebrew solutions - our custom yarn task command being a prime example. This PR continues that pattern. I would really like to discontinue this trend and instead normalize our repository as much as possible: using standard practices and the best tools available rather than building our own.
5. LLM Era Consideration
This is especially important in the age of LLMs. Using well-documented, widely-adopted tools means LLMs have extensive training data to help debug, optimize, and extend configurations. Custom infrastructure is harder for LLMs to assist with since it's unique to our codebase. Normalizing our setup makes it easier for both humans and AI to contribute.
This isn't blocking - the PR is a clear improvement and I'm happy to approve.
However, I think we need a very strong case to continue the remaining milestones if we can get so much for free by using a dedicated tool instead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @scripts/ci/sandboxes.ts:
- Around line 326-336: The conditional around including testRunnerJob is
ambiguous about intent; decide whether test-runner validates the runner once or
per-sandbox and then make the code and comment explicit. If test-runner is
intended to validate the test runner globally, simplify to include testRunnerJob
once (remove per-sandbox conditional) and add a clarifying comment; if it must
validate each sandbox, change the condition to run testRunnerJob for every
sandbox unless chromatic is running for that sandbox (or document that chromatic
skips test-runner globally). Update the condition that currently uses skipTasks,
testRunnerJob, and chromatic to match the chosen intent and add a short comment
explaining the rationale so future readers know why testRunnerJob is included or
omitted.
- Around line 345-366: The code uses fragile numeric indexing (sandbox.jobs[1])
to reference the build job; refactor defineSandboxFlow to return a jobs object
with named properties (e.g., jobs: { create, build, dev, chromatic, vitest, e2e,
testRunner } and keep the original flat array as allJobs for compatibility),
then update defineSandboxTestRunner to use the named reference
(sandbox.jobs.build) for both the job id in the name and the dependency list
instead of sandbox.jobs[1]; ensure other callers still work by preserving the
flat jobs array as allJobs.
- Around line 368-468: The Windows sandbox jobs use fragile numeric indices on
sandbox.jobs; replace those with named variables by destructuring or finding the
roles at the top of each function (e.g., const [createJob, buildJob, devJob] =
sandbox.jobs) and then use createJob, buildJob, devJob (and their .id) in the
defineJob name and the dependency arrays instead of sandbox.jobs[0],
sandbox.jobs[1], and sandbox.jobs[2]; update references in
defineWindowsSandboxDev (where sandbox.jobs[2] and [sandbox.jobs[0]] are used)
and defineWindowsSandboxBuild (where sandbox.jobs[1] and [sandbox.jobs[0]] are
used).
In @scripts/ci/utils/types.ts:
- Around line 20-29: The JobImplementationObj executor union is missing the
shell property for the Windows executor variant; update the second union branch
(the one with name: 'win/default') to include a shell field (e.g., shell: string
or a constrained union of known shells like 'bash.exe') so code that uses shell
(e.g., scripts/ci/init-empty.ts and scripts/ci/common-jobs.ts) type-checks; keep
the existing size field and only add the shell property to the Windows-specific
executor shape.
🧹 Nitpick comments (1)
scripts/ci/utils/types.ts (1)
52-52: Consider stricter typing for steps array.The
steps: unknown[]type provides no compile-time safety. The TODO comment acknowledges this, but given that steps have a well-defined structure (run commands, restore_cache, attach_workspace, etc.), a union type would catch errors earlier.This can be addressed when the team has bandwidth, as the TODO indicates this is already on the radar.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
scripts/ci/common-jobs.tsscripts/ci/init-empty.tsscripts/ci/main.tsscripts/ci/sandboxes.tsscripts/ci/test-storybooks.tsscripts/ci/utils/helpers.tsscripts/ci/utils/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/ci/main.ts
- scripts/ci/test-storybooks.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
scripts/ci/init-empty.tsscripts/ci/utils/types.tsscripts/ci/sandboxes.tsscripts/ci/common-jobs.tsscripts/ci/utils/helpers.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
scripts/ci/init-empty.tsscripts/ci/utils/types.tsscripts/ci/sandboxes.tsscripts/ci/common-jobs.tsscripts/ci/utils/helpers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
scripts/ci/init-empty.tsscripts/ci/utils/types.tsscripts/ci/sandboxes.tsscripts/ci/common-jobs.tsscripts/ci/utils/helpers.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
scripts/ci/init-empty.tsscripts/ci/utils/types.tsscripts/ci/sandboxes.tsscripts/ci/common-jobs.tsscripts/ci/utils/helpers.ts
🧠 Learnings (2)
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Sandboxes are generated outside the repository at `../storybook-sandboxes/` by default, not in `./sandbox` or `code/sandbox/`
Applied to files:
scripts/ci/sandboxes.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/ci/utils/helpers.ts
🧬 Code graph analysis (3)
scripts/ci/init-empty.ts (4)
scripts/ci/utils/types.ts (5)
defineJob(76-90)defineNoOpJob(99-109)Workflow(111-111)JobOrNoOpJob(12-12)isWorkflowOrAbove(133-146)scripts/ci/utils/helpers.ts (3)
workflow(170-232)verdaccio(147-168)server(135-145)scripts/ci/utils/constants.ts (1)
WINDOWS_ROOT_DIR(17-17)scripts/ci/common-jobs.ts (1)
build_linux(24-65)
scripts/ci/utils/types.ts (3)
scripts/ci/utils/helpers.ts (2)
workflow(170-232)toId(126-133)scripts/ci/utils/executors.ts (1)
executors(3-64)scripts/ci/utils/parameters.ts (1)
parameters(2-19)
scripts/ci/utils/helpers.ts (3)
scripts/ci/utils/constants.ts (2)
LINUX_ROOT_DIR(16-16)WINDOWS_ROOT_DIR(17-17)code/core/src/manager-api/modules/shortcuts.ts (1)
keys(23-25)scripts/ci/utils/types.ts (2)
Workflow(111-111)JobOrNoOpJob(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: merged-generated
- GitHub Check: nx
🔇 Additional comments (21)
scripts/ci/utils/types.ts (3)
1-10: LGTM!The core Job type structure is well-designed with proper generics for type safety.
76-109: LGTM!The factory functions provide a clean, type-safe API for creating jobs and no-op hubs. The automatic ID generation via
toIdensures consistency.
133-146: LGTM!The workflow comparison logic correctly implements the hierarchy described in the JSDoc. The handling of 'docs' as independent and the default case for 'skipped' are both appropriate.
scripts/ci/utils/helpers.ts (7)
1-49: LGTM!The workspace, cache, and artifact helper namespaces provide clean abstractions over CircleCI primitives with sensible defaults.
51-78: LGTM!The git helpers provide flexible checkout options with sensible defaults for CI environments.
80-124: LGTM!The Node and npm installation helpers handle platform-specific concerns appropriately, including Windows PowerShell setup and corepack configuration.
126-133: LGTM!The
toIdfunction correctly normalizes job names into valid CircleCI identifiers.
135-168: LGTM!The server and verdaccio helpers provide clean abstractions for local registry management in CI.
170-262: LGTM!The workflow restoration and reporting helpers are well-documented, including an inline explanation of the Windows workaround at lines 181-186. The progressive cache key generation is clever.
264-281: LGTM with a note on implementation.The function correctly handles circular dependencies through the
results.findcheck at line 273. Theshift()mutation of the input array is a bit unconventional but works correctly here.For future maintainability, consider adding a comment explaining how circular dependencies are handled by the results array check.
scripts/ci/init-empty.ts (4)
12-52: LGTM!The empty init flow correctly sets up a temporary directory, configures the npm registry for verdaccio, and runs Storybook initialization with appropriate environment flags.
54-94: LGTM!This flow tests the create-storybook command with feature flags, which is a distinct code path from the standard init flow. The use of vitest for validation is appropriate for this scenario.
96-137: LGTM!The Windows init flow appropriately uses bash.exe and Windows paths. The Unix-style commands (mkdir, cd) work correctly in Git Bash for Windows.
139-155: LGTM!The workflow-based composition correctly scales up the init tests: react-vite-ts baseline, additional templates for merged, Windows for daily, and features for normal+.
scripts/ci/common-jobs.ts (5)
24-120: LGTM!The build jobs correctly use glob to dynamically discover workspace paths for persistence. Using
glob.syncat config generation time is appropriate since this code runs once to produce the CircleCI config, not during job execution.
122-229: LGTM!The validation jobs (check, lint, knip) follow consistent patterns. Note that Knip uses
--no-exit-codeat line 223, which means it runs for informational purposes without failing the build.
231-287: LGTM!The test jobs correctly leverage CircleCI's test splitting feature and persist JUnit results for reporting. The separation of unit tests and story tests is appropriate given the different executor requirements (Playwright for stories).
289-341: LGTM!The Windows test job and benchmark job are well-structured. The benchmark job correctly uses CircleCI pipeline parameters to pass PR metadata for comparison purposes.
22-22: Node.js version supportsimport.meta.dirname. The project uses Node.js 22.21.1 (per.nvmrc), which fully supports this ES module feature (available since Node.js 20.11.0). No issues.scripts/ci/sandboxes.ts (2)
20-116: LGTM!The internal sandbox job builders correctly handle the build and dev scenarios, with appropriate conditional logic for e2e testing.
485-499: LGTM with note on Windows job coverage.The composition logic is clear. Note that lines 491-493 only create Windows and test-runner jobs for the first sandbox (
sandboxes[0]), which aligns with the PR comment about Windows jobs being expensive (~30% of workflow cost).This selective approach is appropriate given the cost concerns mentioned in the PR objectives.
| /** | ||
| * Question: What is this for? Do we want to know if the test-runner works? Or do we want to | ||
| * know if the sandbox works? | ||
| * | ||
| * If it's the first, we actually only need to run the test-runner job once, on any sandbox. If | ||
| * it's the second, we need to run the test-runner job for each sandbox, but then we don't need | ||
| * to run it when we're already running the chromatic job. | ||
| */ | ||
| !skipTasks?.includes('test-runner') && skipTasks.includes('chromatic') | ||
| ? testRunnerJob | ||
| : undefined, |
There was a problem hiding this comment.
Clarify the purpose of the test-runner job.
The comment reveals uncertainty about whether this job validates the test-runner itself or the sandbox. This ambiguity suggests the team should document or reconsider this logic before it becomes harder to maintain.
Consider either:
- Documenting the intent clearly if the current behavior is correct
- Simplifying the logic at lines 334-336 if the conditional inclusion is overly complex
🤖 Prompt for AI Agents
In @scripts/ci/sandboxes.ts around lines 326 - 336, The conditional around
including testRunnerJob is ambiguous about intent; decide whether test-runner
validates the runner once or per-sandbox and then make the code and comment
explicit. If test-runner is intended to validate the test runner globally,
simplify to include testRunnerJob once (remove per-sandbox conditional) and add
a clarifying comment; if it must validate each sandbox, change the condition to
run testRunnerJob for every sandbox unless chromatic is running for that sandbox
(or document that chromatic skips test-runner globally). Update the condition
that currently uses skipTasks, testRunnerJob, and chromatic to match the chosen
intent and add a short comment explaining the rationale so future readers know
why testRunnerJob is included or omitted.
| export function defineSandboxTestRunner(sandbox: ReturnType<typeof defineSandboxFlow>) { | ||
| return defineJob( | ||
| `${sandbox.jobs[1].id}-test-runner`, | ||
| () => ({ | ||
| executor: { | ||
| name: 'sb_playwright', | ||
| class: 'medium', | ||
| }, | ||
| steps: [ | ||
| ...workflow.restoreLinux(), | ||
| { | ||
| run: { | ||
| name: 'Running test-runner', | ||
| command: `yarn task test-runner --template ${sandbox.name} --no-link -s test-runner --junit`, | ||
| }, | ||
| }, | ||
| testResults.persist(join(LINUX_ROOT_DIR, WORKING_DIR, 'test-results')), | ||
| ], | ||
| }), | ||
| [sandbox.jobs[1]] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fragile array indexing: Replace hardcoded indices with named references.
Lines 347 and 364 access sandbox.jobs[1] assuming it's the build job. If the job order in defineSandboxFlow changes (lines 318-337), this will break silently.
🔧 Suggested refactor for maintainability
Consider restructuring defineSandboxFlow to return a named object:
// In defineSandboxFlow, instead of returning:
return {
name: key,
path,
jobs,
};
// Return:
return {
name: key,
path,
jobs: {
create: createJob,
build: buildJob,
dev: devJob,
chromatic: chromaticJob,
vitest: vitestJob,
e2e: e2eJob,
testRunner: testRunnerJob,
},
allJobs: jobs, // flat array for backward compatibility
};
// Then in defineSandboxTestRunner:
[sandbox.jobs.build] // instead of sandbox.jobs[1]This makes dependencies explicit and prevents silent breakage.
🤖 Prompt for AI Agents
In @scripts/ci/sandboxes.ts around lines 345 - 366, The code uses fragile
numeric indexing (sandbox.jobs[1]) to reference the build job; refactor
defineSandboxFlow to return a jobs object with named properties (e.g., jobs: {
create, build, dev, chromatic, vitest, e2e, testRunner } and keep the original
flat array as allJobs for compatibility), then update defineSandboxTestRunner to
use the named reference (sandbox.jobs.build) for both the job id in the name and
the dependency list instead of sandbox.jobs[1]; ensure other callers still work
by preserving the flat jobs array as allJobs.
| export function defineWindowsSandboxDev(sandbox: ReturnType<typeof defineSandboxFlow>) { | ||
| return defineJob( | ||
| `${sandbox.jobs[2].id}-windows`, | ||
| () => ({ | ||
| executor: { | ||
| name: 'win/default', | ||
| size: 'xlarge', | ||
| shell: 'bash.exe', | ||
| }, | ||
| steps: [ | ||
| ...workflow.restoreWindows(), | ||
| verdaccio.start(), | ||
| server.wait([...verdaccio.ports]), | ||
| { | ||
| run: { | ||
| name: 'Run Install', | ||
| working_directory: `${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.path}`, | ||
| command: 'yarn install', | ||
| }, | ||
| }, | ||
| { | ||
| run: { | ||
| name: 'Install playwright', | ||
| command: 'yarn playwright install chromium --with-deps', | ||
| }, | ||
| }, | ||
| { | ||
| run: { | ||
| name: 'Run storybook', | ||
| background: true, | ||
| working_directory: `${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.path}`, | ||
| command: 'yarn storybook --port 8001', | ||
| }, | ||
| }, | ||
| server.wait(['8001']), | ||
| { | ||
| run: { | ||
| name: 'Running E2E Tests', | ||
| working_directory: 'code', | ||
| command: `yarn task e2e-tests-dev --template ${sandbox.name} --no-link -s e2e-tests-dev --junit`, | ||
| }, | ||
| }, | ||
| testResults.persist(`${WINDOWS_ROOT_DIR}\\${WORKING_DIR}\\test-results`), | ||
| ], | ||
| }), | ||
| [sandbox.jobs[0]] | ||
| ); | ||
| } | ||
|
|
||
| export function defineWindowsSandboxBuild(sandbox: ReturnType<typeof defineSandboxFlow>) { | ||
| return defineJob( | ||
| `${sandbox.jobs[1].id}-windows`, | ||
| () => ({ | ||
| executor: { | ||
| name: 'win/default', | ||
| size: 'xlarge', | ||
| shell: 'bash.exe', | ||
| }, | ||
| steps: [ | ||
| ...workflow.restoreWindows(), | ||
| verdaccio.start(), | ||
| server.wait([...verdaccio.ports]), | ||
| { | ||
| run: { | ||
| name: 'Run Install', | ||
| working_directory: `${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.path}`, | ||
| command: 'yarn install', | ||
| }, | ||
| }, | ||
| { | ||
| run: { | ||
| name: 'Install playwright', | ||
| command: 'yarn playwright install chromium --with-deps', | ||
| }, | ||
| }, | ||
| { | ||
| run: { | ||
| name: 'Build storybook', | ||
| command: `yarn task build --template ${sandbox.name} --no-link -s build`, | ||
| }, | ||
| }, | ||
| { | ||
| run: { | ||
| name: 'Serve storybook', | ||
| background: true, | ||
| command: `yarn task serve --template ${sandbox.name} --no-link -s serve`, | ||
| }, | ||
| }, | ||
| server.wait(['8001']), | ||
| { | ||
| run: { | ||
| name: 'Running E2E Tests', | ||
| working_directory: 'code', | ||
| command: `yarn task e2e-tests --template ${sandbox.name} --no-link -s e2e-tests`, | ||
| }, | ||
| }, | ||
| ], | ||
| }), | ||
| [sandbox.jobs[0]] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fragile array indexing: Same issue with Windows sandbox jobs.
Lines 370, 413, 419, and 466 use hardcoded array indices [0], [1], [2] to access create, build, and dev jobs. This is part of the same maintainability risk flagged in defineSandboxTestRunner.
Apply the same refactoring suggested in the previous comment to use named job references instead of array indices. This will make all sandbox-related functions more resilient to changes.
🤖 Prompt for AI Agents
In @scripts/ci/sandboxes.ts around lines 368 - 468, The Windows sandbox jobs use
fragile numeric indices on sandbox.jobs; replace those with named variables by
destructuring or finding the roles at the top of each function (e.g., const
[createJob, buildJob, devJob] = sandbox.jobs) and then use createJob, buildJob,
devJob (and their .id) in the defineJob name and the dependency arrays instead
of sandbox.jobs[0], sandbox.jobs[1], and sandbox.jobs[2]; update references in
defineWindowsSandboxDev (where sandbox.jobs[2] and [sandbox.jobs[0]] are used)
and defineWindowsSandboxBuild (where sandbox.jobs[1] and [sandbox.jobs[0]] are
used).
| export type JobImplementationObj = { | ||
| executor: | ||
| | { | ||
| name: keyof typeof executors; | ||
| class: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge'; | ||
| } | ||
| | { | ||
| name: 'win/default'; | ||
| size: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge'; | ||
| }; |
There was a problem hiding this comment.
Add shell property to the Windows executor type.
The executor type definition is missing the shell property that's used in practice. For example, scripts/ci/init-empty.ts line 103 and scripts/ci/common-jobs.ts line 89 use shell: 'bash.exe', but the type doesn't include this field.
🔧 Proposed fix to add shell property
| {
name: 'win/default';
size: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge';
+ shell?: string;
};📝 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.
| export type JobImplementationObj = { | |
| executor: | |
| | { | |
| name: keyof typeof executors; | |
| class: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge'; | |
| } | |
| | { | |
| name: 'win/default'; | |
| size: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge'; | |
| }; | |
| export type JobImplementationObj = { | |
| executor: | |
| | { | |
| name: keyof typeof executors; | |
| class: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge'; | |
| } | |
| | { | |
| name: 'win/default'; | |
| size: 'small' | 'medium' | 'medium+' | 'large' | 'xlarge'; | |
| shell?: string; | |
| }; |
🤖 Prompt for AI Agents
In @scripts/ci/utils/types.ts around lines 20 - 29, The JobImplementationObj
executor union is missing the shell property for the Windows executor variant;
update the second union branch (the one with name: 'win/default') to include a
shell field (e.g., shell: string or a constrained union of known shells like
'bash.exe') so code that uses shell (e.g., scripts/ci/init-empty.ts and
scripts/ci/common-jobs.ts) type-checks; keep the existing size field and only
add the shell property to the Windows-specific executor shape.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/sandbox-templates.ts (1)
739-752: Add TODO comment with issue reference for tracking.The commented-out template includes a helpful explanation of why it's disabled, but lacks a TODO or issue reference for re-enablement. Per the guidance at lines 82-83, templates in development should include a TODO comment to track their status.
📝 Suggested fix
- /** This is currently broken, we generate components and stories that do not work */ + /** + * TODO: Re-enable once Qwik template generation is fixed + * This is currently broken, we generate components and stories that do not work + * @see https://github.com/storybookjs/storybook/issues/XXXXX + */ // 'qwik-vite/default-ts': {Consider opening a tracking issue if one doesn't exist, or referencing an existing issue number.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/lib/cli-storybook/src/sandbox-templates.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
🧠 Learnings (4)
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Sandboxes are generated outside the repository at `../storybook-sandboxes/` by default, not in `./sandbox` or `code/sandbox/`
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: merged-generated
- GitHub Check: nx
🔇 Additional comments (3)
code/lib/cli-storybook/src/sandbox-templates.ts (3)
7-10: LGTM! Well-structured type exports for CI tooling.The new type aliases provide a clean, type-safe interface for template orchestration in CI scripts. The use of
Pickto expose only specific Template properties andkeyof typeoffor key extraction follows TypeScript best practices.
26-26: LGTM! Cadence type provides good type safety.The
Cadencetype correctly derives fromtemplatesByCadencekeys, enabling type-safe cadence selection across the CI configuration system.
1039-1039: LGTM! Consistent with template removal.The commented-out entry correctly reflects the disabled qwik-vite template.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/sandbox-templates.ts (1)
7-10: Consider documenting the relationship betweenAllTemplatesKeyandTemplateKey.
AllTemplatesKey(line 8) andTemplateKey(lines 21-24) are functionally equivalent types—both resolve to the union of keys frombaseTemplates,internalTemplates, andbenchTemplates. While having both may serve different semantic purposes (e.g.,AllTemplatesKeyfor script consumption), a brief comment clarifying why both exist would help future maintainers.The forward reference to
allTemplates(defined at line 983) is valid TypeScript but could be slightly surprising to readers scanning top-to-bottom.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/lib/cli-storybook/src/sandbox-templates.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/lib/cli-storybook/src/sandbox-templates.ts
🧠 Learnings (4)
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Sandboxes are generated outside the repository at `../storybook-sandboxes/` by default, not in `./sandbox` or `code/sandbox/`
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: merged-generated
- GitHub Check: nx
🔇 Additional comments (2)
code/lib/cli-storybook/src/sandbox-templates.ts (2)
736-749: LGTM — disabled template is well-documented.The comment clearly explains that the Qwik template is broken (component/story generation issues), and preserving the full configuration in comments aids future re-enablement. The corresponding removal from the
dailyarray (line 1036) is consistent.
1036-1036: Consistent with the template definition change.The commented-out entry here aligns with the disabled
qwik-vite/default-tstemplate definition at lines 736-749.
4 participants
What I did
Related: #19476
Tracking: #32936
I'm overhauling the CI config.
In this PR I introduce the
scripts/cifolder, in which a CircleCI config is dynamically generated.This dynamic generation allows us to check the repo/PR/branch state and read dynamic config files (such as
sandboxes-templates.ts) to determine what the optimum CI workflow would look like.Here's is an overview:
The hope is that in the future, we'll be able to do change detection, and instead of adding
ci:*-labels to PRs, we automatically only run affected sandboxes.Testing
The changes in this PR are covered in the following automated tests:
Manual testing
ci:*-label at least once, checking if every runs as expected.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.