Build: Speed up Windows jobs on CircleCI by optimizing cache management #33134
Conversation
valentinpalkovic
added
build
|
View your CI Pipeline Execution ↗ for commit d06a7e3
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds per-job restore/save caches for Changes
*Ellipses denote additional job files following the same cache/sandbox pattern. Sequence Diagram(s)sequenceDiagram
participant CI as CircleCI
participant Cache as CacheStore
participant Runner as JobRunner
rect `#e8f3ff`
Note over CI,Cache: Pre-install shared node_modules restore
CI->>Cache: restore code-node-modules-{{ checksum "code/yarn.lock" }}
CI->>Cache: restore scripts-node-modules-{{ checksum "scripts/yarn.lock" }}
end
rect `#f2fff0`
Note over CI,Runner: Sandbox key computation & sandbox cache restore
CI->>Runner: run "Compute sandbox cache key" (yarn get-template -> write .sandbox-package-path.txt)
Runner-->>CI: .sandbox-package-path.txt
CI->>Cache: restore sandbox-{{ .Revision }}-{{ checksum ".sandbox-package-path.txt" }}
end
CI->>Runner: install dependencies / install sandbox deps
Runner-->>CI: run tests/builds
rect `#fff7e6`
Note over CI,Cache: Post-run save caches
CI->>Cache: save code-node-modules-{{ checksum "code/yarn.lock" }}
CI->>Cache: save scripts-node-modules-{{ checksum "scripts/yarn.lock" }}
CI->>Cache: save sandbox-{{ .Revision }}-{{ checksum ".sandbox-package-path.txt" }}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
valentinpalkovic
added
ci:daily
valentinpalkovic
force-pushed
the
valentin/speed-up-windows-jobs-on-ci
branch
from
e3bfaf0 to
23c5fb1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
.circleci/src/jobs/create-sandboxes.yml (1)
47-55: Duplication:yarn get-templateandyarn get-sandbox-dirare recomputed unnecessarily.These commands were already executed in the "Create Sandboxes" step (lines 35 and 41). Consider computing both values once earlier (e.g., in an initial run step) and storing them in $BASH_ENV or the job environment for reuse across subsequent steps.
.circleci/src/jobs/chromatic-sandboxes.yml (1)
35-36: Remove redundant sandbox template computation.The "Install sandbox dependencies" step recomputes
TEMPLATEandsandbox-dirusing the same yarn commands that were already executed and exported in the "Compute sandbox cache key" step (lines 24–26). Reusing the exported environment variables would be more efficient.Apply this diff to reuse the previously computed values:
- run: name: Install sandbox dependencies command: | - TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task chromatic) - cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn + source $BASH_ENV + cd sandbox/$sandbox-dir && yarnNote: Ensure
$BASH_ENVis sourced to access thesandbox-dirvariable computed in the earlier step..circleci/src/jobs/vitest-integration.yml (1)
16-21: Add fallback keys torestore_cachesteps to handle cache misses gracefully.Both
restore_cachesteps have only a single key pattern with no fallback keys. If the cache is missed (e.g., on first run, after cache cleanup, or if the yarn.lock file changes), the steps will fail rather than allowing the job to proceed.Apply this diff to add fallback keys:
- restore_cache: keys: - code-node-modules-{{ checksum "code/yarn.lock" }} + - code-node-modules- - restore_cache: keys: - scripts-node-modules-{{ checksum "scripts/yarn.lock" }} + - scripts-node-modules-.circleci/src/jobs/e2e-production.yml (1)
22-37: Recommend consolidating sandbox-dir computation.The
sandbox-dirvalue is computed on line 27 via the "Compute sandbox cache key" step, but line 36-37 independently computesyarn get-sandbox-diragain (viayarn get-templateand subsequent call). If the value should be deterministic, consider exporting the computedsandbox-dirfrom line 27-28 and reusing it in line 36-37 to avoid redundant computation and ensure consistency.For example:
- run: name: Install sandbox dependencies command: | source $BASH_ENV # Already has sandbox-dir from previous step cd sandbox/$sandbox-dir && yarn
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.circleci/config.yml(22 hunks).circleci/src/jobs/bench-packages.yml(1 hunks).circleci/src/jobs/bench-sandboxes.yml(1 hunks).circleci/src/jobs/build.yml(1 hunks).circleci/src/jobs/chromatic-internal-storybook.yml(1 hunks).circleci/src/jobs/chromatic-sandboxes.yml(1 hunks).circleci/src/jobs/create-sandboxes.yml(2 hunks).circleci/src/jobs/e2e-dev.yml(1 hunks).circleci/src/jobs/e2e-production.yml(1 hunks).circleci/src/jobs/e2e-ui-vitest-3.yml(1 hunks).circleci/src/jobs/e2e-ui.yml(1 hunks).circleci/src/jobs/knip.yml(1 hunks).circleci/src/jobs/lint.yml(1 hunks).circleci/src/jobs/script-checks.yml(1 hunks).circleci/src/jobs/stories-tests.yml(1 hunks).circleci/src/jobs/test-init-empty.yml(1 hunks).circleci/src/jobs/test-init-features.yml(1 hunks).circleci/src/jobs/test-portable-stories.yml(1 hunks).circleci/src/jobs/test-runner-production.yml(1 hunks).circleci/src/jobs/test-yarn-pnp.yml(1 hunks).circleci/src/jobs/unit-tests.yml(1 hunks).circleci/src/jobs/vitest-integration.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- .circleci/src/jobs/build.yml
- .circleci/src/jobs/unit-tests.yml
- .circleci/src/jobs/test-init-empty.yml
- .circleci/src/jobs/stories-tests.yml
- .circleci/src/jobs/e2e-ui-vitest-3.yml
- .circleci/src/jobs/chromatic-internal-storybook.yml
- .circleci/config.yml
- .circleci/src/jobs/script-checks.yml
- .circleci/src/jobs/test-yarn-pnp.yml
- .circleci/src/jobs/lint.yml
- .circleci/src/jobs/test-portable-stories.yml
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (13)
.circleci/src/jobs/e2e-ui.yml (2)
10-15: Verify this job definition is applied only to Windows jobs.The PR objectives state that cache optimization is specifically for Windows jobs (to avoid slow workspace attachment on Windows). However, the file name (
e2e-ui.yml) has no Windows-specific suffix, which suggests it may be shared across platform variants. If this job runs on both Linux and Windows, caching node_modules built on one platform and restoring them on another could cause compatibility issues.Please confirm whether:
- This job is used exclusively for Windows CI jobs, or
- There should be separate Linux/Windows job variants to avoid platform-specific build artifacts in the cache.
10-15: Confirm that corresponding save_cache steps exist elsewhere.The restore_cache steps assume that matching caches with keys
code-node-modules-{{ checksum "code/yarn.lock" }}andscripts-node-modules-{{ checksum "scripts/yarn.lock" }}have been previously saved by other jobs. Verify that correspondingsave_cachesteps are defined in upstream jobs (e.g., in install or build steps)..circleci/src/jobs/bench-sandboxes.yml (1)
16-21: Review comment is incorrect - remove it.The
save_cachesteps are not missing; they exist inbuild.yml(lines 24-29) with matching cache keys (code-node-modules-*andscripts-node-modules-*). This is the standard CircleCI workflow pattern:build.ymlsaves node_modules to cache, and dependent jobs likebench-sandboxes.ymlrestore from it. Cache keys are perfectly aligned across all jobs.Additionally,
bench-sandboxes.ymluses thesb_playwrightexecutor (not Windows-specific), consistent with most other jobs in the codebase. The restore_cache steps are correctly placed and properly keyed. No issues exist.Likely an incorrect or invalid review comment.
.circleci/src/jobs/knip.yml (1)
8-15: No action needed—all verification confirms the implementation is correct.The workspace has been properly narrowed to exclude node_modules (only compiled artifacts and
.verdaccio-cacheare persisted), corresponding save_cache steps exist in build.yml with consistentcode-node-modules-andscripts-node-modules-keys, and the restore_cache pattern is correctly applied across all jobs including knip.yml. The implementation fully addresses the PR objective..circleci/src/jobs/e2e-dev.yml (3)
22-30: Verify yarn is available at this step and will compute the correct cache key.The sandbox cache key computation (lines 22-30) executes
yarn get-templateandyarn get-sandbox-dircommands before sandbox or project dependencies are installed. This assumes:
- A
yarnbinary is available in the environment (system yarn or from attach_workspace)- The yarn commands (
get-template,get-sandbox-dir) are available and return correct values at this point- The computed values reflect the actual sandbox that will be used later
Confirm these assumptions hold, especially on Windows where path handling may differ.
Can you verify that:
- The yarn commands are available before step 33 (install sandbox dependencies)?
- These commands return the correct template and sandbox directory name?
- This works consistently across Linux and Windows environments?
16-21: Verify corresponding save_cache steps exist for these restore_cache steps.The file adds
restore_cachesteps forcode/node_modulesandscripts/node_modules(lines 16-21), but the provided file excerpt does not show correspondingsave_cachesteps that would populate these caches. The PR description mentions "saving node_modules in the CircleCI cache via a save_cache step."Confirm that:
- Corresponding
save_cachesteps exist elsewhere in the job or in upstream jobs- The cache keys in the
save_cachesteps match those in theserestore_cachesteps- The caches are populated before this job runs, or early enough in the job's dependency graph
If
save_cachesteps are missing from the job definition, therestore_cachesteps will silently succeed (cache miss is not an error) but provide no benefit.
14-15: Confirm that workspace attachment aligns with caching strategy.The job still uses
attach_workspace(lines 14-15), but the PR goal is to avoid attachingnode_moduleson Windows due to slow file operations.Verify that:
- The workspace save configuration (in upstream jobs) has been narrowed to exclude
node_modulesdirectories- Only other essential artifacts (source code, build outputs, etc.) are attached via workspace
- The
node_modulesare now sourced exclusively from therestore_cachesteps (lines 16-21).circleci/src/jobs/test-init-features.yml (1)
10-15: The cited file path is incorrect; the job is not defined in a separate file.The review references
.circleci/src/jobs/test-init-features.yml, but this file does not exist in the repository. Thetest-init-featuresjob is actually defined inline within.circleci/config.yml, not as a standalone file in thesrc/jobs/directory.The substantive concerns about cache management (e.g., whether
save_cachesteps exist and whetherattach_workspaceis needed) should be verified against the actual job definition in the main config file, not against a non-existent source file. Line numbers and file paths in the review do not correspond to the actual code location.Likely an incorrect or invalid review comment.
.circleci/src/jobs/chromatic-sandboxes.yml (1)
15-31: The review comment is incorrect and should be dismissed.All three
restore_cachesteps in chromatic-sandboxes have correspondingsave_cachesteps in upstream jobs:
- The
buildjob savescode-node-modulesandscripts-node-modulescaches- The
create-sandboxesjob saves thesandboxcacheSince chromatic-sandboxes requires create-sandboxes, and create-sandboxes requires build, all cache keys are populated before chromatic-sandboxes runs. The caches are properly persisted across the workflow dependency chain.
Likely an incorrect or invalid review comment.
.circleci/src/jobs/test-runner-production.yml (2)
22-30: Verify environment variable propagation torestore_cachestep.Environment variables set in a run step via
$BASH_ENVsourcing are local to that step's shell context. The subsequentrestore_cachestep (lines 31–32) may not have access to$sandbox-dirunless CircleCI implicitly persists variables across steps. Verify that this pattern works in your CircleCI environment by testing it in a build, or refactor to ensure the cache key is fully resolved within this step.
16-21: Verify restore_cache semantics and fallback behavior.The two restore_cache steps restore node_modules for code and scripts using checksums of yarn.lock files. Confirm that:
- These cache entries are being populated by corresponding
save_cachesteps in an upstream job (e.g., dependency installation or setup job).- The fallback behavior is appropriate if the exact cache key is not found (CircleCI will attempt partial key matches if provided in a
keyslist, but here only a singlekeysentry is provided). Consider adding fallback keys if an exact match miss should not cause the job to fail..circleci/src/jobs/e2e-production.yml (2)
16-21: Approve cache restoration for node_modules.The restore_cache steps for code and scripts node_modules are well-structured with appropriate checksum-based cache keys.
22-30: Verify that sandbox-dir variable is properly available in cache key computation.The environment variables are correctly written to
$BASH_ENVand sourced, which should propagate them to subsequent steps. However, there is a concern about the cache key on line 32 that uses the${sandbox-dir}variable in the checksum path.
| - restore_cache: | ||
| keys: | ||
| - code-node-modules-{{ checksum "code/yarn.lock" }} | ||
| - restore_cache: | ||
| keys: | ||
| - scripts-node-modules-{{ checksum "scripts/yarn.lock" }} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that corresponding save_cache steps exist and job platform targeting is correct.
The added restore_cache steps look syntactically correct and appropriately keyed by yarn.lock checksums. However, the PR description mentions switching to "saving node_modules in the CircleCI cache with distinct cache keys via a save_cache step," but no save_cache steps are visible in this file.
Several clarifications needed:
-
Missing save_cache steps: Are there corresponding
save_cachesteps defined elsewhere (e.g., in a shared template or different file)? If not, theserestore_cachesteps won't find cached entries on the first run or after yarn.lock changes. -
Windows job confirmation: The PR rationale targets "Windows jobs" specifically, but the executor definition here (
sb_node_22_classic) doesn't indicate platform targeting. Confirm whether thisbench-packagesjob runs exclusively on Windows, or if platform-conditional logic exists elsewhere in your CircleCI config. -
Cache path verification: Verify that the restore paths (
code/node_modules,scripts/node_modules) match the actual locations where node_modules will be installed after yarn install steps.
To verify that save_cache steps exist elsewhere in the codebase, run:
🏁 Script executed:
#!/bin/bash
# Search for save_cache steps targeting code-node-modules or scripts-node-modules
rg -n "save_cache" --type yaml --type yml -A 5 | rg -A 5 "code-node-modules|scripts-node-modules"Length of output: 27
I'll fix the script syntax error and re-verify the codebase for corresponding save_cache steps.
🏁 Script executed:
#!/bin/bash
# Search for save_cache steps with relevant cache keys
rg -n "save_cache" --type yaml -A 10 | rg -B 2 -A 8 "code-node-modules|scripts-node-modules"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Also search for any save_cache definitions to see what caches are being saved
rg -n "save_cache:" --type yaml -A 8Length of output: 0
Let me search more broadly to understand the caching strategy in this repository:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for the cache key names anywhere in YAML files
rg -n "code-node-modules|scripts-node-modules" --type yaml -B 2 -A 2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the structure of the bench-packages.yml file
cat ".circleci/src/jobs/bench-packages.yml"Length of output: 2233
🏁 Script executed:
#!/bin/bash
# Search for all restore_cache steps to understand the overall caching pattern
rg -n "restore_cache:" --type yaml -A 3Length of output: 0
Let me expand the search to find save_cache steps and understand the overall workflow context:
🏁 Script executed:
#!/bin/bash
# Search for save_cache in ALL files (not limited to yaml)
rg -n "save_cache" -A 5Length of output: 353
🏁 Script executed:
#!/bin/bash
# Search for yarn install to understand where node_modules gets created
rg -n "yarn install" -B 2 -A 2Length of output: 9484
🏁 Script executed:
#!/bin/bash
# Find workflow files that reference bench-packages
rg -n "bench-packages" --type yaml -B 3 -A 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any orb definitions or job templates that might contain cache logic
find .circleci -name "*.yml" -o -name "*.yaml" | head -20Length of output: 793
| - save_cache: | ||
| key: sandbox-{{ .BuildNum }}-{{ checksum "sandbox/${sandbox-dir}/package.json" }} | ||
| paths: | ||
| - sandbox/** |
There was a problem hiding this comment.
Major: .BuildNum in cache key prevents cross-build reuse.
Using {{ .BuildNum }} in the cache key means each build receives a unique key, so the cache will never be hit on subsequent builds. If the intent is to cache sandbox builds for reuse within the same build, clarify this. If cross-build reuse is desired, consider using the sandbox package.json checksum alone or a stable identifier.
Example revised key:
key: sandbox-{{ checksum "sandbox/${sandbox-dir}/package.json" }}(Once the variable interpolation issue is resolved.)
🤖 Prompt for AI Agents
In .circleci/src/jobs/create-sandboxes.yml around lines 56 to 59, the cache key
uses {{ .BuildNum }} which makes the key unique per build and prevents cache
reuse; remove the build number and use a stable key (for example the checksum of
sandbox/${sandbox-dir}/package.json or another stable identifier) so the cache
can be reused across builds, and ensure the final key interpolation uses only
stable variables (e.g., key: sandbox-{{ checksum
"sandbox/${sandbox-dir}/package.json" }}).
Critical: Bash variable won't be available in CircleCI cache key context.
The cache key sandbox-{{ .BuildNum }}-{{ checksum "sandbox/${sandbox-dir}/package.json" }} references ${sandbox-dir}, which is set in the previous step via $BASH_ENV. CircleCI's cache key templating ({{ }}) does not interpolate bash variables from previous steps—the variable will be treated as a literal string, causing the checksum path to be invalid.
Workaround options:
- Option 1: Set
sandbox-diras an environment variable in the job'senvironmentsection (requires computing it earlier or in the job root). - Option 2: Export the variable globally by appending to
$HOME/.bashrcor similar persistent location. - Option 3: Use CircleCI's native context parameters or pass the value via a separate mechanism.
🤖 Prompt for AI Agents
In .circleci/src/jobs/create-sandboxes.yml around lines 56-59, the cache key
uses a Bash variable (${sandbox-dir}) inside CircleCI template braces which
won't be expanded; replace this by ensuring the sandbox-dir is available to
CircleCI templating — e.g., set sandbox-dir as a job-level environment variable
(or via a CircleCI context) before the cache step so the checksum expression can
reference the real path, or alternatively compute the checksum in an earlier
step (where you can use Bash), store the result in an env var or file, and use
that concrete value in the cache key; update the save_cache key to reference the
resolved value rather than ${sandbox-dir}.
23c5fb1 to
f8c27d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.circleci/src/jobs/test-runner-production.yml (1)
22-30: CRITICAL (duplicate): Broken sandbox cache key pattern—shell variables won't resolve in CircleCI templates.This same issue was previously flagged. CircleCI evaluates configuration before any environment is set up, so the
{{ checksum "sandbox/${sandbox-template}/package.json" }}pattern (line 30) cannot access the${sandbox-template}variable computed in the prior run step (lines 24-28).Additionally, the cache key is computed using
--task sandbox(line 25), but the actual sandbox install uses--task test-runner(line 34). Different tasks may resolve to different templates, invalidating the cache.Refer to the previous review comment in
.circleci/config.ymlfor the recommended workaround using a file-based checksum approach..circleci/src/jobs/e2e-production.yml (1)
22-30: CRITICAL (duplicate): Two issues prevent cache from functioning—broken template pattern and task mismatch.This issue was previously flagged with detailed analysis:
Template evaluation timing: CircleCI evaluates configuration before any environment is set up. The
{{ checksum "sandbox/${sandbox-template}/package.json" }}key (line 30) cannot access the${sandbox-template}variable from the prior run step.Task parameter mismatch: Line 25 computes the cache key using
--task sandbox, but lines 34+ install/run using--task e2e-tests. If these tasks produce different sandbox directories, the cache key won't match the actual path being used.Refer to the previous review for the recommended file-based checksum workaround and ensure both places use the same task parameter.
.circleci/src/jobs/e2e-dev.yml (1)
22-30: CRITICAL (duplicate): Broken sandbox cache key pattern with task parameter mismatch.This issue was previously flagged. CircleCI evaluates configuration before any environment is set up, so the
{{ checksum "sandbox/${sandbox-template}/package.json" }}(line 30) cannot reference the runtime variable${sandbox-template}from the prior run step.Additionally, line 25 uses
--task sandboxto compute the cache key, but lines 34+ use--task e2e-tests-devfor actual execution. If these tasks resolve to different sandboxes, the cache will be invalid.Refer to previous review comments for the recommended workaround using file-based checksums and task parameter alignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.circleci/config.yml(22 hunks).circleci/src/jobs/bench-packages.yml(1 hunks).circleci/src/jobs/bench-sandboxes.yml(1 hunks).circleci/src/jobs/build.yml(1 hunks).circleci/src/jobs/chromatic-internal-storybook.yml(1 hunks).circleci/src/jobs/chromatic-sandboxes.yml(1 hunks).circleci/src/jobs/create-sandboxes.yml(2 hunks).circleci/src/jobs/e2e-dev.yml(1 hunks).circleci/src/jobs/e2e-production.yml(1 hunks).circleci/src/jobs/e2e-ui-vitest-3.yml(1 hunks).circleci/src/jobs/e2e-ui.yml(1 hunks).circleci/src/jobs/knip.yml(1 hunks).circleci/src/jobs/lint.yml(1 hunks).circleci/src/jobs/script-checks.yml(1 hunks).circleci/src/jobs/stories-tests.yml(1 hunks).circleci/src/jobs/test-init-empty.yml(1 hunks).circleci/src/jobs/test-init-features.yml(1 hunks).circleci/src/jobs/test-portable-stories.yml(1 hunks).circleci/src/jobs/test-runner-production.yml(1 hunks).circleci/src/jobs/test-yarn-pnp.yml(1 hunks).circleci/src/jobs/unit-tests.yml(1 hunks).circleci/src/jobs/vitest-integration.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- .circleci/src/jobs/build.yml
- .circleci/src/jobs/chromatic-sandboxes.yml
- .circleci/src/jobs/create-sandboxes.yml
- .circleci/src/jobs/stories-tests.yml
- .circleci/src/jobs/bench-packages.yml
- .circleci/src/jobs/vitest-integration.yml
- .circleci/src/jobs/test-init-features.yml
- .circleci/src/jobs/e2e-ui.yml
- .circleci/src/jobs/test-portable-stories.yml
- .circleci/src/jobs/test-yarn-pnp.yml
- .circleci/src/jobs/unit-tests.yml
- .circleci/src/jobs/knip.yml
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (10)
.circleci/src/jobs/e2e-ui-vitest-3.yml (1)
10-15: Restore_cache syntax is correct, but clarify platform applicability and workspace necessity.The added restore_cache steps follow proper CircleCI syntax and cache keys correctly align with the save_cache definitions in
build.yml. However, the verification reveals a discrepancy:This job (
e2e-ui-vitest-3) shows no Windows-specific platform indicators—no conditional execution, no Windows executor, and consistent executor naming (sb_playwright) with other non-Windows jobs. Yet the PR objective mentions "avoiding node_modules attachment on Windows jobs."Additionally, the workspace attachment at lines 8-9 remains present. Clarify whether:
- This job should target Windows specifically (no evidence currently suggests it)
- Workspace attachment should be retained or removed for this job
- This change applies only to Windows jobs, or if this is a broader cache optimization unrelated to platform
.circleci/src/jobs/script-checks.yml (2)
6-13: Alignment with PR objective: Decoupling node_modules from workspace.The addition of restore_cache steps immediately after attach_workspace correctly implements the optimization strategy outlined in the PR: by restoring node_modules from cache instead of including them in the workspace, this should improve performance on Windows jobs where one-to-one file operations are costly.
8-13: Cache strategy is correctly implemented.Verification confirms that the restore_cache steps in script-checks.yml are properly configured:
- ✅ Cache keys match exactly: Both
code-node-modules-{{ checksum "code/yarn.lock" }}andscripts-node-modules-{{ checksum "scripts/yarn.lock" }}in script-checks.yml match the corresponding save_cache keys in build.yml- ✅ Caching strategy is complete: build.yml saves node_modules to cache and persists only source artifacts to workspace (with explicit comment: "Persist source/artifacts only (exclude node_modules)")
- ✅ Decoupling is effective: node_modules are now managed via cache instead of workspace attachment, reducing unnecessary data transfer
The changes correctly implement the PR objective to optimize node_modules handling by decoupling them from workspace attachment.
.circleci/src/jobs/bench-sandboxes.yml (2)
14-21: No issues found—yarn.lock files exist at the expected locations.Verification confirms both
code/yarn.lockandscripts/yarn.lockexist in the repository. The cache restoration keys reference valid paths.
16-21: The original review comment is incorrect and can be disregarded.The restore_cache keys in bench-sandboxes.yml exactly match the save_cache keys defined in build.yml. The confusion stems from mixing two different CircleCI cache concepts: the
keyfield (which uses hyphens likecode-node-modules-{{ checksum... }}) identifies the cache and is consistent across all jobs, while thepathsfield (which uses slashes likecode/node_modules) specifies which directories to cache—these serve different purposes and are not interchangeable naming schemes. The cache key identifiers are properly synchronized across all jobs.Likely an incorrect or invalid review comment.
.circleci/src/jobs/lint.yml (1)
10-15: Cache restoration strategy for node_modules looks good.The addition of separate
restore_cachesteps forcode/node_modulesandscripts/node_modulesusing their respectiveyarn.lockchecksums is the correct approach. These use static file paths, so the checksum template functions work properly..circleci/src/jobs/test-init-empty.yml (1)
16-21: Cache restoration strategy for node_modules looks good.The separate
restore_cachesteps forcode/node_modulesandscripts/node_modulesusing their respectiveyarn.lockchecksums are correctly implemented with static file paths..circleci/config.yml (2)
413-414: Sandbox cache cleanup and save strategy needs clarity.Line 413-414 removes
node_modulesfrom the sandbox before caching to "keep cache light". However, line 430-433 then saves all sandbox contents. This means:
- Sandbox dependencies won't be in the cache (they're deleted)
- Later jobs that restore this cache will need to reinstall dependencies
- This may defeat the purpose of sandbox caching
Verify that this is intentional—the cache appears to be for the sandbox code/structure, not for its dependencies. If that's the intent, document it clearly; otherwise, consider whether removing
node_modulesbefore cache save contradicts the caching strategy.Also applies to: 430-433
887-903: Windows job (test-init-empty-windows) does not use centralized node_modules caching.Unlike the Linux equivalent (
test-init-empty), this Windows job installs dependencies directly viayarn install(lines 905-913) and does not restore the centralizedcode/node_modulesorscripts/node_modulescaches. This appears intentional per the PR's goal to avoid attaching node_modules on Windows due to file operation overhead.However, verify that:
- Windows build times are acceptable without pre-cached node_modules
- The install steps on Windows (lines 905-913) don't duplicate or conflict with any cached state
- If this approach is successful, consider documenting it as the recommended pattern for Windows jobs
.circleci/src/jobs/chromatic-internal-storybook.yml (1)
13-18: Cache restoration strategy for node_modules looks good.The separate
restore_cachesteps forcode/node_modulesandscripts/node_modulesare correctly implemented with static file paths and properly placed before the Chromatic task.
valentinpalkovic
force-pushed
the
valentin/speed-up-windows-jobs-on-ci
branch
from
f8c27d6 to
5174882
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.circleci/config.yml (1)
343-351: 🔴 CRITICAL: Sandbox cache key pattern cannot interpolate shell variables—persists from previous review.The pattern at lines 343–351 (and repeated at lines 419–427, 451–459, 496–504, 1164–1172, 1263–1271) cannot work as intended. CircleCI evaluates
{{ checksum }}templates at config parse time, before any run steps execute. The shell variable${sandbox-template}exported via$BASH_ENVis not available at config parsing time, so the cache key contains the literal string${sandbox-template}instead of the actual template value (e.g.,"react"). This causes all sandbox cache lookups to fail.Fix: Compute the checksum of a stable file path instead. Write the computed sandbox template to a file, then checksum that file:
- run: command: | TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task sandbox) echo "sandbox-template=$TEMPLATE" >> $BASH_ENV + # Write the sandbox package.json path to a stable file for checksum + echo "sandbox/$TEMPLATE/package.json" > .sandbox-path.txt source $BASH_ENV echo "Computed sandbox template: $sandbox-template" - restore_cache: - key: sandbox-{{ .BuildNum }}-{{ checksum "sandbox/${sandbox-template}/package.json" }} + key: sandbox-{{ .BuildNum }}-{{ checksum ".sandbox-path.txt" }}Apply this pattern to all six affected cache operations (lines 351, 427, 459, 504, 1172, 1271) and corresponding
save_cachesteps (line 427 in create-sandboxes).
🧹 Nitpick comments (1)
.circleci/config.yml (1)
887-905: Consider adding cache restoration to Windows test job.The
test-init-empty-windowsjob (lines 879–1043) manually installs dependencies with separateyarn installcommands forcodeandscriptsdirectories. While this is necessary for Windows, consider whether pre-restoring node_modules caches (if they exist from prior builds) would speed up the job, similar to the pattern used in other jobs. This is optional but may improve Windows job performance.If cache restoration would help, add restore_cache steps after
attach_workspaceand before the manual install commands (around line 888):- attach_workspace: at: . + - restore_cache: + keys: + - code-node-modules-{{ checksum "code/yarn.lock" }} + - restore_cache: + keys: + - scripts-node-modules-{{ checksum "scripts/yarn.lock" }} - run: command: | choco install nodejs-lts --version=22.11.0 -yHowever, verify that Windows builds produce compatible node_modules with Linux builds; if not, this optimization may not be applicable (which aligns with the PR rationale about Windows builds being different).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.circleci/config.yml(22 hunks).circleci/src/jobs/bench-packages.yml(1 hunks).circleci/src/jobs/bench-sandboxes.yml(1 hunks).circleci/src/jobs/build.yml(1 hunks).circleci/src/jobs/chromatic-internal-storybook.yml(1 hunks).circleci/src/jobs/chromatic-sandboxes.yml(1 hunks).circleci/src/jobs/create-sandboxes.yml(2 hunks).circleci/src/jobs/e2e-dev.yml(1 hunks).circleci/src/jobs/e2e-production.yml(1 hunks).circleci/src/jobs/e2e-ui-vitest-3.yml(1 hunks).circleci/src/jobs/e2e-ui.yml(1 hunks).circleci/src/jobs/knip.yml(1 hunks).circleci/src/jobs/lint.yml(1 hunks).circleci/src/jobs/script-checks.yml(1 hunks).circleci/src/jobs/stories-tests.yml(1 hunks).circleci/src/jobs/test-init-empty.yml(1 hunks).circleci/src/jobs/test-init-features.yml(1 hunks).circleci/src/jobs/test-portable-stories.yml(1 hunks).circleci/src/jobs/test-runner-production.yml(1 hunks).circleci/src/jobs/test-yarn-pnp.yml(1 hunks).circleci/src/jobs/unit-tests.yml(1 hunks).circleci/src/jobs/vitest-integration.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- .circleci/src/jobs/e2e-ui.yml
- .circleci/src/jobs/bench-packages.yml
- .circleci/src/jobs/lint.yml
- .circleci/src/jobs/test-init-features.yml
- .circleci/src/jobs/e2e-dev.yml
- .circleci/src/jobs/unit-tests.yml
- .circleci/src/jobs/e2e-ui-vitest-3.yml
- .circleci/src/jobs/test-init-empty.yml
- .circleci/src/jobs/vitest-integration.yml
- .circleci/src/jobs/chromatic-internal-storybook.yml
- .circleci/src/jobs/build.yml
- .circleci/src/jobs/stories-tests.yml
- .circleci/src/jobs/test-runner-production.yml
- .circleci/src/jobs/bench-sandboxes.yml
- .circleci/src/jobs/create-sandboxes.yml
- .circleci/src/jobs/chromatic-sandboxes.yml
- .circleci/src/jobs/e2e-production.yml
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
.circleci/src/jobs/test-yarn-pnp.yml (1)
10-15: ✓ Correct cache restoration pattern for node_modules.The restore_cache steps correctly use literal filenames in the checksum template, ensuring deterministic cache keys based on yarn.lock files. Placement before the install step is appropriate.
.circleci/src/jobs/script-checks.yml (1)
8-13: ✓ Consistent cache restoration with node_modules checkpoint.Mirrors the correct pattern from other jobs; restore_cache keys reference literal yarn.lock files.
.circleci/config.yml (1)
228-235: ✓ Code and scripts node_modules caching—correct pattern.The separate
save_cacheandrestore_cachefor code/scripts node_modules (lines 228–235 and throughout) use correct syntax with literal yarn.lock filenames. These caches will function as intended..circleci/src/jobs/test-portable-stories.yml (1)
14-19: ****The original review comment based on the concern that restore_cache without an explicit
pathparameter would fail to align with the subdirectory yarn install is not accurate. The cache will be restored to the location from which it was originally saved, which means the root-levelcode/node_modulesandscripts/node_modulescached inbuild.ymlwill be correctly restored to their original root paths regardless of whereyarn installruns.The script output confirms that this caching pattern is consistent and intentional across the entire codebase (20+ jobs), with
save_cachesteps inbuild.ymlsaving the root dependencies andrestore_cachesteps in all downstream jobs (includingtest-portable-stories.yml) using matching keys. Subdirectories that runyarn installwith differentworking_directoryvalues install their own localnode_modules, separate from the cached root-level dependencies. This is a valid monorepo design pattern.No code changes are needed.
Likely an incorrect or invalid review comment.
.circleci/src/jobs/knip.yml (2)
8-15: Original review comment is incorrect.The git diff confirms the changes ADDED two
restore_cachesteps;attach_workspacewas not replaced or removed. The claim that the summary states the step was "replaced" contradicts the actual code changes. Additionally, this job runs onsb_node_22_classic(a Linux executor), not Windows. Thesave_cachesteps that populate these caches exist in thebuild.ymljob, confirming the cache strategy is valid. Bothattach_workspaceandrestore_cachecan coexist without semantic conflict—this is a straightforward optimization to restore specific dependency caches.Likely an incorrect or invalid review comment.
10-15: Cache keys and paths are properly configured; verification complete.The cache strategy is in place:
build.ymlcontainssave_cachesteps for bothcode-node-modulesandscripts-node-moduleskeys, which match therestore_cachekeys inknip.ymlcode/yarn.lockandscripts/yarn.lockexist in the repository- The
sb_node_22_classicexecutor uses a Docker image (cimg/node:22.15.0), not a Windows executor; the cache strategy is for Linux environments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/get-template.ts (2)
53-63: ClarifyisLocalSandboxgating and guard against out-of-range indicesGating the parallelism mismatch check on
!isLocalSandboxmeans that as soon assandboxDirexists (including potentially on CI), misconfiguredCIRCLE_NODE_TOTALvalues will no longer throw here and can instead lead toundefinedbeing returned for someindexvalues. Please confirm thatisLocalSandboxreally only covers the “developer/local” case and that CI still has a deterministic failure mode when parallelism is wrong; otherwise consider tightening the condition or re-enabling the check in CI contexts.Independently of that, it would be safer to guard the index before returning to avoid ever emitting
undefined:- if (!isLocalSandbox && potentialTemplateKeys.length !== total) { + if (!isLocalSandbox && potentialTemplateKeys.length !== total) { throw new Error(dedent`Circle parallelism set incorrectly. @@ } - return potentialTemplateKeys[index]; + if (index < 0 || index >= potentialTemplateKeys.length) { + throw new Error( + `Template index ${index} out of range for ${potentialTemplateKeys.length} templates` + ); + } + + return potentialTemplateKeys[index];Also applies to: 77-90
335-341: Unary+conversion is fine;Number(...)could be slightly clearerThe new “Convert to integer” comment accurately documents the existing use of unary
+for coercion, so behavior is unchanged and OK. If you want a touch more readability,Number(CIRCLE_NODE_INDEX)/Number(CIRCLE_NODE_TOTAL)would make the intent explicit, but that’s purely stylistic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/get-template.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/get-template.ts (1)
scripts/ecosystem-ci/before-test.js (1)
sandboxDir(34-34)
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
valentinpalkovic
force-pushed
the
valentin/speed-up-windows-jobs-on-ci
branch
from
401f4d0 to
e50d7dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.circleci/src/jobs/create-sandboxes.yml (1)
53-56: Cache key uses per-build isolation.The
sandbox-{{ .BuildNum }}-prefix ensures each build receives a unique sandbox cache key, preventing interference from prior builds. This is appropriate for CI safety, though it means sandbox caches won't be reused across builds. If cross-build reuse is desired, consider removing.BuildNum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(24 hunks).circleci/src/jobs/check-sandboxes.yml(1 hunks).circleci/src/jobs/chromatic-sandboxes.yml(1 hunks).circleci/src/jobs/create-sandboxes.yml(2 hunks).circleci/src/jobs/e2e-dev.yml(1 hunks).circleci/src/jobs/e2e-production.yml(1 hunks).circleci/src/jobs/test-runner-dev.yml(1 hunks).circleci/src/jobs/test-runner-production.yml(1 hunks).circleci/src/jobs/vitest-integration.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .circleci/src/jobs/test-runner-production.yml
- .circleci/src/jobs/e2e-production.yml
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (10)
.circleci/src/jobs/check-sandboxes.yml (1)
16-29: Cache key strategy properly implemented.The multi-step cache restoration pattern is sound: computing the template, persisting the path to a file, then checksumming that file for the cache key avoids the shell variable interpolation issue. This approach is consistent across all updated jobs.
.circleci/src/jobs/e2e-dev.yml (1)
22-29: Verify task parameter consistency across sandbox operations.Line 25 uses
--task e2e-tests-devto compute the template, but verify this is the same task parameter used in the "Install sandbox dependencies" step (line 33). If they differ, the sandbox template could mismatch the cache key..circleci/src/jobs/vitest-integration.yml (1)
16-29: LGTM.Task parameter (
vitest-integration) is consistently used in both the compute step and install dependencies step, ensuring proper cache key alignment..circleci/src/jobs/chromatic-sandboxes.yml (1)
15-28: Pattern correctly applied.Cache key computation and restoration follow the established pattern with consistent task parameters throughout the job.
.circleci/src/jobs/create-sandboxes.yml (1)
40-41: Node_modules cleanup before caching is intentional and correct.Removing node_modules before persisting the sandbox cache reduces cache size and avoids persisting platform-specific builds. Downstream jobs properly restore cache and rebuild node_modules via
yarn..circleci/src/jobs/test-runner-dev.yml (1)
22-29: Pattern correctly applied.Sandbox cache computation and restoration are properly sequenced before the dependency installation step.
.circleci/config.yml (4)
228-235: Separate cache saves for code and scripts node_modules.Splitting the combined
build-yarn-2-cache-v5-cache into two separate caches (code-node-modules-andscripts-node-modules-) allows more granular cache invalidation. The build job properly saves both; dependent jobs restore both. This improves cache hit rate when only one lockfile changes.
119-124: Consistent cache restoration pattern across 20+ jobs.All affected jobs follow the correct pattern:
- Restore code/node-modules cache (checksum of code/yarn.lock)
- Restore scripts/node-modules cache (checksum of scripts/yarn.lock)
- For sandbox-dependent jobs: compute key, restore sandbox cache
This consistency ensures predictable behavior and proper cache alignment across the CI system.
Also applies to: 186-191, 290-295, 351-356, 401-406, 457-462, 501-506, 541-546, 603-608, 625-630, 671-676, 726-731, 756-761, 1063-1068, 1106-1111, 1148-1153, 1182-1187, 1224-1229, 1251-1256, 1280-1285
212-214: Build job maintains backward compatibility with old cache key during transition.Restoring the old
build-yarn-2-cache-v5-cache ensures the build job can use existing caches while new split caches are being populated. This is a safe transition strategy. Once the new caches populate, the old key will rarely match, but having it as a fallback prevents cache cold-starts during the rollout.
1-1: Missing verification for scripts/get-template.ts changes.The AI summary mentions "makes a small local-sandbox detection tweak in
scripts/get-template.ts", but this file is not provided for review. The cache key computation depends on the output ofyarn get-template, so changes to this script could impact cache key stability.Please provide
scripts/get-template.tsor confirm that changes there are compatible with the new cache key computation strategy (i.e., same template name is returned consistently for the same workflow/task).
| # Clean node_modules before caching sandbox to keep cache light | ||
| cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && rm -rf node_modules |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Verify node_modules cleanup doesn't break subsequent jobs.
The sandbox node_modules are intentionally removed (line 425: rm -rf node_modules) before the sandbox cache is saved. Verify that all downstream jobs that restore this cache properly rebuild node_modules by running yarn before execution. This is a key requirement for the Windows optimization to work.
Confirm that jobs consuming the sandbox cache (check-sandboxes, chromatic-sandboxes, e2e-, test-runner-) all include a step like:
cd sandbox/... && yarn
after restoring the sandbox cache and before running tests/tasks.
Generate a quick verification script:
🏁 Script executed:
#!/bin/bash
# Verify all sandbox-dependent jobs run yarn after cache restore
rg -B5 "Install sandbox dependencies" .circleci/config.yml | rg "cd sandbox.*&& yarn"Length of output: 587
🏁 Script executed:
#!/bin/bash
# Enhanced verification: Check cache restore → yarn order for each job
# Focus on the specific jobs mentioned in the review
echo "=== Checking check-sandboxes job ==="
rg -A20 "check-sandboxes:" .circleci/config.yml | rg -E "(restore-cache|cache-restore|cd sandbox.*&& yarn)" -C2
echo ""
echo "=== Checking chromatic-sandboxes job ==="
rg -A20 "chromatic-sandboxes:" .circleci/config.yml | rg -E "(restore-cache|cache-restore|cd sandbox.*&& yarn)" -C2
echo ""
echo "=== Checking e2e-* jobs (first e2e job) ==="
rg -A20 '"e2e-' .circleci/config.yml | head -40 | rg -E "(restore-cache|cache-restore|cd sandbox.*&& yarn)" -C2
echo ""
echo "=== Checking test-runner-* jobs (first test-runner job) ==="
rg -A20 '"test-runner-' .circleci/config.yml | head -40 | rg -E "(restore-cache|cache-restore|cd sandbox.*&& yarn)" -C2
echo ""
echo "=== Checking cache save steps that follow node_modules cleanup ==="
rg -B3 -A5 "rm -rf node_modules" .circleci/config.yml | rg -E "(save-cache|cache-save|rm -rf)"Length of output: 2542
🏁 Script executed:
#!/bin/bash
# Verify sandbox cache restore → yarn order for all sandbox-dependent jobs
echo "=== Jobs using restore-sandbox-cache and yarn steps ==="
rg -n "restore-sandbox-cache|Install sandbox dependencies" .circleci/config.yml -A10 | head -100
echo ""
echo "=== Specific job definitions with cache/yarn context ==="
rg -n "check-sandboxes:|chromatic-sandboxes:|e2e-|test-runner-" .circleci/config.yml -A15 | grep -E "check-sandboxes:|chromatic-sandboxes:|e2e-|test-runner-|restore-sandbox-cache|cd sandbox.*yarn" | head -50Length of output: 8655
🏁 Script executed:
#!/bin/bash
# Get full job definitions with all steps to verify cache restore → yarn sequence
echo "=== Full check-sandboxes job definition ==="
rg -n "check-sandboxes:" .circleci/config.yml -A30
echo ""
echo "=== Full chromatic-sandboxes job definition ==="
rg -n "chromatic-sandboxes:" .circleci/config.yml -A30
echo ""
echo "=== Full test-runner-production job definition ==="
rg -n "test-runner-production:" .circleci/config.yml -A30
echo ""
echo "=== Full test-runner-dev job definition ==="
rg -n "test-runner-dev:" .circleci/config.yml -A30 | head -35Length of output: 16391
Critical: test-runner-dev missing yarn rebuild after cache restore.
Verification confirms the concern in the original review comment. While check-sandboxes, chromatic-sandboxes, e2e-dev, e2e-production, and test-runner-production all properly rebuild node_modules with cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn after restoring the sandbox cache, test-runner-dev does not.
At line 1163 of .circleci/config.yml, test-runner-dev goes directly to the yarn task command without the preceding yarn rebuild. This breaks the cache chain since node_modules were intentionally removed before caching.
Fix required: Add the yarn rebuild step to test-runner-dev after the restore_cache at line 1161 and before the yarn task command at line 1163.
🤖 Prompt for AI Agents
In .circleci/config.yml around lines 1161 to 1163, test-runner-dev restores the
sandbox cache but omits the required rebuild of node_modules; add a step after
the restore_cache and before the existing yarn task to cd into the sandbox
folder (cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE)) and run yarn to
rebuild dependencies so node_modules are present (matching the other jobs'
pattern).
e50d7dc to
353958f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.circleci/config.yml (1)
1154-1164: Critical: test-runner-dev is missing the yarn rebuild step after sandbox cache restore.All other sandbox-consuming jobs (check-sandboxes, chromatic-sandboxes, e2e-*, test-runner-production, vitest-integration) rebuild node_modules with
cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarnafter restoring the sandbox cache. Since node_modules are intentionally stripped before caching (line 425), this step is essential.test-runner-dev restores the cache at line 1160-1161 but skips directly to the task command at line 1163 without rebuilding.
Insert the yarn rebuild step between the cache restore (line 1161) and the task command (line 1163):
- restore_cache: key: sandbox-{{ .BuildNum }}-{{ checksum ".sandbox-package-path.txt" }} + - run: + command: | + TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task test-runner-dev) + cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn + name: Install sandbox dependencies - run: command: yarn task --task test-runner-dev ... name: Running Test Runner in Dev mode
🧹 Nitpick comments (2)
.circleci/src/jobs/e2e-production.yml (1)
22-27: Consider memoizing TEMPLATE to avoid redundant script invocations.The template is computed four times using
yarn get-template --cadence << pipeline.parameters.workflow >> --task e2e-tests:
- Line 25 (compute step)
- Line 33 (install step)
- Line 42 (test step)
- Line 44 (reporting step)
Since all calls use identical parameters, they will always return the same result. To reduce overhead, consider writing the result to an env file (e.g.,
.template-cache) that can be sourced in subsequent steps, or using CircleCI's native env persistence mechanisms.Example approach:
- run: name: Compute sandbox cache key command: | TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task e2e-tests) echo "sandbox/$TEMPLATE/package.json" > .sandbox-package-path.txt echo "TEMPLATE=$TEMPLATE" >> $BASH_ENV echo "Computed sandbox template: $TEMPLATE"Then in subsequent run steps,
$TEMPLATEwill be available after$BASH_ENVis sourced (CircleCI does this automatically between steps).Also applies to: 33-33, 42-42, 44-44
.circleci/src/jobs/check-sandboxes.yml (1)
22-27: Consider cachingyarn get-templateoutput to reduce redundant invocations.Lines 25 and 33 both invoke
yarn get-template --cadence << pipeline.parameters.workflow >> --task check-sandbox, computing the same value twice. While likely idempotent, storing the result in an environment variable or output file and sourcing it would be more efficient.Apply this refactor to compute the template once and reuse it:
- run: name: Compute sandbox cache key command: | TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task check-sandbox) echo "sandbox/$TEMPLATE/package.json" > .sandbox-package-path.txt echo "export SANDBOX_TEMPLATE=$TEMPLATE" >> $BASH_ENV echo "Computed sandbox template: $TEMPLATE"Then update the install step to use the cached variable:
- run: name: Install sandbox dependencies command: | - TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task check-sandbox) + source $BASH_ENV - cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn + cd sandbox/$(yarn get-sandbox-dir --template $SANDBOX_TEMPLATE) && yarnThis avoids re-computing the template and improves job runtime.
Also applies to: 30-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(24 hunks).circleci/src/jobs/check-sandboxes.yml(1 hunks).circleci/src/jobs/chromatic-sandboxes.yml(1 hunks).circleci/src/jobs/create-sandboxes.yml(2 hunks).circleci/src/jobs/e2e-dev.yml(1 hunks).circleci/src/jobs/e2e-production.yml(1 hunks).circleci/src/jobs/test-runner-dev.yml(1 hunks).circleci/src/jobs/test-runner-production.yml(1 hunks).circleci/src/jobs/vitest-integration.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .circleci/src/jobs/test-runner-dev.yml
- .circleci/src/jobs/vitest-integration.yml
- .circleci/src/jobs/e2e-dev.yml
- .circleci/src/jobs/test-runner-production.yml
- .circleci/src/jobs/chromatic-sandboxes.yml
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (7)
.circleci/src/jobs/e2e-production.yml (1)
16-29: ✅ Cache key fix properly implemented using file-checksum workaround.The previous critical issues regarding task parameter mismatch and cache key template expansion have been correctly addressed:
- Line 25 and subsequent uses consistently employ
--task e2e-tests(no longer--task sandbox)- Lines 26–29 implement the correct file-checksum pattern: compute sandbox path, write to
.sandbox-package-path.txt, then use{{ checksum ".sandbox-package-path.txt" }}in the cache keyThe file is created before
restore_cacheexecutes, so CircleCI will resolve the checksum at runtime when the step runs. Cache scoping per BuildNum aligns with the PR objective to cache per-workflow..circleci/src/jobs/create-sandboxes.yml (3)
16-21: Cache restoration for dependencies looks correct.Both
code/node_modulesandscripts/node_modulescaches are restored with appropriate yarn.lock checksums. This properly precedes the build steps and aligns with moving dependencies out of the workspace.
40-41: Sandbox cleanup before caching is appropriate.The inline comment and cleanup command correctly remove node_modules to keep the cached sandbox artifact lean. Downstream jobs will perform fresh installs as needed.
47-56: Sandbox cache key computation resolves the critical interpolation issue.The approach of computing the template path in bash (line 50) and writing it to a concrete file (line 51) for checksum (line 54) correctly addresses the previous review's critical issue: CircleCI's template context cannot directly interpolate bash variables, but it can checksum concrete files.
The cache key uses
.BuildNumto keep caches isolated per workflow, which aligns with the stated PR objective of "reusing generated sandboxes across jobs in the same workflow." Within a single build, jobs consuming this cache will share the sandbox artifact, reducing redundant setup overhead..circleci/config.yml (2)
296-303: Verify sandbox cache fallback keys are in place or add them per CircleCI best practices.The past review flagged that sandbox cache restore operations lack fallback keys, which can cause first-run failures when
checksum ".sandbox-package-path.txt"is evaluated. Although marked as "addressed," the current code shows single-key restore patterns without fallback chains (e.g., lines 302–303, 363–364, 470, 514, 1161, 1195, 1293).If fallback keys weren't added as part of the prior commits, add them to ensure graceful degradation on first run or when the checksum file doesn't exist:
- restore_cache: - key: sandbox-{{ .BuildNum }}-{{ checksum ".sandbox-package-path.txt" }} + keys: + - sandbox-{{ .BuildNum }}-{{ checksum ".sandbox-package-path.txt" }} + - sandbox-{{ .BuildNum }}-Alternatively, if the prior commits used a different mitigation (e.g., ensuring the file always exists before checksum evaluation), confirm that approach is working as intended.
Also applies to: 357-364, 463-470, 508-514, 1154-1161, 1188-1195, 1286-1293
228-235: Cache split approach is sound.The refactoring to split build-yarn-2-cache into separate code-node-modules and scripts-node-modules caches keyed by their respective yarn.lock files is a clean, modular design that aligns with the PR's goal of optimizing Windows jobs by moving dependency trees out of workspace persistence.
.circleci/src/jobs/check-sandboxes.yml (1)
16-21: LGTM!The restore_cache steps for code and scripts node_modules are correctly keyed by their respective yarn.lock checksums, enabling dependency invalidation and alignment with the PR's caching strategy.
| - run: | ||
| name: Compute sandbox cache key | ||
| command: | | ||
| TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task check-sandbox) | ||
| echo "sandbox/$TEMPLATE/package.json" > .sandbox-package-path.txt | ||
| echo "Computed sandbox template: $TEMPLATE" | ||
| - restore_cache: | ||
| key: sandbox-{{ .BuildNum }}-{{ checksum ".sandbox-package-path.txt" }} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cache producer saves sandbox cache with matching key format.
The job restores sandbox cache using sandbox-{{ .BuildNum }}-{{ checksum ".sandbox-package-path.txt" }}, which keys caches per workflow by template path. For this to work, a producer job earlier in the workflow must save the cache using this identical key format after building the sandbox.
Please confirm that the sandbox cache is being produced and saved with a matching key format elsewhere in the workflow (likely in a build/setup job). Run the following to locate cache save operations:
🏁 Script executed:
#!/bin/bash
# Search for save_cache steps that reference 'sandbox' in CircleCI config
find .circleci -name "*.yml" -o -name "*.yaml" | xargs grep -n "save_cache" | grep -i sandboxLength of output: 58
🏁 Script executed:
#!/bin/bash
# Examine the save_cache step in create-sandboxes.yml
sed -n '45,65p' .circleci/src/jobs/create-sandboxes.ymlLength of output: 590
🏁 Script executed:
#!/bin/bash
# Check if create-sandboxes.yml computes .sandbox-package-path.txt the same way
grep -n "sandbox-package-path" .circleci/src/jobs/create-sandboxes.ymlLength of output: 157
🏁 Script executed:
#!/bin/bash
# Show broader context around the save_cache step
sed -n '40,70p' .circleci/src/jobs/create-sandboxes.ymlLength of output: 872
Fix task parameter mismatch in sandbox cache key computation.
The producer job (create-sandboxes.yml) and consumer job (check-sandboxes.yml) use different --task parameters when computing the TEMPLATE variable, causing cache key mismatches:
- Producer (create-sandboxes.yml:48-50):
yarn get-template --cadence << pipeline.parameters.workflow >> --task sandbox - Consumer (check-sandboxes.yml:25):
yarn get-template --cadence << pipeline.parameters.workflow >> --task check-sandbox
These different task values produce different TEMPLATE results, leading to different checksums of .sandbox-package-path.txt, which means the cache restore in check-sandboxes.yml will never match the cache saved by create-sandboxes.yml. Change line 25 in check-sandboxes.yml to use --task sandbox instead of --task check-sandbox.
🤖 Prompt for AI Agents
In .circleci/src/jobs/check-sandboxes.yml around lines 22 to 29, the compute
sandbox cache key step uses `--task check-sandbox` which produces a different
TEMPLATE than the producer job (which uses `--task sandbox`) and thus yields a
mismatched checksum and never restores the cache; update the command to call
`yarn get-template --cadence << pipeline.parameters.workflow >> --task sandbox`
so both producer and consumer compute the same TEMPLATE and the cache key
matches.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.circleci/config.yml (2)
1148-1169: test-runner-dev missing sandbox dependency rebuild after cache restore.Line 1160–1161 restores the sandbox cache (which has node_modules intentionally removed per line 425), but then line 1163 immediately runs the task without rebuilding node_modules via
cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn. This breaks the caching strategy: the restored sandbox has no node_modules, so the task execution will fail.Compare against test-runner-production (lines 1197–1200), which correctly includes the "Install sandbox dependencies" step after restore_cache.
Apply this diff to add the missing step:
- restore_cache: key: sandbox-{{ .Revision }}-{{ checksum ".sandbox-package-path.txt" }} + - run: + command: | + TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task test-runner-dev) + cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn + name: Install sandbox dependencies - run: command: yarn task --task test-runner-dev --template $(yarn get-template --cadence << pipeline.parameters.workflow >> --task test-runner-dev) --no-link --start-from=never --junit name: Running Test Runner in Dev mode
696-715: smoke-test-sandboxes missing code/scripts and sandbox cache management.This job lacks the cache restoration pattern that all other sandbox-consuming jobs follow. It does not:
- Restore
code/node_modulescache (missing:restore_cache: key: code-node-modules-{{ checksum "code/yarn.lock" }})- Restore
scripts/node_modulescache (missing:restore_cache: key: scripts-node-modules-{{ checksum "scripts/yarn.lock" }})- Restore sandbox cache (missing: sandbox cache key computation and restore)
- Install sandbox dependencies (missing:
cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn)This defeats the caching optimization introduced in this PR and will force slow dependency reinstallation on every run, degrading performance especially on Windows.
Align smoke-test-sandboxes with the pattern used in bench-sandboxes (lines 186–196):
- attach_workspace: at: . + - restore_cache: + keys: + - code-node-modules-{{ checksum "code/yarn.lock" }} + - restore_cache: + keys: + - scripts-node-modules-{{ checksum "scripts/yarn.lock" }} + - run: + command: | + TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task smoke-test) + echo "sandbox/$TEMPLATE/package.json" > .sandbox-package-path.txt + echo "Computed sandbox template: $TEMPLATE" + name: Compute sandbox cache key + - restore_cache: + key: sandbox-{{ .Revision }}-{{ checksum ".sandbox-package-path.txt" }} + - run: + command: | + TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task smoke-test) + cd sandbox/$(yarn get-sandbox-dir --template $TEMPLATE) && yarn + name: Install sandbox dependencies - run: command: yarn task --task smoke-test --template $(yarn get-template --cadence << pipeline.parameters.workflow >> --task smoke-test) --no-link --start-from=never --junit name: Smoke Testing Sandboxes
🧹 Nitpick comments (1)
.circleci/src/jobs/test-runner-dev.yml (1)
22-34: Refactor to compute sandbox template once and reuse.
yarn get-templateis called three times (lines 25, 32, and 34), recomputing the same value. Extract the template to a shell variable or file and reuse it to avoid redundant work, especially important in a CI environment.Apply this diff to compute the template once and reuse it:
- run: name: Compute sandbox cache key command: | + export TEMPLATE=$(yarn get-template --cadence << pipeline.parameters.workflow >> --task test-runner-dev) + echo "sandbox/$TEMPLATE/package.json" > .sandbox-package-path.txt + echo "export TEMPLATE=$TEMPLATE" >> $BASH_ENV + echo "Computed sandbox template: $TEMPLATE" - restore_cache: key: sandbox-{{ .Revision }}-{{ checksum ".sandbox-package-path.txt" }} - run: name: Running Test Runner in Dev mode - command: yarn task --task test-runner-dev --template $(yarn get-template --cadence << pipeline.parameters.workflow >> --task test-runner-dev) --no-link --start-from=never --junit + command: yarn task --task test-runner-dev --template $TEMPLATE --no-link --start-from=never --junit - report-workflow-on-failure: - template: $(yarn get-template --cadence << pipeline.parameters.workflow >> --task test-runner-dev) + template: $TEMPLATENote: Using
$BASH_ENVto persist the variable across steps ensures the environment variable is available in subsequent steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.circleci/config.yml(24 hunks).circleci/src/jobs/check-sandboxes.yml(1 hunks).circleci/src/jobs/chromatic-sandboxes.yml(1 hunks).circleci/src/jobs/create-sandboxes.yml(2 hunks).circleci/src/jobs/e2e-dev.yml(1 hunks).circleci/src/jobs/e2e-production.yml(1 hunks).circleci/src/jobs/test-runner-dev.yml(1 hunks).circleci/src/jobs/test-runner-production.yml(1 hunks).circleci/src/jobs/vitest-integration.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .circleci/src/jobs/chromatic-sandboxes.yml
- .circleci/src/jobs/check-sandboxes.yml
- .circleci/src/jobs/create-sandboxes.yml
- .circleci/src/jobs/e2e-dev.yml
- .circleci/src/jobs/e2e-production.yml
- .circleci/src/jobs/vitest-integration.yml
⏰ 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: daily
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
.circleci/src/jobs/test-runner-production.yml (3)
16-21: Cache restoration for code and scripts dependencies looks correct.The two restore_cache steps use deterministic keys based on yarn.lock checksums and follow standard patterns.
22-29: Cache key computation correctly addresses the prior critical issue.The implementation now writes the resolved sandbox path to .sandbox-package-path.txt before checksumming, avoiding the problem of using shell variables inside the
checksum()template function. This is a valid workaround that ensures the cache key is deterministic and stable per template/revision combination.Please verify:
- That
yarn get-templatereturns deterministically the same value across repeated calls in the same job (it's called on line 25 and again on line 33).- Consider whether .sandbox-package-path.txt should be cleaned up after use to avoid accumulation across runs, or if it's safe to leave in place.
30-34: Install step logic is sound.The step re-computes the template and sandbox directory, which slightly duplicates work done in the "Compute sandbox cache key" step. While this is a minor efficiency consideration, it ensures the step is self-contained and doesn't depend on prior variable state, which is a safe pattern in CircleCI.
.circleci/src/jobs/test-runner-dev.yml (2)
16-21: Cache restore strategy for code and scripts node_modules looks solid.The separation of node_modules caches keyed by their respective yarn.lock files is a clean way to avoid persisting them through the workspace. This should significantly reduce attach_workspace overhead on Windows.
28-29: No action required - sandbox cache is properly managed.The save_cache step already exists in
create-sandboxes(line 53-54 of.circleci/src/jobs/create-sandboxes.yml) with the exact same key thattest-runner-devrestores. Both usesandbox-{{ .Revision }}-{{ checksum ".sandbox-package-path.txt" }}consistently.Note:
test-runner-devdoesn't currently appear in active workflow definitions (normal, merged, daily), though the job definition exists. If this job is being used, ensure it includesrequires: [create-sandboxes]as a dependency.Likely an incorrect or invalid review comment.
Closes #
What I did
This PR speeds up our Windows jobs on CircleCI by moving large dependency trees out of the workspace and into caches. On Windows, attach_workspace with thousands of files (especially node_modules) is noticeably slow. Instead, the build job now saves code and scripts node_modules to CircleCI cache keyed by their respective yarn.lock files, and only persists sources and artifacts to the workspace. Downstream jobs restore these caches before running, which dramatically reduces setup overhead without changing any task logic.
For sandboxes, we now cache the built sandbox directory per workflow. After creating a sandbox, we strip its node_modules and save the entire sandbox folder under a key composed of the build number and a checksum of the target template path. Jobs that consume sandboxes (type-check, Chromatic, E2E, test runner, Vitest integration) restore this cache and proceed with a normal install inside the sandbox. This reuses the generated sandbox across jobs in the same workflow while keeping caches lean.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
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.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 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>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.