Docs: Scope control input ids to each <Controls /> block instance#34793
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a per-Controls-instance ChangesControl ID Disambiguation
Sequence Diagram: sequenceDiagram
participant Controls
participant PureArgsTable
participant ArgRow
participant ArgControl
participant ControlComponent
Controls->>Controls: useId() -> controlsId
Controls->>PureArgsTable: pass controlsId
PureArgsTable->>ArgRow: include controlsId in common props
ArgRow->>ArgControl: forward controlsId
ArgControl->>ControlComponent: forward controlsId
ControlComponent->>ControlComponent: getControlId(name, storyId, controlsId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/blocks/Controls.stories.tsx (1)
197-204: ⚡ Quick winConsider adding explicit verification of radio button name independence.
The test correctly verifies that all control element IDs are unique. However, the original issue
#29295specifically mentions radio inputs being grouped by sharednameattributes, causing cross-block interference. While your implementation correctly scopes radio names viacontrolId(which includescontrolsId), the test would be more comprehensive if it explicitly verified that radio buttonnameattributes are unique across blocks.🧪 Optional: Add explicit radio name verification
play: async ({ canvasElement }) => { const allIds = Array.from(canvasElement.querySelectorAll('[id^="control-"]')).map( (el) => el.id ); const uniqueIds = new Set(allIds); await expect(allIds.length).toBeGreaterThan(0); await expect(uniqueIds.size).toBe(allIds.length); + + // Explicitly verify radio button name independence across blocks + const radioNames = Array.from(canvasElement.querySelectorAll('input[type="radio"]')).map( + (el) => (el as HTMLInputElement).name + ); + if (radioNames.length > 0) { + const uniqueRadioNames = new Set(radioNames); + await expect(uniqueRadioNames.size).toBe(radioNames.length); + } },🤖 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 197 - 204, Extend the current play test in Controls.stories.tsx (the play: async ({ canvasElement }) => { ... }) to also collect radio inputs and assert their name attributes are unique across blocks: query the canvasElement for radio inputs (e.g., canvasElement.querySelectorAll('input[type="radio"]') or scoped to '[id^="control-"] input[type="radio"]'), build a Set of their name values, and add an expectation that the Set size equals the total radio elements count so radio name attributes are not shared across blocks (keep existing ID uniqueness checks intact).
🤖 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.
Nitpick comments:
In `@code/addons/docs/src/blocks/blocks/Controls.stories.tsx`:
- Around line 197-204: Extend the current play test in Controls.stories.tsx (the
play: async ({ canvasElement }) => { ... }) to also collect radio inputs and
assert their name attributes are unique across blocks: query the canvasElement
for radio inputs (e.g., canvasElement.querySelectorAll('input[type="radio"]') or
scoped to '[id^="control-"] input[type="radio"]'), build a Set of their name
values, and add an expectation that the Set size equals the total radio elements
count so radio name attributes are not shared across blocks (keep existing ID
uniqueness checks intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f51c3ba2-c267-4d75-b004-7ada095dd5f1
📒 Files selected for processing (19)
code/addons/docs/src/blocks/blocks/Controls.stories.tsxcode/addons/docs/src/blocks/blocks/Controls.tsxcode/addons/docs/src/blocks/components/ArgsTable/ArgControl.tsxcode/addons/docs/src/blocks/components/ArgsTable/ArgRow.tsxcode/addons/docs/src/blocks/components/ArgsTable/ArgsTable.tsxcode/addons/docs/src/blocks/controls/Boolean.tsxcode/addons/docs/src/blocks/controls/Color.tsxcode/addons/docs/src/blocks/controls/Date.tsxcode/addons/docs/src/blocks/controls/Files.tsxcode/addons/docs/src/blocks/controls/Number.tsxcode/addons/docs/src/blocks/controls/Object.tsxcode/addons/docs/src/blocks/controls/Range.tsxcode/addons/docs/src/blocks/controls/Text.tsxcode/addons/docs/src/blocks/controls/helpers.test.tscode/addons/docs/src/blocks/controls/helpers.tscode/addons/docs/src/blocks/controls/options/Checkbox.tsxcode/addons/docs/src/blocks/controls/options/Radio.tsxcode/addons/docs/src/blocks/controls/options/Select.tsxcode/addons/docs/src/blocks/controls/types.ts
|
@coderabbitai re: the nitpick on explicit radio name verification — the current ID-uniqueness check transitively covers this. In |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Code LGTM, thanks @TheSeydiCharyyev!
There's a small issue with useId that is incompatible with older React versions, see comment for a proposed fix.
Sidnioulz
force-pushed
the
fix/issue-29295-non-unique-control-ids
branch
from
87534e6 to
deeb872
Compare
github-project-automation
Bot
moved this from Empathy Queue (prioritized)
to Done
in Core Team Projects
3 participants
What I did
Multiple
<Controls />blocks rendered for the same story on a single docs page produce control inputs with duplicateidandnameattributes. This breaks<label htmlFor>association and causes radio button groups across the blocks to merge — the browser groups<input type="radio">by sharedname, so editing the selection in one block silently changes the other.#34021 added a
storyIdparameter togetControlIdto disambiguate<Controls />blocks for different stories on the same page. It does not cover the case in this issue because both blocks pass the samestoryId.This PR threads a per-
<Controls />-instancecontrolsIdthrough the same chain that already carriesstoryId:Controls.tsx→ArgsTable→ArgRow→ArgControl→ individual controls (Boolean,Radio,Checkbox,Select,Range,Text,Object,Files,Date,Color,Number).controlsIdis produced byReact.useId()(with colons stripped so the value is safe inside CSS selectors) and added as an optional third argument togetControlIdandgetControlSetterButtonId:Both extra arguments remain optional, so direct consumers of
<ArgsTable>outside the<Controls />block are unaffected.Fixes #29295
How to test
cd code && yarn storybook:uiBlocks → Controls → Multiple Controls For Same Story On Same Pageplayfunction asserts that all control inputs across both blocks have unique IDs.Manual radio-grouping check (any story that exposes a radio control):
<Controls />blocks on one pageManual testing
yarn fmt:writeyarn nx run-many -t check(clean for changed projects)yarn nx compile addon-docs --skip-nx-cacheMultipleControlsForSameStoryOnSamePagestory passes itsplayassertions in the internal Storybook UISummary by CodeRabbit
Bug Fixes
Tests
Documentation