[tabs] Fire onValueChange for automatic tab selection#3758
[tabs] Fire onValueChange for automatic tab selection#3758michaldudak wants to merge 35 commits intomui:masterfrom
onValueChange for automatic tab selection#3758Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Greptile SummaryThis PR fixes issue #2097 by ensuring Key changes:
Issue found:
The test coverage is thorough and validates the main scenarios. The implementation properly uses Confidence Score: 4/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let reason: TabsRoot.ChangeEventReason; | ||
| if (isInitialRun) { | ||
| reason = REASONS.initial; | ||
| } else if (selectionIsDisabled) { | ||
| reason = REASONS.disabled; | ||
| } else { | ||
| reason = REASONS.missing; | ||
| } |
There was a problem hiding this comment.
logic: The reason assignment logic always uses 'initial' for the first run, even when an explicit defaultValue is provided but the corresponding tab doesn't exist. According to the PR description, 'initial' should only be used when "no value/defaultValue provided", but this code will fire with 'initial' reason even if defaultValue={1} is provided but tab 1 is missing.
Consider refining the logic:
| let reason: TabsRoot.ChangeEventReason; | |
| if (isInitialRun) { | |
| reason = REASONS.initial; | |
| } else if (selectionIsDisabled) { | |
| reason = REASONS.disabled; | |
| } else { | |
| reason = REASONS.missing; | |
| } | |
| // Determine the appropriate reason based on context: | |
| // - 'initial': First automatic selection (no value/defaultValue provided) | |
| // - 'disabled': Tab became disabled after initial render | |
| // - 'missing': Tab was removed from DOM or never existed | |
| let reason: TabsRoot.ChangeEventReason; | |
| if (isInitialRun && isAutomaticDefault) { | |
| reason = REASONS.initial; | |
| } else if (selectionIsDisabled) { | |
| reason = REASONS.disabled; | |
| } else { | |
| reason = REASONS.missing; | |
| } |
This ensures 'initial' is only used when no explicit value was provided, making the behavior match the documentation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/tabs/root/TabsRoot.tsx
Line: 261:268
Comment:
**logic:** The reason assignment logic always uses `'initial'` for the first run, even when an explicit `defaultValue` is provided but the corresponding tab doesn't exist. According to the PR description, `'initial'` should only be used when "no value/defaultValue provided", but this code will fire with `'initial'` reason even if `defaultValue={1}` is provided but tab 1 is missing.
Consider refining the logic:
```suggestion
// Determine the appropriate reason based on context:
// - 'initial': First automatic selection (no value/defaultValue provided)
// - 'disabled': Tab became disabled after initial render
// - 'missing': Tab was removed from DOM or never existed
let reason: TabsRoot.ChangeEventReason;
if (isInitialRun && isAutomaticDefault) {
reason = REASONS.initial;
} else if (selectionIsDisabled) {
reason = REASONS.disabled;
} else {
reason = REASONS.missing;
}
```
This ensures `'initial'` is only used when no explicit value was provided, making the behavior match the documentation.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| const selectionIsDisabled = selectedTabMetadata?.disabled; | ||
| const selectionIsMissing = selectedTabMetadata == null && value !== null; | ||
| const hasNoSelection = value == null; // Catches both null and undefined |
There was a problem hiding this comment.
It seems uncontrolled defaultValue={null} can no longer mean "no active tab" as hasNoSelection treats null as a signal to auto-select, so the effect will always pick the first enabled tab (and fire onValueChange) even when null is explicitly provided
There was a problem hiding this comment.
I restored the null value behavior: https://codesandbox.io/p/sandbox/hero---base-ui-example-forked-j4cmf6
Codex ReviewOverviewThis patch makes uncontrolled Findings1. 🔴 [Blocking] Selected panel is hidden until the client builds the tab mapImpact: Server-rendered tabs no longer include the active panel in the initial HTML, so a normal Evidence: Recommendation: Decouple panel openness from the runtime tab-id lookup. The selected panel should stay open whenever Confidence: 4/5High confidence based on a full-pass review of the current Notes
|
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
Codex ReviewOverviewThis patch makes uncontrolled Findings (None)No blocking issues found in this patch. Confidence: 4/5High confidence based on a full-pass review of the current Notes
|
Codex ReviewOverviewThis patch makes uncontrolled Findings1. 🔴 [Blocking] Selected panel is hidden until the client builds the tab mapImpact: Server-rendered tabs no longer include the active panel in the initial HTML, so a normal Evidence: Recommendation: Decouple panel openness from the runtime tab-id lookup. The selected panel should stay open whenever Confidence: 4/5High confidence based on a full-pass review of the current Notes
|
michaldudak
changed the title
onValueChange for automatic tab selection
github-actions
bot
added
the
PR: out-of-date
3 participants
Fixes a bug where the
onValueChangecallback was not invoked when the selected tab changed automatically due to external factors.Previously, onValueChange only fired for user-initiated tab changes (clicks, keyboard navigation). It did not fire when:
Now,
onValueChangefires in all these scenarios with appropriate reason values.It also fires during initial render when no
valueordefaultValueis provided, and the component automatically selects the first enabled tab.New reason values in onValueChange:
'initial': First automatic selection on mount (no value/defaultValue provided)'disabled': Selected tab became disabled after initial render'missing': Selected tab was removed from the DOMThe existing
'none'reason is used for user-initiated changes.--
New experiment: https://deploy-preview-3758--base-ui.netlify.app/experiments/tabs/tabs-onValueChange
Fixes #2097