Skip to content

test(docs): assert independent radio groups across duplicate Controls blocks#34922

Closed
leno23 wants to merge 1 commit into
storybookjs:nextfrom
leno23:fix/controls-radio-name-regression-34864
Closed

test(docs): assert independent radio groups across duplicate Controls blocks#34922
leno23 wants to merge 1 commit into
storybookjs:nextfrom
leno23:fix/controls-radio-name-regression-34864

Conversation

@leno23
Copy link
Copy Markdown

@leno23 leno23 commented May 26, 2026

Summary

  • Extend MultipleControlsForSameStoryOnSamePage to render duplicate <Controls /> blocks with inline-radio args
  • Assert each radio group's name attribute is unique per Controls instance
  • Assert selecting a radio in one block does not change the selection in the other

The underlying fix landed in #34793 (controlsId via useId()); this adds regression coverage for the radio name collision reported in #34864 / #29295.

Test plan

  • Story play function covers unique IDs, unique radio names, and independent selection behavior
  • CI (awaiting maintainer bug + ci:normal labels for Danger JS)

Closes #34864

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Added a new story example demonstrating inline radio control functionality with multiple selection options.
  • Tests

    • Enhanced test coverage for Controls blocks to verify multiple independent controls work correctly on the same page.
    • Expanded test assertions to validate unique control identifiers and independent state management across controls.

Review Change Stack

… blocks

Extend the duplicate-<Controls /> regression story to use inline-radio args,
verify unique radio group names per block, and assert selections in one block
do not affect the other.

Closes storybookjs#34864

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions
Copy link
Copy Markdown
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","build","dependencies"]

🚫

PR is not labeled with one of: ["ci:normal","ci:merged","ci:daily","ci:docs"]

🚫 PR title must be in the format of "Area: Summary", With both Area and Summary starting with a capital letter Good examples: - "Docs: Describe Canvas Doc Block" - "Svelte: Support Svelte v4" Bad examples: - "add new api docs" - "fix: Svelte 4 support" - "Vue: improve docs"
🚫 PR description is missing the mandatory "#### Manual testing" section. Please add it so that reviewers know how to manually test your changes.

Generated by 🚫 dangerJS against ee5f8c0

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

📝 Walkthrough

Walkthrough

This PR adds a reusable WithInlineRadio story example with inline-radio control configuration, then updates the MultipleControlsForSameStoryOnSamePage test to use it and verify that multiple control blocks render with unique radio input names and maintain independent checked states across user interactions.

Changes

Inline-radio Controls test expansion

Layer / File(s) Summary
Add WithInlineRadio story fixture
code/addons/docs/src/blocks/examples/ControlsParameters.stories.tsx
New WithInlineRadio story export defines default args and an inline-radio control for prop a with options small, medium, and large.
Update MultipleControlsForSameStoryOnSamePage with expanded assertions
code/addons/docs/src/blocks/blocks/Controls.stories.tsx
Test imports updated to userEvent and expect; story revised to render two Controls blocks for WithInlineRadio and expanded play function validates unique radio input names across blocks, confirms two argument tables, clicks specific radios per block, and asserts independently checked values (medium in first block, large in second).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • storybookjs/storybook#34021: Both PRs update Controls.stories.tsx to validate uniqueness of identifiers when multiple <Controls> blocks render on the same page; this PR adds stronger radio name assertions that depend on the underlying fix introduced by the retrieved PR.
  • storybookjs/storybook#34793: The main PR extends the MultipleControlsForSameStoryOnSamePage test with stronger per-block radio assertions, directly building on the collision/id-uniqueness test framework introduced by the retrieved PR.

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@code/addons/docs/src/blocks/blocks/Controls.stories.tsx`:
- Around line 206-211: The test currently asserts that all radio input names are
unique, which is incorrect for radio groups; in Controls.stories.tsx where
radioInputs and radioNames are computed, change the second assertion to verify
that all radios share the same name (i.e., new Set(radioNames).size === 1)
rather than matching the total input count, while keeping the check that
radioNames.length > 0.
🪄 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: 739eacc8-57a1-42df-8f2b-37f870c2b582

📥 Commits

Reviewing files that changed from the base of the PR and between ac3a378 and ee5f8c0.

📒 Files selected for processing (2)
  • code/addons/docs/src/blocks/blocks/Controls.stories.tsx
  • code/addons/docs/src/blocks/examples/ControlsParameters.stories.tsx

Comment on lines +206 to +211
const radioInputs = Array.from(
canvasElement.querySelectorAll<HTMLInputElement>('input[type="radio"]')
);
const radioNames = radioInputs.map((input) => input.name);
await expect(radioNames.length).toBeGreaterThan(0);
await expect(new Set(radioNames).size).toBe(radioNames.length);
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 | 🔴 Critical | ⚡ Quick win

Fix radio name assertion to match radio-group semantics.

This check is currently wrong for radios. Inputs in the same group should share the same name, so Set.size === total inputs will fail for valid markup.

Suggested fix
-    const radioInputs = Array.from(
-      canvasElement.querySelectorAll<HTMLInputElement>('input[type="radio"]')
-    );
-    const radioNames = radioInputs.map((input) => input.name);
-    await expect(radioNames.length).toBeGreaterThan(0);
-    await expect(new Set(radioNames).size).toBe(radioNames.length);
+    const tables = canvasElement.querySelectorAll('.docblock-argstable');
+    await expect(tables.length).toBe(2);
+
+    const firstNames = new Set(
+      Array.from(tables[0].querySelectorAll<HTMLInputElement>('input[type="radio"]')).map(
+        (input) => input.name
+      )
+    );
+    const secondNames = new Set(
+      Array.from(tables[1].querySelectorAll<HTMLInputElement>('input[type="radio"]')).map(
+        (input) => input.name
+      )
+    );
+    await expect(firstNames.size).toBe(1);
+    await expect(secondNames.size).toBe(1);
+    await expect([...firstNames][0]).not.toBe([...secondNames][0]);
 
-    const tables = canvasElement.querySelectorAll('.docblock-argstable');
-    await expect(tables.length).toBe(2);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/addons/docs/src/blocks/blocks/Controls.stories.tsx` around lines 206 -
211, The test currently asserts that all radio input names are unique, which is
incorrect for radio groups; in Controls.stories.tsx where radioInputs and
radioNames are computed, change the second assertion to verify that all radios
share the same name (i.e., new Set(radioNames).size === 1) rather than matching
the total input count, while keeping the check that radioNames.length > 0.

@Sidnioulz
Copy link
Copy Markdown
Contributor

AI spam

@Sidnioulz Sidnioulz closed this May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Potential collisions between radio/checklist group names when a control is rendered multiple times

2 participants