[tabs] Fix activationDirection not updating on programmatic value changes#4347
[tabs] Fix activationDirection not updating on programmatic value changes#4347LukasTy wants to merge 14 commits intomui:masterfrom
activationDirection not updating on programmatic value changes#4347Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Copilot (Opus) self-review: Review SummaryThe PR correctly solves the issue. No blockers found. What the PR doesMoves activation direction logic from
The old Correctness analysis
One thing worth noting
Pragmatic and justifiable tradeoff. TestsNew programmatic-change tests cover both horizontal and vertical orientations. Existing click-based tests are strengthened with |
Codex ReviewOverviewThis patch moves activation-direction detection from Findings1. 🟡 [Non-blocking] Adding and selecting a new tab in one controlled update still yields
|
|
Final pass cleanup result:
|
|
Minor issue in new review, above:
|
Fixed it. |
activationDirection not updating on programmatic value changesactivationDirection not updating on programmatic value changes
|
New:
|
Is it new? |
|
Maybe they have the same cause, but given Chrome Tabs have this UX (pressing to add a new tab selects it immediately) it seems like it would be common to hit (as it mentions) |
Good point and very precise example. 👍 The solution provides an eventual correctness (the last render will have the correct Now I understand the full picture. The issue is that when a new tab is added and selected in the same update, The root cause: The cleanest fix: add a value-based fallback in The fix has two parts working together: 1. Value-based fallback in 2. Deferred ref sync via |
Claude Opus 4.6 follow-up reviewMerge Confidence: 88/100SummaryAll 183 tabs tests pass (both Chromium and JSDOM). The PR cleanly fixes #3873 and the reported comment-thread concerns have been addressed (dynamic tab reverts applied per atomiks/LukasTy agreement). What's solid
Minor concerns (not blockers)
|
Fixes #3873
Alternative solution with one detection path, but the DOM as the source of truth to #3883.
Had to be very precise and explicit with tests to ensure this result.
Summary
Previously,
activationDirectionwas computed insideTabsList's click handler (useActivationDirectionDetector), so programmaticvaluechanges in controlled mode never updateddata-activation-direction.Approach
Direction detection is moved from
TabsListtoTabsRoot, where it can observevaluechanges regardless of origin. Two complementary mechanisms ensure correctness:Render-time detection — when
valuechanges, direction is computed and set viasetStateduring render, soTabs.Paneland other children receive the correcttabActivationDirectionon their very first render.previousValueRefis read-only during render to stay compatible with React 18 strict mode; it is synced after commit viauseValueChanged.onValueChangehandler — computes and sets direction together withsetValue, populatingeventDetails.activationDirectionfor the public callback.Changes
TabsRoot.tsxpreviousValueRef,computeActivationDirection(comparesgetBoundingClientRectof old/new tab elements), and the render-time detection blockuseValueChanged(value, ...)to syncpreviousValueRefafter commitonValueChangenow computes direction and setstabActivationDirectiondirectlygetTabElementBySelectedValueearlier (before first usage)TabsList.tsxuseActivationDirectionDetectorhook,getInsethelper, and direction computation fromonTabActivationonTabActivationnow delegates toonValueChangewithout intercepting directionTabsRoot.test.tsxTabs.Panelreceives the correct direction on first render viapanelRenderMock