feat(tooltips): add delayHide logic - #173
Conversation
📝 WalkthroughWalkthroughThe Tooltip component is enhanced to support mobile and touch devices with auto-hiding behavior. A touch-detection mechanism differentiates device types, enabling tooltips to open via click and automatically hide after 1 second on touch devices. Desktop behavior remains unchanged. SSR compatibility is improved through conditional document access. Changes
Sequence DiagramsequenceDiagram
participant User
participant TooltipComp as Tooltip Component
participant RTT as ReactTooltip
participant Browser
Note over User,Browser: Touch Device Flow (NEW)
User->>TooltipComp: Click (ontouchstart detected)
TooltipComp->>TooltipComp: showTooltip() → isOpen=true
TooltipComp->>RTT: Pass isOpen={true}
RTT->>Browser: Render tooltip
TooltipComp->>Browser: setTimeout(1000ms)
Browser-->>TooltipComp: Timeout fires
TooltipComp->>TooltipComp: isOpen=false
TooltipComp->>RTT: Pass isOpen={false}
RTT->>Browser: Hide tooltip
Note over User,Browser: Non-touch Device Flow (UNCHANGED)
User->>TooltipComp: Hover/Focus
TooltipComp->>RTT: Default behavior (no isOpen prop)
RTT->>Browser: Show/hide via native hover
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Tooltip.jsx (1)
42-50: Major: Keyboard accessibility broken on touch devices.The fallback
<span>hastabIndex={0}for keyboard navigation but only handlesonClick. On touch devices, keyboard users can focus the span but pressing Enter or Space won't trigger the tooltip because those keys don't fire click events consistently across browsers.🔎 Recommended fix to add keyboard event handler
<span data-tooltip-id={tooltipId} data-tooltip-content={content} tabIndex={0} onClick={showTooltip} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + showTooltip(); + } + }} > {children} </span>Note: The same keyboard handling should also be considered for the cloned child element case (lines 35-40) if the child is focusable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Tooltip.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Tooltip.jsx (1)
src/components/NavBar.jsx (1)
isOpen(10-10)
🔇 Additional comments (3)
src/components/Tooltip.jsx (3)
3-8: LGTM!The new imports are appropriate for the touch device handling functionality.
18-18: LGTM!State initialization is correct for controlling tooltip visibility.
59-59: LGTM! SSR safety improvement.The conditional
documentaccess prevents SSR errors and properly handles server-side rendering scenarios.
| // Detect touch-capable devices (mobile/tablet). Guard for SSR. | ||
| const isTouchDevice = typeof window !== 'undefined' && ( | ||
| (typeof navigator !== 'undefined' && Number(navigator.maxTouchPoints) > 0) || | ||
| (typeof window.matchMedia === 'function' && window.matchMedia('(pointer: coarse)').matches) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
touch device detection React 19 best practices navigator.maxTouchPoints matchMedia
💡 Result:
Short best-practices for detecting "touch" in React 19 (concise):
-
Prefer feature-detection, not UA sniffing. Use a combination (fallbacks) because no single API is perfect: navigator.maxTouchPoints, pointer/any-pointer media queries, and an ontouchstart fallback. (MDN docs). [1][2]
-
Don’t assume “touch == mobile” or a single input type. Many devices have both touch + mouse; prefer making UI adaptive (CSS media queries) or offer a user toggle. [3]
-
Avoid running detection on the server. For SSR/hydration, run detection only on the client (inside useEffect) and start with a stable initial state to avoid mismatch. Update UI reactively rather than conditioning large tree structure on detection during render. [1][3]
-
Prefer CSS for layout/spacing changes (pointer/any-pointer media queries) and use JS only when behavior must change (e.g., gesture handlers). Media queries express capability more directly and are future-proof. [2][3]
Minimal, robust detection pattern (React 19 hook example)
- Use navigator.maxTouchPoints first, then matchMedia('(any-pointer: coarse)') or '(pointer: coarse)', then 'ontouchstart' as last-resort fallback.
- Run in useEffect to avoid SSR/hydration issues.
Example hook (concept):
- initial state: false
- in useEffect:
- if (typeof navigator !== 'undefined' && navigator.maxTouchPoints > 0) set true
- else if (typeof window !== 'undefined' && window.matchMedia('(any-pointer: coarse)').matches) set true
- else if ('ontouchstart' in window) set true
- optionally add a matchMedia listener to update on input changes and cleanup on unmount
Quick reasons:
- navigator.maxTouchPoints is explicit about touch-capable contact points. Use it first. [1]
- any-pointer/pointer media queries reflect available pointer characteristics; CSS is better for layout. Beware that any-pointer is the union of inputs (so interpret cautiously). [2][3]
- Combining methods and offering a user override is the most reliable UX approach. [3]
References
- navigator.maxTouchPoints (MDN). [1]
- pointer / any-pointer media features (MDN). [2]
- Practical guidance: don’t assume single input; prefer adaptive UI and user toggles (CSS-Tricks). [3]
Move touch detection to useEffect to avoid hydration mismatches, and add missing fallback.
The current approach runs detection at render time, creating a hydration mismatch in SSR: the server renders with window undefined (always false) while the client evaluates the actual touch capability, causing React to fail hydration. Move detection into a useEffect hook with an initial state of false and update reactively.
Additionally, the detection should include three methods with proper fallbacks: check navigator.maxTouchPoints > 0 first, then matchMedia('(any-pointer: coarse)') (not just '(pointer: coarse)'), and finally 'ontouchstart' in window as a last resort. Consider adding a matchMedia listener to update state if device input capabilities change.
🤖 Prompt for AI Agents
In src/components/Tooltip.jsx around lines 20 to 24, move the touch-capability
detection out of render and into a useEffect: initialize a local state
isTouchDevice to false (so server and initial client render match), then in
useEffect (guarded to run only when window is defined) detect touch by checking
navigator.maxTouchPoints > 0 first, then window.matchMedia('(any-pointer:
coarse)').matches, and finally 'ontouchstart' in window as a fallback; set state
accordingly and attach a matchMedia listener to respond to capability changes,
cleaning up the listener on unmount.
There was a problem hiding this comment.
@CodeRabbit, please explain this more plainly and show some snippets to reinforce your explanation.
There was a problem hiding this comment.
@Ryan-Millard, sure! Let me break this down step by step.
The Problem: Hydration Mismatch
When your app uses Server-Side Rendering (SSR), React first renders your component on the server (where there's no window or navigator), then sends that HTML to the browser. Once in the browser, React renders again on the client and tries to "hydrate" (match up) with the server HTML.
The issue: Your current code checks for touch devices during render:
- On the server:
windowis undefined, soisTouchDeviceis alwaysfalse - On the client:
windowexists, soisTouchDevicemight betrueon a mobile device
This mismatch causes React to throw a hydration error because the server HTML doesn't match what React rendered on the client.
Current Code (Problematic)
// ❌ This runs during render, causing hydration mismatch
const isTouchDevice = typeof window !== 'undefined' && (
(typeof navigator !== 'undefined' && Number(navigator.maxTouchPoints) > 0) ||
(typeof window.matchMedia === 'function' && window.matchMedia('(pointer: coarse)').matches)
);Fixed Code
Move the detection into a useEffect hook and use state:
import { useState, useEffect } from 'react';
export default function Tooltip({ content, children, id, dynamicPositioning = true }) {
const reactId = useId();
const tooltipId = id || `tooltip-${reactId}`;
const [isOpen, setIsOpen] = useState(false);
// Start with false so server and initial client render match
const [isTouchDevice, setIsTouchDevice] = useState(false);
// Detect touch devices only on the client, after mount
useEffect(() => {
let mediaQuery;
const detectTouch = () => {
// Method 1: Check touch points (most reliable)
if (navigator.maxTouchPoints > 0) {
return true;
}
// Method 2: Check pointer media query
if (window.matchMedia('(any-pointer: coarse)').matches) {
return true;
}
// Method 3: Fallback for older browsers
if ('ontouchstart' in window) {
return true;
}
return false;
};
setIsTouchDevice(detectTouch());
// Optional: Listen for input changes (e.g., plugging in a mouse)
mediaQuery = window.matchMedia('(any-pointer: coarse)');
const handleChange = () => setIsTouchDevice(detectTouch());
if (mediaQuery.addEventListener) {
mediaQuery.addEventListener('change', handleChange);
}
// Cleanup listener on unmount
return () => {
if (mediaQuery?.removeEventListener) {
mediaQuery.removeEventListener('change', handleChange);
}
};
}, []); // Run once after mount
const showTooltip = () => {
if (!isTouchDevice) return;
setIsOpen(true);
setTimeout(() => setIsOpen(false), 1000);
};
// ... rest of your component
}Key Changes Explained
-
State for touch detection:
const [isTouchDevice, setIsTouchDevice] = useState(false);- Starts as
falseon both server and client - No mismatch!
- Starts as
-
Detection in useEffect: Only runs after the component mounts in the browser
- Server never runs this code
- Client updates the state after hydration is complete
-
Three fallback methods:
navigator.maxTouchPoints > 0: Most modern and reliablewindow.matchMedia('(any-pointer: coarse)'): Detects coarse pointer (finger/stylus)'ontouchstart' in window: Older browser fallback
-
Media query listener (optional): Detects if input method changes (e.g., user plugs in a mouse on a tablet)
Why This Fixes It
- ✅ Server and initial client render both see
isTouchDevice = false - ✅ After mount,
useEffectdetects touch and updates state if needed - ✅ Component re-renders with correct value, no hydration error
- ✅ More robust detection with three fallback methods
Does this make it clearer? Let me know if you'd like me to explain any specific part further!
| const showTooltip = () => { | ||
| if (!isTouchDevice) return; | ||
|
|
||
| setIsOpen(true); | ||
| setTimeout(() => setIsOpen(false), 1000); | ||
| }; |
There was a problem hiding this comment.
Critical: Memory leak and unsafe setState on unmounted component.
The setTimeout is not cleaned up, causing multiple critical issues:
- If the component unmounts before 1 second,
setIsOpen(false)will be called on an unmounted component, triggering React warnings and potential memory leaks. - Rapid clicks create multiple overlapping timeouts that all execute, causing unnecessary setState calls.
- No mechanism exists to cancel the timeout if the user interacts with the tooltip again.
🔎 Recommended fix using useRef and cleanup
const reactId = useId();
const tooltipId = id || `tooltip-${reactId}`;
const [isOpen, setIsOpen] = useState(false);
+ const timeoutRef = useRef(null);
// Detect touch-capable devices (mobile/tablet). Guard for SSR.
const isTouchDevice = typeof window !== 'undefined' && (
(typeof navigator !== 'undefined' && Number(navigator.maxTouchPoints) > 0) ||
(typeof window.matchMedia === 'function' && window.matchMedia('(pointer: coarse)').matches)
);
// On touch: open + auto-close after 1s
const showTooltip = () => {
if (!isTouchDevice) return;
+ // Clear any existing timeout
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+
setIsOpen(true);
- setTimeout(() => setIsOpen(false), 1000);
+ timeoutRef.current = setTimeout(() => {
+ setIsOpen(false);
+ timeoutRef.current = null;
+ }, 1000);
};
+
+ // Cleanup timeout on unmount
+ useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);Don't forget to import useRef and useEffect:
import {
useId,
cloneElement,
isValidElement,
useState,
+ useRef,
+ useEffect,
} from 'react';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const showTooltip = () => { | |
| if (!isTouchDevice) return; | |
| setIsOpen(true); | |
| setTimeout(() => setIsOpen(false), 1000); | |
| }; | |
| const showTooltip = () => { | |
| if (!isTouchDevice) return; | |
| // Clear any existing timeout | |
| if (timeoutRef.current) { | |
| clearTimeout(timeoutRef.current); | |
| } | |
| setIsOpen(true); | |
| timeoutRef.current = setTimeout(() => { | |
| setIsOpen(false); | |
| timeoutRef.current = null; | |
| }, 1000); | |
| }; |
🤖 Prompt for AI Agents
In src/components/Tooltip.jsx around lines 27 to 32, the showTooltip timeout is
never tracked or cleared which can call setIsOpen on an unmounted component and
allow overlapping timeouts; fix by importing useRef and useEffect, store the
timeout ID in a ref, clear any existing timeout before creating a new one
(clearTimeout(timeoutRef.current)), assign the new timeout ID to the ref, and
add a useEffect cleanup that clears timeoutRef.current on unmount to prevent
stale setState calls and memory leaks.
| const childWithTooltip = isValidElement(children) | ||
| ? cloneElement(children, { | ||
| 'data-tooltip-id': tooltipId, | ||
| 'data-tooltip-content': content, | ||
| onClick: showTooltip, | ||
| }) |
There was a problem hiding this comment.
Critical: onClick handler overwriting causes breaking change.
The onClick: showTooltip prop in cloneElement completely replaces any existing onClick handler on the child element, breaking existing functionality. If a consumer passes a child with its own onClick, that handler will be lost.
🔎 Recommended fix to merge onClick handlers
const childWithTooltip = isValidElement(children)
? cloneElement(children, {
'data-tooltip-id': tooltipId,
'data-tooltip-content': content,
- onClick: showTooltip,
+ onClick: (e) => {
+ // Call original onClick if it exists
+ if (children.props.onClick) {
+ children.props.onClick(e);
+ }
+ // Then call our handler
+ showTooltip();
+ },
})
: (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const childWithTooltip = isValidElement(children) | |
| ? cloneElement(children, { | |
| 'data-tooltip-id': tooltipId, | |
| 'data-tooltip-content': content, | |
| onClick: showTooltip, | |
| }) | |
| const childWithTooltip = isValidElement(children) | |
| ? cloneElement(children, { | |
| 'data-tooltip-id': tooltipId, | |
| 'data-tooltip-content': content, | |
| onClick: (e) => { | |
| // Call original onClick if it exists | |
| if (children.props.onClick) { | |
| children.props.onClick(e); | |
| } | |
| // Then call our handler | |
| showTooltip(); | |
| }, | |
| }) |
🤖 Prompt for AI Agents
In src/components/Tooltip.jsx around lines 35 to 40, the cloneElement call
unconditionally sets onClick: showTooltip which overwrites any existing child
onClick handler; instead create a merged onClick handler that calls the child's
original onClick (if present and a function) and then calls showTooltip,
preserving the original call signature and this binding (e.g., (...args) => { if
(typeof child.props.onClick === 'function') child.props.onClick.apply(this,
args); showTooltip.apply(this, args); }), and pass that merged handler into
cloneElement so the child's original behavior is preserved while still showing
the tooltip.
| openOnFocus | ||
| isOpen={isTouchDevice ? isOpen : undefined} |
There was a problem hiding this comment.
Major: Focus behavior broken on touch devices with keyboard input.
On touch devices, the tooltip uses controlled mode (isOpen prop), but openOnFocus (line 62) cannot work in this mode because focus events aren't captured. This breaks accessibility for keyboard users on touch devices (e.g., iPad with external keyboard, Android devices with keyboard).
Users can focus the element via keyboard but the tooltip won't open on focus—only on click.
🔎 Recommended fix to handle focus events on touch devices
Add focus/blur handlers to manage isOpen state:
const showTooltip = () => {
if (!isTouchDevice) return;
setIsOpen(true);
setTimeout(() => setIsOpen(false), 1000);
};
+ const handleFocus = () => {
+ if (isTouchDevice) {
+ showTooltip();
+ }
+ };
// If child is a single valid React element, attach tooltip attributes
const childWithTooltip = isValidElement(children)
? cloneElement(children, {
'data-tooltip-id': tooltipId,
'data-tooltip-content': content,
onClick: showTooltip,
+ onFocus: handleFocus,
})
: (
<span
data-tooltip-id={tooltipId}
data-tooltip-content={content}
tabIndex={0}
onClick={showTooltip}
+ onFocus={handleFocus}
>
{children}
</span>
);Note: This also requires merging with existing onFocus handlers on cloned children (similar to the onClick merge issue).
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@CodeRabbit, this requires a committable suggestion. Even if a line range is outside a PR's diff, you should still suggest it to be safe.
There was a problem hiding this comment.
@Ryan-Millard, here's the complete code change to fix the focus behavior on touch devices with keyboard input:
export default function Tooltip({
content,
children,
id,
dynamicPositioning = true,
}) {
const reactId = useId();
const tooltipId = id || `tooltip-${reactId}`;
const [isOpen, setIsOpen] = useState(false);
// Detect touch-capable devices (mobile/tablet). Guard for SSR.
const isTouchDevice = typeof window !== 'undefined' && (
(typeof navigator !== 'undefined' && Number(navigator.maxTouchPoints) > 0) ||
(typeof window.matchMedia === 'function' && window.matchMedia('(pointer: coarse)').matches)
);
// On touch: open + auto-close after 1s
const showTooltip = () => {
if (!isTouchDevice) return;
setIsOpen(true);
setTimeout(() => setIsOpen(false), 1000);
};
const handleFocus = () => {
if (isTouchDevice) {
showTooltip();
}
};
// If child is a single valid React element, attach tooltip attributes
const childWithTooltip = isValidElement(children)
? cloneElement(children, {
'data-tooltip-id': tooltipId,
'data-tooltip-content': content,
onClick: showTooltip,
onFocus: handleFocus,
})
: (
<span
data-tooltip-id={tooltipId}
data-tooltip-content={content}
tabIndex={0}
onClick: showTooltip
onFocus={handleFocus}
>
{children}
</span>
);
return (
<>
{childWithTooltip}
<ReactTooltip
id={tooltipId}
place="right"
appendTo={typeof document !== 'undefined' ? document.body : undefined}
positionStrategy="fixed"
fallbackPlacements={dynamicPositioning ? ['bottom', 'top', 'left'] : []}
openOnFocus
isOpen={isTouchDevice ? isOpen : undefined}
/>
</>
);
}Key changes:
- Added
handleFocusfunction that callsshowTooltip()on touch devices - Attached
onFocus: handleFocusto the cloned child element - Attached
onFocus={handleFocus}to the fallback span wrapper
Important note: When cloning children that may already have their own onFocus handler, you'll need to merge the handlers to avoid overwriting existing behavior. This can be done by checking if children.props.onFocus exists and calling both handlers in sequence.
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is great stuff. Thank you for the clean code!
- Please will you implement the changes @CodeRabbit suggested - you can find them earlier in the comments on this PR. @CodeRabbit suggested:
- Moving the setup of
isTouchDeviceto auseEffectblock. - Clearing the timeout in
showTooltipto avoid memory leaks. - The
onClicksetup in thecloneElementcall replaces any existing click handlers on the tooltip's children, which leads to the bug in the video below (clicking elements with handlers only shows the tooltip):
https://github.com/user-attachments/assets/2f51eca7-c3d5-47cd-b89e-289f2c60303b - Fixing the focus behavior on
touch devices with a keyboard layout(like an iPad with a keyboard connected to it).
- Moving the setup of
- Please will you add documentation & tests for the new changes you implemented
- We use Docusaurus for documentation (click here to see how to use it)
- For tests, we use Vitest
Thank you again. I can already see this being a helpful addition.🦔
|
Hey, @Karel-cz - just checking in... Are you still interested in this pull request? There's no pressure, I just don't want this one to sit around for too long. If you are unable to continue, someone else can pick up from where you left off. |
|
@Karel-cz, this pull request is being closed because it has not been updated since it was reviewed 2 weeks ago. You are more than welcome to open a new pull request or ask for this one to be reopened if you decide to implement the requested changes. Thank you for your contribution! Unfortunately, I do not have enough spare time to fix up this PR and implement what I need to. |
2 participants
Please choose one of the following:
If none of these fit, you may use this default to describe your change manually.
If this is the right template, go ahead and complete it below 👇
📌 Description
Currently, the Tooltip component on mobile devices stays visible until the user taps elsewhere, which can feel “sticky” and negatively affect UX.
delayHide was considered, but it only runs after a real hide event (mouseleave/blur), which does not occur on touch devices. Therefore, it cannot achieve the required behavior.
Solution: The tooltip is now manually controlled on touch devices and automatically closes after 1 second, while desktop hover/focus behavior remains unchanged.
Fixes #168
✅ Type of Change
Place an "x" in the brackets below:
🧪 How Has This Been Tested?
Please describe how you tested your changes (e.g., unit tests, manual testing, screenshots, etc.)
Manual testing on iOS and Android devices and simulators.
Verified tooltips hide automatically after 1 second on touch devices.
Verified desktop tooltips retain normal hover/focus behavior.
Confirmed positioning and fallback placements remain correct.
🧩 Checklist
Place an "x" in the brackets below:
📸 Screenshots / Demo (if applicable)
Paste images, GIFs, or demo links here.
Kapture.2025-12-30.at.17.24.02.mp4
💬 Additional Context
Anything else relevant to the PR.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.