Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions code/lib/cli-storybook/src/bin/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,10 @@ command('upgrade')
.option('-f --force', 'force the upgrade, skipping autoblockers')
.option('-n --dry-run', 'Only check for upgrades, do not install')
.option('-s --skip-check', 'Skip postinstall version and automigration checks')
.option(
'--skip-automigrations',
'Skip running automigrations entirely (only update package versions and install)'
)
.option(
'-c, --config-dir <dir-name...>',
'Directory(ies) where to load Storybook configurations from'
Expand Down
17 changes: 12 additions & 5 deletions code/lib/cli-storybook/src/upgrade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export const checkVersionConsistency = () => {

export type UpgradeOptions = {
skipCheck: boolean;
skipAutomigrations?: boolean;
packageManager?: PackageManagerName;
dryRun: boolean;
yes: boolean;
Expand Down Expand Up @@ -413,11 +414,17 @@ export async function upgrade(options: UpgradeOptions): Promise<void> {
}
}

// Run automigrations for all projects
const { automigrationResults, detectedAutomigrations } = await runAutomigrations(
storybookProjects,
options
);
// Run automigrations for all projects (unless explicitly skipped)
let automigrationResults: Record<string, AutomigrationResult> = {};
let detectedAutomigrations: AutomigrationCheckResult[] = [];
if (options.skipAutomigrations) {
logger.log('Skipping automigrations (--skip-automigrations).');
} else {
({ automigrationResults, detectedAutomigrations } = await runAutomigrations(
storybookProjects,
options
));
}
Comment on lines +417 to +427
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Variable shadowing causes signal handler to always see empty automigration results.

Line 341 declares const automigrationResults in the outer scope, which is captured by the handleInterruption closure (line 348). The new let automigrationResults at line 418 creates a shadowing variable in the inner scope.

If a user interrupts (SIGINT/SIGTERM) after automigrations complete, the signal handler will send telemetry with empty automigrationResults because it references the outer empty object, not the inner populated one.

🐛 Proposed fix: Remove shadowing by reassigning the outer variable
-    // Run automigrations for all projects (unless explicitly skipped)
-    let automigrationResults: Record<string, AutomigrationResult> = {};
-    let detectedAutomigrations: AutomigrationCheckResult[] = [];
-    if (options.skipAutomigrations) {
+    // Run automigrations for all projects (unless explicitly skipped)
+    let detectedAutomigrations: AutomigrationCheckResult[] = [];
+    if (options.skipAutomigrations) {
       logger.log('Skipping automigrations (--skip-automigrations).');
     } else {
-      ({ automigrationResults, detectedAutomigrations } = await runAutomigrations(
+      const result = await runAutomigrations(
         storybookProjects,
         options
-      ));
+      );
+      Object.assign(automigrationResults, result.automigrationResults);
+      detectedAutomigrations = result.detectedAutomigrations;
     }

Also change line 341 from const to let to allow reassignment, or use Object.assign as shown above to mutate the existing object in place.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/lib/cli-storybook/src/upgrade.ts` around lines 417 - 427, The signal
handler closure handleInterruption captures the outer
automigrationResults/detectedAutomigrations but the inner block redeclares them
with let, causing shadowing and empty telemetry; fix by removing the inner
declarations and assigning into the outer variables instead (either change the
outer declaration to let so you can reassign the result of
runAutomigrations(storybookProjects, options) to automigrationResults and
detectedAutomigrations, or keep outer const and mutate it with Object.assign
into automigrationResults and set detectedAutomigrations accordingly), and
ensure the code path that skips automigrations still leaves the outer variables
in the correct state so handleInterruption sees the real results.


// Install dependencies
const rootPackageManager =
Expand Down
27 changes: 27 additions & 0 deletions scripts/eval/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,33 @@ node scripts/eval/sync-baselines.ts --skip-push

The script ensures each repo is on its default branch with no local changes, fetches the latest from origin, replaces the `.storybook` directory with the canonical baseline, and commits/pushes if anything changed.

## Syncing the Storybook version

`sync-storybook-version.ts` bumps every benchmark repo to a specific Storybook version. It mirrors the shape of `sync-baselines.ts`: for each project it ensures the source clone is present and clean, checks out and fast-forwards the default branch, runs `npx storybook@<version> upgrade --yes --force --skip-check --skip-automigrations -c <projectDir>/.storybook` from the **repo root**, then commits and pushes any resulting changes. Running from the repo root (with `-c` pointing at the project's `.storybook` dir) lets the Storybook CLI discover the correct workspace `package.json` in pnpm/yarn monorepos where the Storybook deps live at the workspace root and the config lives in a sub-package.

```sh
# Upgrade every benchmark repo to a stable version
node scripts/eval/sync-storybook-version.ts --version 9.1.0

# Upgrade to a canary published from a Storybook PR
node scripts/eval/sync-storybook-version.ts --version 0.0.0-pr-34297-sha-abcdef12

# Upgrade a subset of projects
node scripts/eval/sync-storybook-version.ts --version latest --project mealdrop --project edgy

# Commit locally without pushing yet
node scripts/eval/sync-storybook-version.ts --version 9.1.0 --skip-push
```

The upgrade passes the following flags:

- `--yes` — auto-accepts prompts.
- `--force` — skips the autoblocker gate (useful for canary or major-version bumps).
- `--skip-check` — skips the postinstall self-check.
- `--skip-automigrations` — prevents the CLI from rewriting source files (e.g. the `wrap-getAbsolutePath` migration).

The commit message defaults to `Eval: upgrade Storybook to <version>`. If you review a `--skip-push` run first, rerun the same command without `--skip-push` to push the existing local upgrade commits. Run `sync-baselines.ts` afterwards if you also need to refresh the canonical `.storybook` config in every repo.

## Collecting results

After running trials, `collect-pr-data.ts` scrapes the published draft PRs and loads the data into a local SQLite database.
Expand Down
2 changes: 2 additions & 0 deletions scripts/eval/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ export const EXAMPLE_PROMPT_BASENAME = 'pattern-copy-play';
export const NODE_EVAL_TRIAL_SCRIPT = 'scripts/eval/eval.ts' as const;
export const NODE_EVAL_RUN_BATCH_SCRIPT = 'scripts/eval/run-batch.ts' as const;
export const NODE_EVAL_SYNC_BASELINES_SCRIPT = 'scripts/eval/sync-baselines.ts' as const;
export const NODE_EVAL_SYNC_STORYBOOK_VERSION_SCRIPT =
'scripts/eval/sync-storybook-version.ts' as const;
export const NODE_EVAL_COLLECT_PR_DATA_SCRIPT = 'scripts/eval/collect-pr-data.ts' as const;
export const STORYBOOK_DIRNAME = '.storybook';
export const EVAL_RESULTS_DIRNAME = 'eval-results';
Expand Down
Loading
Loading