fix: keep the arrow middleware last when merging custom Floating UI options - #3477
fix: keep the arrow middleware last when merging custom Floating UI options#3477chuckcarpenter wants to merge 1 commit into
Conversation
…ptions
`getFloatingUIOptions` pushed `arrow()` into `options.middleware` before
merging `floatingUIOptions`. Because `deepmerge` concatenates arrays, any
user supplied middleware landed *after* the arrow.
Floating UI runs middleware sequentially, threading `x`/`y` from one to the
next, and `arrow()` computes its offset from the coordinates as they stand on
its own turn. Anything running after it moves the tooltip again and silently
invalidates `middlewareData.arrow`, which `placeArrow()` writes straight to
the DOM. The arrow ends up off by exactly however far the trailing middleware
shifted the element -- 60px for an `offset({ crossAxis: 60 })`, 12px for the
`offset({ mainAxis: 0, crossAxis: 12 })` recipe in our own cookbook.
Merge the user options first, then append the arrow, so it always runs last.
When the user supplies their own `arrow()` middleware theirs still wins and
ours is not appended, preserving the previous behavior.
Tooltip coordinates and resolved placement are unchanged in every case; only
the arrow moves.
Fixes #3034
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Floating UI option builder now preserves user middleware, appends Shepherd’s arrow middleware last when needed, skips it for centered steps or existing arrows, and validates these cases with unit tests. ChangesFloating UI middleware handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This localized change keeps the arrow middleware last so custom middleware no longer leaves the tooltip arrow misaligned; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
shepherd.js/test/unit/utils/floating-ui.spec.js(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.8.1 A config object is using the "root" key, which is not supported in flat config system. Flat configs always act as if they are the root config file, so this key can be safely removed. 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 |
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (1)
🛟 Help
|
1 participant
Fixes #3034.
The bug
getFloatingUIOptionspushedarrow()intooptions.middlewarebefore merging the user'sfloatingUIOptions.deepmergeconcatenates arrays, so any user-supplied middleware landed after the arrow:Floating UI runs middleware sequentially, threading
x/yfrom one to the next, andarrow()computes its offset from the coordinates as they stand on its own turn. Anything running after it moves the tooltip again and silently invalidatesmiddlewareData.arrow— whichplaceArrow()then writes straight to the DOM.The arrow ends up off by exactly however far the trailing middleware shifted the element. Measured against the real Floating UI middleware chain:
[shift({padding: 32}), offset(16)](the issue's repro)offset({ mainAxis: 0, crossAxis: 60 })offset({ mainAxis: 0, crossAxis: 12 })— our own cookbook recipeThat third row is worth calling out: the Offsets recipe in our docs has been shipping this bug.
It presents as silent because
middlewareData.arrow.centerOffsetis also computed atarrow()'s turn, so the stale data looks perfectly healthy from the outside.The fix
Merge the user's options first, then append
arrow()— so it always runs last. If the user supplies their ownarrow()middleware, theirs still wins and ours is not appended, preserving current behavior (previously theirs won by being last; now it wins by suppressing ours).Tooltip coordinates and resolved placement are unchanged in every case — only the arrow moves.
What was deliberately not done
Two larger changes were prototyped and rejected after testing:
offset()to the front. Unnecessary (arrow-last alone does 100% of the work) and actively harmful — it feeds offset intoflip's overflow detection, flipping placementbottom→topand moving a tooltip 250px in one test.mergeTooltipConfig.test/unit/step.spec.jsdeliberately asserts that tour-level and step-level middleware concatenate (anoffset({crossAxis: 32})and anoffset({crossAxis: -32})summing to zero). That behavior is intentional and is left alone.Testing
New
test/unit/utils/floating-ui.spec.js, 7 tests asserting on actual middleware order, not length. Mutation-tested: 13 deliberately broken variants of the fix were built and run against the suite — including arrow-first, double-arrow, dropped user middleware, dropped falsy entries, missing centre guard, hardcoded padding, and wrong arrow element. All are caught.describe('arrow padding'))types:check, and build all cleanNote for whoever reviews both: this adds a file at the same path as #3441, so whichever lands second needs to merge the two
describebodies by hand. The source files merge cleanly.Semver: patch.
getFloatingUIOptionsis not part of the published type surface, and the only behavior change is an arrow moving from a demonstrably wrong position to the correct one.🤖 Generated with Claude Code
Summary by CodeRabbit