fix(ebay-tabs): prevent onSelect from firing on initial render#579
fix(ebay-tabs): prevent onSelect from firing on initial render#579
Conversation
🦋 Changeset detectedLatest commit: 516c8ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
HenriqueLimas
left a comment
HenriqueLimas
left a comment
There was a problem hiding this comment.
Thank you for the PR! I think the best approach would be to get rid of useEffect instead. We also need a unit test for use case
| useEffect(() => { | ||
| handleSelect(index); | ||
| if (prevIndex.current !== index) { | ||
| prevIndex.current = index; | ||
| handleSelect(index); | ||
| } | ||
| }, [index]); |
There was a problem hiding this comment.
I think we should remove the useEffect instead. We already call handleSelect on click
There was a problem hiding this comment.
Pull request overview
This PR fixes @ebay/ui-core-react’s EbayTabs so onSelect no longer fires on the initial mount, addressing issue #407 where mount-time callbacks broke analytics/tracking expectations.
Changes:
- Track the previous
selectedIndexviauseRefand only invoke selection handling when the prop value changes. - Add a changeset to release the fix as a patch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ebayui-core-react/src/ebay-tabs/tabs.tsx | Adds prevIndex tracking to avoid calling selection logic on initial render. |
| .changeset/cyan-toes-happen.md | Declares a patch release note for the tabs onSelect behavior change. |
| handleSelect(index); | ||
| if (prevIndex.current !== index) { | ||
| prevIndex.current = index; | ||
| handleSelect(index); |
There was a problem hiding this comment.
The effect still calls handleSelect(index), which triggers onSelect. This means onSelect will fire for any prop-driven selectedIndex change after mount (not just user interaction), and it will also double-fire in a controlled pattern where the parent updates selectedIndex in response to onSelect (click triggers onSelect, then the prop change triggers onSelect again). Consider syncing internal state from the selectedIndex prop in the effect without calling onSelect (e.g., update selectedIndex state directly), so onSelect is reserved for user-initiated selection changes.
| handleSelect(index); | |
| setSelectedIndex(index); | |
| setFocusedIndex(index); |
| const prevIndex = useRef(index); | ||
| useEffect(() => { | ||
| handleSelect(index); | ||
| if (prevIndex.current !== index) { | ||
| prevIndex.current = index; | ||
| handleSelect(index); | ||
| } | ||
| }, [index]); |
There was a problem hiding this comment.
Please add/adjust unit coverage for this behavior (Vitest tests exist for EbayTabs): at minimum, assert onSelect is not called on initial mount, and (if selectedIndex is used in a controlled pattern) that a single user click results in a single onSelect call (no extra call from prop-sync). This will prevent regressions of the mount/double-fire issues.
Description
onSelectfired on mount becauseuseEffect(() => { handleSelect(index) }, [index])runs on the initial render. Broke any analytics/tracking code that expectedonSelectto mean "user clicked a tab."Switched to tracking the previous index with a
useRef.onSelectonly fires whenindexactually changes. This is cleaner than theisMountedpattern -isMountedjust suppresses the first call, whileprevIndexexpresses what we actually want: skip if nothing changed.Notes
isMountedwould've also worked, but it gets the edge case wrong: if the component mounts with a non-zero index and then receives the same value again,isMountedwould fireonSelectanyway.prevIndexdoesn't.Screenshots
http://localhost:9001/?path=/story/navigation-disclosure-ebay-tabs--default-tabs
As-Is
To-Be
Checklist