Skip to content

Fix: Deduplicate variable names in builder-vite codegen to prevent hash collisions#34424

Closed
karthik-idikuda wants to merge 2 commits into
storybookjs:nextfrom
karthik-idikuda:fix/builder-vite-hash-collision-dedup
Closed

Fix: Deduplicate variable names in builder-vite codegen to prevent hash collisions#34424
karthik-idikuda wants to merge 2 commits into
storybookjs:nextfrom
karthik-idikuda:fix/builder-vite-hash-collision-dedup

Conversation

@karthik-idikuda
Copy link
Copy Markdown

@karthik-idikuda karthik-idikuda commented Apr 1, 2026

Description

Adds a collision-proof deduplication mechanism to the project annotations codegen in @storybook/builder-vite.

When two different addon preview paths produce the same hash value (e.g., addon-links and addon-a11y both generating hash 22070), the generated virtual module contains duplicate variable names:

import * as preview_22070 from "...addon-links...";
import * as preview_22070 from "...addon-a11y...";  // Duplicate!

This causes Uncaught SyntaxError: Identifier 'preview_22070' has already been declared and prevents Storybook from loading.

Fixes #34372

Changes

codegen-project-annotations.ts:

  • Track generated variable names in a Set
  • When a collision is detected, append _2, _3, etc. to make the name unique
  • The while-loop handles arbitrary N-way collisions

codegen-project-annotations.test.ts (new):

  • Test that two paths producing the same hash get unique variable names
  • Test that non-colliding paths are unaffected
  • Test that three-way collisions are handled correctly

Context

PR #34274 improved the hash function from a simple character-code sum to djb2, which greatly reduces collision probability. However, djb2 can still theoretically collide, and the original simple hash is what's deployed in Storybook 10.3.x. This PR adds a defensive dedup layer that guarantees correctness regardless of the hash function used.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented import alias collisions in generated project annotations so all generated variable names are unique across collision scenarios.
  • Tests

    • Added tests covering multiple collision cases to verify generated import name uniqueness.

When two different addon preview paths produce the same djb2 hash
(e.g., addon-links and addon-a11y on certain systems), the generated
virtual module contains duplicate variable names, causing:

  Uncaught SyntaxError: Identifier 'preview_22070' has already been declared

Track seen variable names with a Set and append a numeric suffix (_2,
_3, ...) when a collision is detected. This guarantees unique
identifiers regardless of the hash function used.

Fixes storybookjs#34372
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 17fcf9be-bb09-4efb-af0c-2403c7772edc

📥 Commits

Reviewing files that changed from the base of the PR and between 4c2083e and 6cc9487.

📒 Files selected for processing (1)
  • code/builders/builder-vite/src/codegen-project-annotations.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/builders/builder-vite/src/codegen-project-annotations.test.ts

📝 Walkthrough

Walkthrough

Added collision detection and resolution logic to the project annotations codegen to ensure unique variable names during import statement generation. When variable name collisions occur, numeric suffixes are appended incrementally. Comprehensive test coverage verifies uniqueness across single collision, no-collision, and multi-collision scenarios.

Changes

Cohort / File(s) Summary
Variable Name Collision Detection
code/builders/builder-vite/src/codegen-project-annotations.ts
Introduced a seen set to track generated variable names and appended incrementing numeric suffixes (starting at _2) when collisions are detected so import aliases remain unique.
Test Suite
code/builders/builder-vite/src/codegen-project-annotations.test.ts
Added Vitest tests that call generateProjectAnnotationsCodeFromPreviews with varying previewAnnotations to assert generated namespace import identifiers are unique in collision, no-collision, and three-way-collision scenarios (regex extraction + Set size checks).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown
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: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@code/builders/builder-vite/src/codegen-project-annotations.test.ts`:
- Around line 19-21: The test currently only asserts uniqueness of the imported
variable names extracted via `importedVars = [...result.matchAll(/import \* as
(\w+) from/g)].map((m) => m[1])` and `expect(new
Set(importedVars).size).toBe(importedVars.length)`, but doesn’t assert the
expected number of imports; add explicit length assertions for each case
(expect(importedVars.length).toBe(2), expect(importedVars.length).toBe(2), and
expect(importedVars.length).toBe(3) respectively) immediately after extracting
`importedVars`, and make the same addition for the other occurrences around the
`result.matchAll` usages at the locations corresponding to lines 35-37 and 55-56
so the tests check both uniqueness and completeness.
- Line 38: The current assertion using importedVars.every((v) =>
!/_\d+$/.test(v) || /^\w+_\d+$/.test(v)) is too permissive; replace it with a
stricter check that each identifier is alphanumeric and may have an optional
numeric dedupe suffix that is a positive integer without leading zeros (for
example: /^\w+(?:_[1-9]\d*)?$/.test(v)). Update the assertion on importedVars to
use that regex so the test fails for malformed or unintended dedupe suffixes;
locate the check using the importedVars symbol in
codegen-project-annotations.test.ts and swap the predicate accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c38c9060-d8a1-48ab-a15b-b63988245e8a

📥 Commits

Reviewing files that changed from the base of the PR and between 199f656 and 4c2083e.

📒 Files selected for processing (2)
  • code/builders/builder-vite/src/codegen-project-annotations.test.ts
  • code/builders/builder-vite/src/codegen-project-annotations.ts

Comment thread code/builders/builder-vite/src/codegen-project-annotations.test.ts
Comment thread code/builders/builder-vite/src/codegen-project-annotations.test.ts Outdated
@valentinpalkovic
Copy link
Copy Markdown
Contributor

Should already be resolved by #34274

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hash collision causes duplicate identifier error, Uncaught SyntaxError: Identifier 'preview_22070' has already been declared

2 participants