Fix SelectItem infinite loop when id prop is undefined #4985
Conversation
🦋 Changeset detectedLatest commit: 4a9bd0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughPatch fixes SelectItem id handling so an explicit Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant S as Select
participant SI as SelectItem
participant ID as useId()
participant DOM as Listbox/DOM
Note over SI,ID: New flow — ignore incoming id if explicitly undefined
U->>S: Open select
S->>SI: Render items (props may include id=undefined)
SI->>ID: id = useId(providedId)
ID-->>SI: stable id (generated or provided)
SI->>DOM: Render item with stable id (forwarded props exclude original id)
U->>SI: Click item
SI-->>S: Notify selection
S->>DOM: Update combobox and close listbox
sequenceDiagram
autonumber
participant S as Select
participant SI as SelectItem (Old)
participant ID as useId()
participant DOM as Listbox/DOM
Note over SI,DOM: Old flow — incoming undefined id could override generated id
S->>SI: Render items (props include id=undefined)
SI->>ID: id = useId(undefined)
ID-->>SI: generated id
SI->>DOM: Merge props (props.id === undefined overrides generated id)
Note right of DOM: Mismatched id -> blur/focus churn
DOM-->>SI: Repeated focus/blur -> re-renders (loop)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an infinite loop bug in the SelectItem component that occurred when the id prop was explicitly passed as undefined. The issue was caused by calling useId(props.id) after the id had already been destructured and removed from props.
- Fixed the infinite loop by destructuring the
idprop separately and using the original value withuseId - Added a regression test to ensure the bug doesn't reoccur
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/ariakit-react-core/src/select/select-item.tsx | Refactored prop handling to avoid passing undefined to useId hook |
| examples/select/test.ts | Added regression test to verify SelectItems render with valid IDs |
| .changeset/4983-fix-select-item-id-undefined.md | Added changeset documenting the bug fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| props = { ...restPropsWithoutId }; | ||
|
|
||
| const id = useId(providedId); | ||
| const disabled = disabledFromProps(props); |
There was a problem hiding this comment.
Mutating the props parameter is an anti-pattern that can make code harder to understand and debug. Consider using restPropsWithoutId directly in subsequent code instead of reassigning props.
| props = { ...restPropsWithoutId }; | |
| const id = useId(providedId); | |
| const disabled = disabledFromProps(props); | |
| const selectItemProps = { ...restPropsWithoutId }; | |
| const id = useId(providedId); | |
| const disabled = disabledFromProps(selectItemProps); |
nvignola
force-pushed
the
fix/select-item-id-undefined
branch
2 times, most recently
from
be4b8b4 to
5de4e7e
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/ariakit-react-core/src/select/select-item.tsx (1)
80-82: Avoid reassigning thepropsparameter.Reassigning function parameters hurts readability and tooling. Prefer introducing a new local (e.g.,
let nextProps = { ...restPropsWithoutId };) and use that onward. This mirrors prior feedback.
🧹 Nitpick comments (3)
packages/ariakit-react-core/src/select/select-item.tsx (2)
77-84: Make the computed id win even if a later step re‑addsid.To be belt-and-suspenders, set
idafter the spread where the element props are composed so it can’t be overridden if any wrapper addsidback.- props = { - id, - role: getPopupItemRole(listElement), - "aria-selected": selected, - children: value, - ...props, - autoFocus: props.autoFocus ?? autoFocus, - onClick, - }; + props = { + role: getPopupItemRole(listElement), + "aria-selected": selected, + children: value, + ...props, + id, // ensure our computed id always wins + autoFocus: props.autoFocus ?? autoFocus, + onClick, + };Please confirm no other hooks in this pipeline inject an
idbefore this point. If they do, the diff above will make the behavior robust.
77-80: Comment slightly misattributes the root cause.The loop was caused by the merged props’
id(undefined) overriding the generated one, not by passingundefinedtouseIditself. Consider rewording to avoid confusion for future readers.examples/select/test.ts (1)
215-217: Tighten assertions (and drop?.).
q.option()shouldn’t be nullable; optional chaining is unnecessary. You can also rely on jest-dom helpers you already use above.- expect(q.option("Apple")?.getAttribute("id")).toBeTruthy(); - expect(q.option("Apple")?.getAttribute("id")).not.toBe("undefined"); + expect(q.option("Apple")).toHaveAttribute("id"); + expect(q.option("Apple").getAttribute("id")).not.toBe("undefined");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/4985-fix-select-item-id-undefined.md(1 hunks)examples/select/test.ts(1 hunks)packages/ariakit-react-core/src/select/select-item.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/select/test.ts (3)
examples/test-utils.ts (1)
test(26-42)packages/ariakit-test/src/click.ts (1)
click(128-169)packages/ariakit-test/src/query.ts (1)
q(165-165)
packages/ariakit-react-core/src/select/select-item.tsx (1)
packages/ariakit-react-core/src/utils/hooks.ts (1)
useId(165-178)
🔇 Additional comments (3)
.changeset/4985-fix-select-item-id-undefined.md (1)
1-5: Changelog entry is correct and scoped.Patch note clearly describes the fix for
@ariakit/react-core. LGTM.packages/ariakit-react-core/src/select/select-item.tsx (1)
77-84: Correctly sources a stable id; fix looks good.Using
const { id: providedId, ...restPropsWithoutId } = props;anduseId(providedId)prevents anundefinedprop from overriding the generated id. Nice.examples/select/test.ts (1)
203-221: Doesn't exercise id={undefined} — add an explicit example or parameterize the test.Repo search finds no <SelectItem ... id=> usages; examples/select/test.ts only asserts generated IDs and never renders . Add a tiny example (or update this test) that renders SelectItem with id={undefined} to guard issue #4593.
nvignola
force-pushed
the
fix/select-item-id-undefined
branch
from
5de4e7e to
53fc6c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ariakit-react-core/src/select/select-item.tsx (2)
159-167: Make generated id authoritative in the final merge.Place
idafter...propsso no intermediate transforms can override it in the rare event an upstream wrapper injects anid.- props = { - id, - role: getPopupItemRole(listElement), - "aria-selected": selected, - children: value, - ...props, - autoFocus: props.autoFocus ?? autoFocus, - onClick, - }; + props = { + role: getPopupItemRole(listElement), + "aria-selected": selected, + children: value, + ...props, + autoFocus: props.autoFocus ?? autoFocus, + onClick, + id, + };
77-84: Drop redundant spread when reassigning props.Replace props = { ...restPropsWithoutId } with props = restPropsWithoutId — same semantics, avoids an unnecessary shallow copy.
File: packages/ariakit-react-core/src/select/select-item.tsx (around lines 77–84)
Apply:
- const { id: providedId, ...restPropsWithoutId } = props; - props = { ...restPropsWithoutId }; + const { id: providedId, ...restPropsWithoutId } = props; + props = restPropsWithoutId;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/4985-fix-select-item-id-undefined.md(1 hunks)examples/select/test.ts(1 hunks)packages/ariakit-react-core/src/select/select-item.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/select/test.ts
- .changeset/4985-fix-select-item-id-undefined.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ariakit-react-core/src/select/select-item.tsx (1)
packages/ariakit-react-core/src/utils/hooks.ts (1)
useId(165-178)
nvignola
force-pushed
the
fix/select-item-id-undefined
branch
from
53fc6c2 to
4a9bd0b
Compare
1 participant
Summary
Fixes an infinite loop bug in
SelectItemwhen theidprop is explicitly passed asundefined.The issue occurred because
useId(props.id)was called afteridhad been destructured and removed fromprops, causingprops.idto beundefined.Changes
useId(props.id)touseId(providedId)to use the originally destructured valueTest
Closes #4593
Summary by CodeRabbit
Bug Fixes
Tests
Chores