Skip to content

Revert "fix(scraps): align leadingItems in compactSelect with check box/icon (#106167)"#106889

Merged
TkDodo merged 1 commit intomasterfrom
tkdodo/fix/revert-align_leadingItems_in_compactSelect_with_check_boxicon
Jan 23, 2026
Merged

Revert "fix(scraps): align leadingItems in compactSelect with check box/icon (#106167)"#106889
TkDodo merged 1 commit intomasterfrom
tkdodo/fix/revert-align_leadingItems_in_compactSelect_with_check_boxicon

Conversation

@TkDodo
Copy link
Collaborator

@TkDodo TkDodo commented Jan 23, 2026

This reverts commit 56c668f.

@TkDodo TkDodo marked this pull request as ready for review January 23, 2026 17:54
@TkDodo TkDodo requested review from a team as code owners January 23, 2026 17:54
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 23, 2026
</CheckWrap>
)}
{leading ? <LeadWrap aria-hidden="true">{leading}</LeadWrap> : null}
{leadingItems}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The component will crash if the leadingItems prop is a function because it's rendered directly instead of being called first.
Severity: HIGH

Suggested Fix

In listBox/option.tsx, before rendering leadingItems, check if it is a function. If it is, call it with the appropriate state arguments ({disabled: isDisabled, isFocused, isSelected}) to get the renderable React.ReactNode, similar to the implementation in gridList/option.tsx.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/components/core/compactSelect/listBox/option.tsx#L96

Potential issue: In `listBox/option.tsx`, the `leadingItems` prop is rendered directly.
However, the type definition for this prop allows it to be a function that returns a
`React.ReactNode`. If a function is passed as the `leadingItems` prop, which is a valid
use case as seen in `hybridFilter.tsx`, React will attempt to render the function itself
as a child. This will cause a runtime error stating "Functions are not valid as a React
child", crashing the component. The corresponding implementation in
`gridList/option.tsx` correctly handles this by checking if `leadingItems` is a function
and invoking it if so.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

</CheckWrap>
)}
{leading ? <LeadWrap aria-hidden="true">{leading}</LeadWrap> : null}
{leadingItems}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function leadingItems not called in listBox option

High Severity

The leadingItems prop can be a function (of type ExtraContent), but line 96 renders {leadingItems} directly without checking the type. When leadingItems is a function, it won't be invoked and will render incorrectly or not at all. The sibling component gridList/option.tsx correctly handles this with typeof leadingItems === 'function' ? leadingItems({disabled, isFocused, isSelected}) : leadingItems, and menuListItem.tsx follows the same pattern.

Fix in Cursor Fix in Web

);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [multiple, isSelected, isDisabled, isFocused, size, leadingItems, hideCheck]);
}, [multiple, isSelected, isDisabled, size, leadingItems, hideCheck]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing isFocused dependency causes stale closure

Medium Severity

The useMemo dependency array on line 114 is missing isFocused, but line 109 passes isFocused to the leadingItems function call. When the focus state changes, the memoized value won't recalculate, causing the leadingItems function to receive a stale isFocused value.

Fix in Cursor Fix in Web

@TkDodo TkDodo merged commit 43df0b7 into master Jan 23, 2026
54 checks passed
@TkDodo TkDodo deleted the tkdodo/fix/revert-align_leadingItems_in_compactSelect_with_check_boxicon branch January 23, 2026 18:04
TkDodo added a commit that referenced this pull request Jan 26, 2026
JonasBa pushed a commit that referenced this pull request Jan 27, 2026
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants