Skip to content

fix: dedupe builder-vite project annotation imports#34374

Closed
raashish1601 wants to merge 3 commits into
storybookjs:nextfrom
raashish1601:contributor-15/storybook-34372
Closed

fix: dedupe builder-vite project annotation imports#34374
raashish1601 wants to merge 3 commits into
storybookjs:nextfrom
raashish1601:contributor-15/storybook-34372

Conversation

@raashish1601
Copy link
Copy Markdown

@raashish1601 raashish1601 commented Mar 27, 2026

Summary

  • dedupe builder-vite project annotation import aliases when hash-based names collide
  • keep the existing alias format for the common case and suffix only repeated names
  • add a focused regression covering colliding preview annotation paths

Testing

  • corepack yarn vitest run --config code/builders/builder-vite/vitest.config.ts code/builders/builder-vite/src/codegen-project-annotations.test.ts
  • corepack yarn --cwd code lint:js:cmd builders/builder-vite/src/codegen-project-annotations.ts builders/builder-vite/src/codegen-project-annotations.test.ts
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes

    • Resolved collisions when generating import aliases for preview annotations so each import identifier is unique and deterministic (uses predictable suffixing when needed).
  • Tests

    • Added tests that verify unique identifier generation, deterministic suffix sequencing for repeated collisions, and correct module reference assignment.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 27, 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: da0feb7b-87cb-4177-afee-c7abfc45ba54

📥 Commits

Reviewing files that changed from the base of the PR and between 7849868 and 6d0acc7.

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

📝 Walkthrough

Walkthrough

GenerateProjectAnnotations now ensures import alias uniqueness by tracking used variable names and appending incrementing suffixes (_2, _3, ...) for collisions. A Vitest suite was added to verify deterministic alias generation and correct HMR module references.

Changes

Cohort / File(s) Summary
Preview Annotation Generator
code/builders/builder-vite/src/codegen-project-annotations.ts
Added a usedVariables set and getUniqueImportVariable(baseVariable, usedVariables) to pick unique import aliases by suffixing _2, _3, ...; changed import generation to use the unique alias and adjusted hash comment formatting.
Collision Handling Tests
code/builders/builder-vite/src/codegen-project-annotations.test.ts
New Vitest tests that extract generated namespace import identifiers and assert uniqueness and deterministic suffixing across colliding preview paths, and verify hmrPreviewAnnotationModules[index] ?? <importVar> references the correct identifiers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 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.

🧹 Nitpick comments (1)
code/builders/builder-vite/src/codegen-project-annotations.test.ts (1)

42-46: The self-referential assertion pattern is valid but consider adding a sanity check.

The assertion constructs the expected array using importVariables[0], which correctly tests the relative suffix pattern (_2, _3) without hardcoding the base name. However, if the base name were somehow empty or malformed, the test could still pass spuriously.

Consider adding a minimal sanity assertion on the base identifier format:

💡 Optional: Add sanity check for base identifier
     const importVariables = [...result.matchAll(/import \* as (\w+) from/g)].map(
       (match) => match[1]
     );
 
+    // Sanity check: base identifier should match expected format (preview_<hash>)
+    expect(importVariables[0]).toMatch(/^preview_\d+$/);
     expect(importVariables).toEqual([
       importVariables[0],
       `${importVariables[0]}_2`,
       `${importVariables[0]}_3`,
     ]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/builders/builder-vite/src/codegen-project-annotations.test.ts` around
lines 42 - 46, The test currently builds expected values from
importVariables[0], which can mask a malformed base; add a quick sanity
assertion before the existing expect to ensure importVariables[0] is a valid,
non-empty JS identifier (for example assert it matches a regex like
/^[A-Za-z_$][A-Za-z0-9_$]*$/ or at least is non-empty) so the subsequent
relative-suffix assertions (the expect that compares to
`${importVariables[0]}_2`, `${importVariables[0]}_3`) cannot pass spuriously if
the base name is empty or invalid; update the test in
codegen-project-annotations.test.ts to include this check prior to the existing
equality assertion involving importVariables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@code/builders/builder-vite/src/codegen-project-annotations.test.ts`:
- Around line 42-46: The test currently builds expected values from
importVariables[0], which can mask a malformed base; add a quick sanity
assertion before the existing expect to ensure importVariables[0] is a valid,
non-empty JS identifier (for example assert it matches a regex like
/^[A-Za-z_$][A-Za-z0-9_$]*$/ or at least is non-empty) so the subsequent
relative-suffix assertions (the expect that compares to
`${importVariables[0]}_2`, `${importVariables[0]}_3`) cannot pass spuriously if
the base name is empty or invalid; update the test in
codegen-project-annotations.test.ts to include this check prior to the existing
equality assertion involving importVariables.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 21c76c57-cf4c-498b-abb5-9338e47e4678

📥 Commits

Reviewing files that changed from the base of the PR and between 65a3215 and 7849868.

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

@valentinpalkovic
Copy link
Copy Markdown
Contributor

Hi @raashish1601 ,

Due to a recent high volume of unreviewed AI-generated PRs, we are requesting verification and proof that the implemented fix actually works. Please provide a simple GIF/Video or image of how the bugfix works, optimally with before-and-after comparisons.

Thank you for your understanding!

@valentinpalkovic
Copy link
Copy Markdown
Contributor

Hi @raashish1601,

We requested verification that this PR works as intended, but haven't received a response in over two weeks. We're closing this PR for now.

If you'd like to continue working on this, feel free to reopen the PR with the requested verification (a GIF/video or screenshot showing before-and-after behavior).

Thank you for your contribution!

@github-project-automation github-project-automation Bot moved this from On Hold to Done in Core Team Projects Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants