Docs: Ensure unique control id attributes across multiple Controls blocks#34021
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)
📝 WalkthroughWalkthroughThreads a new optional Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Page
end
participant Controls
participant ArgsTable
participant ArgRow
participant ArgControl
participant ControlComponent
participant Helpers
Page->>Controls: render (story.id)
Controls->>ArgsTable: render with storyId
ArgsTable->>ArgRow: render row (includes storyId)
ArgRow->>ArgControl: render control (includes storyId)
ArgControl->>ControlComponent: instantiate control props (includes storyId)
ControlComponent->>Helpers: getControlId(name, storyId)
Helpers-->>ControlComponent: return scoped id
ControlComponent-->>Page: render element with scoped id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 1
🤖 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/addons/docs/src/blocks/controls/types.ts`:
- Line 5: ControlProps currently only uses storyId for scoping which allows
collisions for multiple Controls blocks bound to the same story; add an optional
per-block token (e.g., controlsId?: string) to the ControlProps interface in
types.ts and update any ID generation helpers (the functions that build control
element IDs and the code referencing storyId) to include this controlsId when
present so each <Controls of={...}> block can produce unique IDs; ensure
defaults preserve existing behavior when controlsId is undefined and propagate
the new property through components/functions that call the ID helpers (search
for ControlProps, storyId, and the control ID helper functions to update).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4926d203-a8a7-450b-a9ea-77e7e33cebb8
📒 Files selected for processing (18)
code/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
|
|
||
| export interface ControlProps<T> { | ||
| name: string; | ||
| storyId?: string; |
There was a problem hiding this comment.
storyId alone can still collide for repeated <Controls of={sameStory}> blocks.
On Line 5, using only storyId as scope means two Controls blocks bound to the same story will still generate identical control IDs. Consider extending ControlProps with an optional per-block scope token (e.g., controlsId) and threading that into ID helpers for full uniqueness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/addons/docs/src/blocks/controls/types.ts` at line 5, ControlProps
currently only uses storyId for scoping which allows collisions for multiple
Controls blocks bound to the same story; add an optional per-block token (e.g.,
controlsId?: string) to the ControlProps interface in types.ts and update any ID
generation helpers (the functions that build control element IDs and the code
referencing storyId) to include this controlsId when present so each <Controls
of={...}> block can produce unique IDs; ensure defaults preserve existing
behavior when controlsId is undefined and propagate the new property through
components/functions that call the ID helpers (search for ControlProps, storyId,
and the control ID helper functions to update).
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the PR @TheSeydiCharyyev!
Before going into the code review, could you please
- Edit the PR description to match our PR template, with the "Manual testing" section written in?
- Add a story that specifically tests the bug you're addressing with multiple ArgsTables for different stories in the same docs page?
Thanks,
|
@Sidnioulz Thanks for the review! I've updated the PR description to match the template with the Manual testing section. Regarding the story — I already have MultipleControlsOnSamePage in Controls.stories.tsx that renders two blocks for different stories and asserts all control id attributes are unique. Let me know if you'd like a different kind of test! |
|
@Sidnioulz Hey! Just following up — I've updated the PR description with the Manual testing section and added the MultipleControlsOnSamePage story as requested. Let me know if anything else is needed! |
Thanks! I have a bit of backlog but I'll look at it in the coming days :) |
Sidnioulz
changed the title
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks @TheSeydiCharyyev! The angular CI failure is likely due to an external factor. I'll check it out and will let you know if you have anything left to address!
3 participants
Closes #26144
What I did
When multiple
<Controls>blocks render on the same docs page (for different stories), all controls generate identicalidattributes (e.g.,control-disabled). This causes<label htmlFor>to always target the control in the first table instead of the correct one.Root cause:
getControlId(name)inhelpers.tsonly uses the arg name, without any story context.Fix: Added an optional
storyIdparameter togetControlId()andgetControlSetterButtonId(), and threaded it through the component chain:Controls → ArgsTable → ArgRow → ArgControl → all control components. WhenstoryIdis provided, IDs become unique per story (e.g.,control-story--name-disabled). Existing usage withoutstoryIdis unaffected.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-ts<Controls>blocks for different stories (e.g., create an MDX page with two<Controls of={Story1} />and<Controls of={Story2} />)idshould now include the story ID prefix, making them unique across tablesThe
MultipleControlsOnSamePagestory inControls.stories.tsxalso verifies this automatically — it renders two Controls blocks and asserts allidattributes are unique.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can befound in
code/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.