Skip to content

Conversation

@kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Nov 28, 2025

Closes #

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team 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

  • Chores
    • Aligned Windows CI workspace and working directory for more reliable Windows runs.
    • Reordered sandbox install steps so sandbox artifacts are prepared after navigating to the sandbox directory (improves Windows sandbox reliability).
    • Updated a scaffold script to apply npm registry changes globally so registry adjustments persist across sessions.
    • Updated project template to track the next prerelease of Angular Forms and removed legacy inline comments.

✏️ Tip: You can customize this high-level summary in your review settings.

@kasperpeulen kasperpeulen added ci:merged Run the CI jobs that normally run when merged. build Internal-facing build tooling & test updates labels Nov 28, 2025
@nx-cloud
Copy link

nx-cloud bot commented Nov 28, 2025

View your CI Pipeline Execution ↗ for commit c5af483

Command Status Duration Result
nx run-many -t compile --parallel=3 ✅ Succeeded 44s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-28 17:42:36 UTC

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

📝 Walkthrough

Walkthrough

Windows-specific CircleCI steps were adjusted to use Windows paths and a Windows working directory; Chromatic sandbox step ordering was changed to cd into the sandbox before copying artifacts; npm registry commands in the sandbox generator now use the global (-g) flag; an Angular sandbox template dependency changed to @angular/forms@next.

Changes

Cohort / File(s) Summary
CircleCI configs
/.circleci/config.yml, /.circleci/src/jobs/chromatic-sandboxes.yml, /.circleci/src/jobs/test-init-empty-windows.yml
Updated Windows workspace/paths to C:\Users\circleci; set working_directory: 'C:\Users\circleci\storybook' for the Windows job; reordered Chromatic sandbox steps to cd $(yarn get-sandbox-dir ...) before copying /tmp/storybook-sandboxes.
Sandbox script
scripts/sandbox/generate.ts
Switched npm registry config commands to include the -g (global) flag when setting and restoring the registry.
CLI Storybook templates
code/lib/cli-storybook/src/sandbox-templates.ts
Replaced '@angular/forms@^21.0.0' with '@angular/forms@next' in the angular-cli/prerelease template and removed two legacy inline comments.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Verify no remaining steps reference /tmp where Windows executors expect C:\....
  • Confirm working_directory: 'C:\Users\circleci\storybook' is valid and interacts correctly with caching/artifact steps.
  • Review Chromatic sandbox step ordering to ensure the copied artifacts land in the expected target after cd.
  • Check global npm config -g changes for unintended persistence on CI runners.
  • Validate @angular/forms@next compatibility with the prerelease template and downstream installs.

Possibly related PRs


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16c6db0 and c5af483.

📒 Files selected for processing (2)
  • .circleci/config.yml (3 hunks)
  • .circleci/src/jobs/chromatic-sandboxes.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .circleci/config.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). (3)
  • GitHub Check: daily
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
.circleci/src/jobs/chromatic-sandboxes.yml (2)

21-22: This review comment is based on a false premise and should be removed.

The chromatic-sandboxes job uses the sb_node_22_browsers executor, which is a Linux Docker executor (cimg/node:22.15.0-browsers—a CircleCI managed Linux image). Unix paths like /tmp/storybook-sandboxes are correct and expected for this environment. The job does not run on Windows at all.

The Windows-specific paths mentioned (e.g., C:\Users\circleci\storybook) exist in test-init-empty-windows.yml, which is a separate, Windows-only job using the win/default executor. These are entirely different jobs with different executors, so no cross-platform compatibility issue exists in the code under review.


21-22: Now I have all the context I need. Let me trace through the logic:

  1. create-sandboxes.yml persists /tmp/storybook-sandboxes to the workspace (with node_modules removed)
  2. chromatic-sandboxes.yml attaches workspace at /tmp, restoring /tmp/storybook-sandboxes
  3. Line 21: cd $(yarn get-sandbox-dir --template $TEMPLATE) && yarn
    • get-sandbox-dir returns a path like /tmp/storybook-sandboxes/react-18 (based on SANDBOX_DIRECTORY + template)
    • Changes to that directory and runs yarn to install dependencies there
  4. Line 22: cp /tmp/storybook-sandboxes /tmp/storybook/sandbox -r --remove-destination
    • Copies the entire /tmp/storybook-sandboxes (which now includes the newly installed dependencies from line 21) to /tmp/storybook/sandbox

The original review's concern is incorrect. The sequence is actually correct:

  • Line 21 installs dependencies into a specific template's directory within /tmp/storybook-sandboxes/
  • Line 22 then copies the entire /tmp/storybook-sandboxes/ directory (including the newly installed dependencies) to a fresh destination

The --remove-destination flag ensures a clean copy, which is appropriate. The dependencies are preserved, not removed.



Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a381c7d and 2c5725b.

📒 Files selected for processing (1)
  • .circleci/config.yml (1 hunks)
⏰ 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). (3)
  • GitHub Check: nx
  • GitHub Check: merged
  • GitHub Check: Core Unit Tests, windows-latest

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7a6973 and 4895eb4.

📒 Files selected for processing (2)
  • .circleci/config.yml (2 hunks)
  • .circleci/src/jobs/test-init-empty-windows.yml (1 hunks)
⏰ 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). (3)
  • GitHub Check: merged
  • GitHub Check: nx
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
.circleci/src/jobs/test-init-empty-windows.yml (1)

49-51: Validate relative path navigation with new working_directory.

The job steps use cd .. to navigate from the working directory (%HOME%\storybook) to the parent (%HOME%) where the attached workspace resides. Verify that this navigation correctly reaches the code directory (which contains Verdaccio and yarn local-registry) after attaching the workspace at %HOME%.

Also applies to: 81-83, 114-116, 147-149

.circleci/config.yml (1)

768-927: Verify working_directory placement in YAML structure.

The working_directory is placed at the end of the test-init-empty-windows job definition (line 927), after all the steps. While this may be valid YAML, please verify this is the intended CircleCI configuration style. Typically, job metadata like working_directory appears before the steps section for clarity. Compare with the non-Windows test-init-empty job (starting at line 627) which does not have a working_directory override.

Comment on lines 9 to 14
working_directory: \%HOME%\storybook

steps:
- checkout
- attach_workspace:
at: /tmp
at: \%HOME%
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

CircleCI Windows executor working_directory attach_workspace path syntax YAML

💡 Result:

Short answer: working_directory in CircleCI is treated as an absolute path and may be created automatically; attach_workspace.at may be an absolute path or a path relative to working_directory. On Windows executors use Windows-style absolute paths (e.g. C:\path\to\dir) or a path relative to the working_directory. Persist_to_workspace.root follows the same rule (absolute or relative to working_directory) and persist_to_workspace.paths are relative to that root. (circleci.com)

Minimal Windows YAML example:

version: 2.1
jobs:
build:
machine:
image: windows-server-2019-vs2019
working_directory: C:\project
steps:
- run: mkdir C:\project\workspace
- run: echo Hello > C:\project\workspace\out.txt
- persist_to_workspace:
root: C:\project\workspace
paths:
- out.txt

downstream:
machine:
image: windows-server-2019-vs2019
working_directory: C:\project
steps:
- attach_workspace:
at: C:\project\workspace
- run: type C:\project\workspace\out.txt

References:

  • CircleCI configuration reference (executors / working_directory). [1]
  • Using workspaces (attach_workspace example + rules). [2]
  • persist_to_workspace / attach_workspace keys (root/paths/at details). [3]

Citations:


🏁 Script executed:

#!/bin/bash
# Check for other CircleCI Windows job configs in the repo
fd -e yml -e yaml | xargs grep -l "working_directory.*%HOME%" 2>/dev/null || echo "No matches found with %HOME%"

# Also search for Windows path patterns
fd -e yml -e yaml | xargs grep -n "working_directory.*\\\\" 2>/dev/null | head -20 || echo "No backslash patterns found"

# Look at the specific file to understand full context
echo "=== Full test-init-empty-windows.yml file ==="
cat ".circleci/src/jobs/test-init-empty-windows.yml"

Repository: storybookjs/storybook

Length of output: 5555


Correct the Windows path escaping syntax: remove the leading backslash before %HOME%.

Based on CircleCI's official documentation, Windows paths in CircleCI YAML should use standard Windows format without leading backslash escaping. The configuration shows:

working_directory: \%HOME%\storybook
at: \%HOME%

This should be:

working_directory: %HOME%\storybook
at: %HOME%

The leading backslash before %HOME% is incorrect and will prevent proper environment variable expansion. CircleCI's documented example uses C:\project (no leading backslash), and environment variables expand as %VAR% not \%VAR%.

🤖 Prompt for AI Agents
In .circleci/src/jobs/test-init-empty-windows.yml around lines 9 to 14, the
Windows paths incorrectly include a leading backslash before the %HOME%
environment variable; remove the leading backslash so the values become standard
Windows-style with environment variable expansion (e.g. change
"\%HOME%\storybook" to "%HOME%\storybook" and "\%HOME%" to "%HOME%") to allow
CircleCI to expand the %HOME% variable correctly.

@kasperpeulen kasperpeulen added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:merged Run the CI jobs that normally run when merged. labels Nov 28, 2025
@kasperpeulen kasperpeulen added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:daily Run the CI jobs that normally run in the daily job. labels Nov 28, 2025
@kasperpeulen kasperpeulen changed the title Build: Fix merged Build: Fix windows Nov 28, 2025
@kasperpeulen kasperpeulen merged commit 95f4f8b into next Nov 28, 2025
129 of 130 checks passed
@kasperpeulen kasperpeulen deleted the kasper/circleci-merged branch November 28, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants