fix: keep clickable tooltips open while pointer moves to body#1269
Conversation
Restore the missing `rendered` dep so tooltip-body mouseover/mouseout listeners attach after the tooltip mounts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds a ChangesTooltip Render State Tracking
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
danielbarion
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks!
Summary
This fixes a regression bug, which appeared in v6.0.0
Solution: re-introduce
rendereddependency on the effect that attaches the tooltip-bodymouseover/mouseoutlisteners. Without it, clickable tooltips close as soon as the pointer leaves the anchor, making any interactive content inside the tooltip unreachable.I have tested this solution with a local package using
npm linkand this change fixes the issue.Reproduction
Live sandbox: https://codesandbox.io/p/devbox/happy-tharp-g39qff
A clickable tooltip with content the user is supposed to interact with (e.g. an upgrade button):
Hover the anchor -> tooltip opens. Move the pointer toward the tooltip -> tooltip closes the moment the pointer leaves the anchor, before the user can reach the button.
Root cause
The effect at
use-tooltip-events.tsxattaches two listeners directly to the tooltip wrapper:Those listeners are how the 100ms hide grace period (the one started when the pointer leaves the anchor) knows to suppress closing while the pointer is on the tooltip body.
The effect's deps array was
[actualOpenEvents, actualCloseEvents, float, clickable]. None of those change between the initial mount and the moment the tooltip first becomesrendered. So the effect runs once, on mount, whentooltipRef.currentis stillnull— the tooltip wrapper hasn't been committed to the DOM yet (renderedisfalse,tooltipNodeisnull). The optional chaining ontooltipElement?.addEventListener(...)silently no-ops, the listeners are never attached, and the effect never re-runs.hoveringTooltip.currenttherefore staysfalseforever. The 100ms grace period always expires unsuppressed → tooltip closes the moment you leave the anchor.Fix
Add
renderedto the deps array so the effect re-runs aftersetRendered(true)commits, whentooltipRef.currentis now a real element. Listeners get attached.hoveringTooltipflips correctly.Test
There was already a test for this exact scenario —
keeps a clickable tooltip open while the pointer moves into itintooltip-interaction-behavior.spec.js. It was passing on broken code because of two compounding weaknesses, both of which had to be fixed for the test to be meaningful:unhoverAnchor, before the 100ms hide grace period could elapse. Whatever the listeners would or wouldn't have done, the timer hadn't fired yet — the test was just observing "tooltip exists immediately after starting close" which is true unconditionally. NowadvanceTimers(150)past the hide-delay so the close path actually runs.expect(...).toBeInTheDocument()would still pass for a tooltip in the closing-but-not-yet-removed state (CSS transition is in progress, unmount happens viaonTransitionEnd). To distinguish "still active" from "actively closing" the assertion needs to check for thereact-tooltip__showclass — switched to that, matching the convention intooltip-props.spec.jsandtooltip-close-and-delay-behavior.spec.js.Verified that the strengthened test:
Expected the element to have class react-tooltip__showRegression history analysis
The dep was inadvertently dropped in db420de9 (Mar 2024, "chore: add hook rules to eslint and refactor relevant code"). That commit added
react-hooks/exhaustive-depsand mechanically reorganized the deps array to satisfy it. ESLint can't see thatrenderedisn't a value used by the effect — it's a deliberate trigger to re-run whentooltipRef.currentflips fromnullto a real element. The pre-existing comment that explained this exact intent......was kept around the now-orphaned location for over a year, then finally cleaned up in a24f389d ("split events and anchors on it's own file") when the file was extracted. By v6 both the comment and the dep were gone.
The other two deps removed in db420de (
updateTooltipPosition,hasClickEvent) stay removed — both have compensating refactors that make them redundant:updateTooltipPositionis now consumed via a ref-stable wrapper (updateTooltipPositionRef.current()in Effect 2), so capturing the latest callback doesn't require a re-run.hasClickEventpropagates transitively throughactualGlobalCloseEvents'suseMemodeps.renderedwas the only dropped dep without compensation.To prevent this from happening again on the next ESLint cleanup, this PR also adds an inline comment on the deps array explaining why
renderedis there, alongside the existing// eslint-disable-next-line react-hooks/exhaustive-deps.Summary by CodeRabbit
Bug Fixes
Tests