refactor: centralize hover/long-press detection into useInteractionMode hook#2022
Open
refactor: centralize hover/long-press detection into useInteractionMode hook#2022
Conversation
Signed-off-by: prxt6529 <117092621+prxt6529@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useInteractionModeHook as useInteractionMode()
participant InteractionMgr as InteractionModeManager
participant BrowserAPI as MediaQuery/PointerEvents
Component->>useInteractionModeHook: call hook (subscribe)
useInteractionModeHook->>InteractionMgr: ensure initialized & subscribe listener
InteractionMgr->>BrowserAPI: setup matchMedia & pointer listeners (any-hover, pointer)
BrowserAPI-->>InteractionMgr: emits changes (pointermove, pointerdown, mql change)
InteractionMgr-->>useInteractionModeHook: push updated state (enableHoverUI/enableLongPress)
useInteractionModeHook-->>Component: rerender with new flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/useDeviceInfo.ts (1)
67-95:⚠️ Potential issue | 🟠 MajorInteraction-mode changes won’t propagate until window resize.
infois recomputed only inside the resize handler. IfenableLongPresschanges,getInfochanges, butsetInfois not called unless a resize happens, leaving stale device info.Proposed fix
useEffect(() => { @@ const update = () => setInfo((prev) => { const next = getInfo(); @@ return next; }); + update(); + if (hasEventListenerApi) { globalThis.addEventListener("resize", update); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useDeviceInfo.ts` around lines 67 - 95, The effect only updates state on "resize", so when getInfo (e.g., due to enableLongPress) changes the info stays stale; modify the useEffect containing getInfo/update to call update() immediately (invoke update once inside the effect) so state refreshes whenever the effect re-runs, and keep the existing addEventListener/removeEventListener logic for "resize" as-is; reference the useEffect, getInfo, and update identifiers to locate where to add the immediate update() call.
🧹 Nitpick comments (7)
src/interaction/useInteractionMode.ts (1)
57-80: Add teardown for global listeners in the singleton lifecycle.
init()installs global listeners but nothing removes them. In dev (Fast Refresh) and tests, this can accumulate duplicate handlers and unnecessarypointermovework. Consider tracking a teardown and running it when the last subscriber unsubscribes.♻️ Suggested lifecycle-safe pattern
let cachedState: InteractionModeState = DEFAULT_STATE; let initialized = false; const subscribers = new Set<(state: InteractionModeState) => void>(); +let teardown: (() => void) | null = null; const init = () => { if (initialized || typeof window === "undefined") { return; } @@ mediaQueries.forEach((query) => { query.addEventListener("change", updateCapabilities); }); window.addEventListener("pointerdown", handlePointerDown, { passive: true }); window.addEventListener("pointermove", handlePointerMove, { passive: true }); + + teardown = () => { + mediaQueries.forEach((query) => { + query.removeEventListener("change", updateCapabilities); + }); + window.removeEventListener("pointerdown", handlePointerDown); + window.removeEventListener("pointermove", handlePointerMove); + initialized = false; + }; }; @@ return () => { subscribers.delete(onChange); + if (subscribers.size === 0 && teardown) { + teardown(); + teardown = null; + } }; }, []);Also applies to: 85-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/useInteractionMode.ts` around lines 57 - 80, init() installs global listeners (pointerdown/pointermove and media query "change") but never removes them, causing leaks in dev/tests; add a teardown that removes the same listeners and restores initialized so handlers aren't duplicated. Implement a subscription count or reference to the teardown function in the module scope, increment when the first subscriber calls init()/subscribe, and on unsubscribe decrement and if 0 run teardown: remove window event listeners for handlePointerDown and handlePointerMove, call removeEventListener("change", updateCapabilities) for each item in mediaQueries, and reset initialized and any cached references (e.g., cachedState) as appropriate; ensure typeof window checks remain. Use the existing symbols init, handlePointerDown, handlePointerMove, mediaQueries, updateCapabilities, updatePointerType, cachedState, and initialized to locate and wire up the cleanup logic.hooks/useSidebarController.ts (1)
55-55: Consider semantic alignment with WebSidebar.tsx.This file derives
isTouchScreenfromenableLongPress, whileWebSidebar.tsxderives it from!enableHoverUI. These have subtly different semantics:
enableLongPress:hoverNone || lastPointerType === "touch"— true when long-press interactions are appropriate!enableHoverUI:!(canHover || hasFinePointer)— true when the device lacks hover capabilityOn hybrid devices (e.g., a laptop with touchscreen using a mouse), these can differ. If the intent is to detect "is this a touch-centric device",
!enableHoverUImight be more consistent withWebSidebar.tsx. If the intent is "is the user currently using touch", thenenableLongPressis correct.Also applies to: 86-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useSidebarController.ts` at line 55, The current useSidebarController.ts maps isTouchScreen from enableLongPress (const { enableLongPress: isTouchScreen } = useInteractionMode()), which semantically differs from WebSidebar.tsx that uses !enableHoverUI; decide which semantics you want and make them consistent: if you intend "touch-centric device" derive isTouchScreen from !enableHoverUI (e.g., const { enableHoverUI } = useInteractionMode(); const isTouchScreen = !enableHoverUI), otherwise keep enableLongPress but add a comment clarifying it means "user currently using touch"; update the other occurrences mentioned around lines 86-89 similarly so all uses of isTouchScreen in this module follow the same chosen definition.components/common/OverlappingAvatars.tsx (1)
82-82:showTooltipshould be keyed offenableHoverUI.Line 82 uses
enableLongPressasisTouchDevice; Lines 109-110 then disable tooltips based on that value. On hybrid devices this can incorrectly suppress hover tooltips.Suggested fix
- const { enableLongPress: isTouchDevice } = useInteractionMode(); + const { enableHoverUI } = useInteractionMode(); - const showTooltip = - !isTouchDevice && item.title !== undefined && item.title !== ""; + const showTooltip = + enableHoverUI && item.title !== undefined && item.title !== "";Also applies to: 109-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/common/OverlappingAvatars.tsx` at line 82, The code currently derives isTouchDevice from useInteractionMode() via the enableLongPress flag and uses it to disable tooltips; instead, base tooltip visibility on the enableHoverUI flag from useInteractionMode. Update the destructuring (in OverlappingAvatars.tsx where you call useInteractionMode) to pull enableHoverUI (or add a const showTooltip = enableHoverUI) and replace uses of isTouchDevice for tooltip gating (the logic currently at the showTooltip / tooltip enablement around the render at lines ~109-110) so hover tooltips are suppressed only when enableHoverUI is false.components/brain/left-sidebar/web/WebDirectMessagesList.tsx (1)
36-36: Tooltip gating should use hover capability, not long-press capability.Line 36 derives
isTouchDevicefromenableLongPress, and Line 187 hides the tooltip when that flag is true. On hybrid input, this can hide hover tooltip even when hover is supported.Suggested fix
- const { enableLongPress: isTouchDevice } = useInteractionMode(); + const { enableHoverUI } = useInteractionMode(); - {!isTouchDevice && shouldRenderCreateDirectMessage && ( + {enableHoverUI && shouldRenderCreateDirectMessage && (Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/left-sidebar/web/WebDirectMessagesList.tsx` at line 36, The tooltip gating currently uses enableLongPress from useInteractionMode (assigned to isTouchDevice) which hides hover tooltips incorrectly; change the logic to read the hover capability instead (e.g., const { enableHover: isHoverSupported } = useInteractionMode() or similar) and use that boolean to decide whether to show/hide the tooltip in the component (replace references to isTouchDevice where tooltip hiding is applied, e.g., the tooltip hide condition around the tooltip render/props). Ensure you rename the variable and update all uses so hover capability—not long-press—is the gating signal.components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx (1)
20-20: UseenableHoverUIfor hover gating;enableLongPressis not a touch-only flag.Line 20 maps
enableLongPresstoisTouch, and Lines 133-135 use it to force click-only tooltip behavior. On hybrid devices,enableLongPressmay be true after touch input while hover is still available, which suppresses hover UI unexpectedly.Suggested direction
- const { enableLongPress: isTouch } = useInteractionMode(); + const { enableHoverUI, enableLongPress } = useInteractionMode(); - openEvents={isTouch ? { click: true } : { mouseenter: true }} - closeEvents={isTouch ? { click: true } : { mouseleave: true }} + openEvents={{ + ...(enableHoverUI ? { mouseenter: true } : {}), + ...(enableLongPress ? { click: true } : {}), + }} + closeEvents={{ + ...(enableHoverUI ? { mouseleave: true } : {}), + ...(enableLongPress ? { click: true } : {}), + }}Also applies to: 133-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx` at line 20, The code currently maps useInteractionMode().enableLongPress to isTouch and uses that to suppress hover tooltips; replace that mapping to useInteractionMode().enableHoverUI (e.g., const { enableHoverUI: isHoverEnabled }) and update the tooltip logic (the block that forces click-only tooltip around the current lines ~133-135) to check isHoverEnabled (or its negation) instead of enableLongPress/isTouch so hover is gated by actual hover capability on hybrid devices; update any variable names and references in WaveSmallLeaderboardItemOutcomes to match the new identifier.components/waves/winners/podium/WavePodiumItemContentOutcomes.tsx (1)
18-18: RenameisTouchto reflect interaction mode, not device capability.At Line 18,
enableLongPressis an interaction-state flag, soisTouchis a bit misleading. A name likeisTouchInteractionorenableLongPressModewould better match behavior and reduce future misreads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/winners/podium/WavePodiumItemContentOutcomes.tsx` at line 18, Rename the misleading local variable isTouch to reflect interaction mode semantics: change the destructure const { enableLongPress: isTouch } = useInteractionMode(); to use a clearer name such as enableLongPressMode or isTouchInteraction (e.g., const { enableLongPress: isTouchInteraction } = useInteractionMode();), then update all usages in WavePodiumItemContentOutcomes (and any exported/derived identifiers) to the new name to keep intent clear and prevent confusion with device capability.hooks/useDeviceInfo.ts (1)
40-47:hasTouchScreencurrently represents mode, not capability.At Line 40, assigning
hasTouchScreen = enableLongPressmakes this value pointer-mode-dependent, while the name suggests stable hardware capability. Since it also feeds Line 46 (iPadDesktopUA), consider separating capability vs current interaction mode to avoid subtle misclassification drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useDeviceInfo.ts` around lines 40 - 47, The variable hasTouchScreen is incorrectly set from enableLongPress (a mode) — split interaction mode vs hardware capability: introduce a touch capability flag (e.g., hasTouchCapability) computed from feature-detection (navigator.maxTouchPoints > 0 || 'ontouchstart' in window || nav.userAgentData?.mobile) and keep enableLongPress or a renamed isTouchMode as the interaction-mode flag; then change iPadDesktopUA to use hasTouchCapability (not enableLongPress) so iPad desktop UA detection uses actual hardware capability rather than current pointer mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/brain/left-sidebar/waves/BrainLeftSidebarWavePin.tsx`:
- Line 27: The code destructures only enableHoverUI from useInteractionMode()
but later uses isTouchDevice (in the BrainLeftSidebarWavePin component), causing
isTouchDevice to be undefined; fix by also destructuring isTouchDevice from
useInteractionMode() (e.g., const { enableHoverUI, isTouchDevice } =
useInteractionMode()) or, if you intend to derive touch capability from hover
like WebSidebar, replace uses of isTouchDevice with the derived expression
(e.g., const isTouchDevice = !enableHoverUI) and update conditional checks (the
block around the usage that returns "tw-opacity-100") accordingly.
In
`@components/profile-activity/list/items/utils/ProfileActivityLogItemValueWithCopy.tsx`:
- Around line 17-18: The current isTouchScreen = !enableHoverUI incorrectly
treats hybrid devices as non-touch; update the detection in
ProfileActivityLogItemValueWithCopy to combine enableHoverUI with a real
touch-capability check (e.g., navigator.maxTouchPoints > 0, 'ontouchstart' in
window, or matchMedia('(hover: none)') ) so touch-capable devices still show the
touch copy affordance; change the isTouchScreen assignment to use this combined
detection logic while keeping useInteractionMode()/enableHoverUI in
consideration.
In
`@components/user/identity/statements/consolidated-addresses/UserPageIdentityStatementsConsolidatedAddressesItem.tsx`:
- Around line 102-103: The file references the hook useInteractionMode (used to
derive isTouchScreen) but never imports it; add an import for useInteractionMode
from its module where other hooks live (so the const { enableHoverUI } =
useInteractionMode(); line resolves). Locate the use of useInteractionMode in
UserPageIdentityStatementsConsolidatedAddressesItem and add the corresponding
import statement for useInteractionMode at the top of the file alongside other
imports.
In `@components/waves/drops/WaveDropReactions.tsx`:
- Line 43: The code in WaveDropReactions maps enableLongPress to isTouchDevice
and reuses it to suppress tooltip hover, conflating long-press enabling with
hover gating; update the useInteractionMode call to destructure both
enableLongPress and the hover-related flag (e.g., enableHover or disableHover)
instead of reusing enableLongPress, then change any tooltip suppression logic in
WaveDropReactions to use the hover-specific variable (not
isTouchDevice/enableLongPress) so hybrid inputs can allow hover affordances
while still respecting long-press behavior.
In `@components/waves/winners/WaveWinnersSmallOutcome.tsx`:
- Around line 19-23: The component's touch path was effectively removed because
handleClick only calls e.stopPropagation when isTouch is true, leaving no way to
open outcome details on touch; restore an explicit non-hover trigger by wiring
handleClick to toggle or request opening details (either restore local open
state or call the parent open callback/prop) when isTouch is true; update
WaveWinnersSmallOutcome's handleClick to, besides stopPropagation, call the
existing parent handler (e.g., a prop like onOpenOutcome/openOutcome) or set
local open state so the details panel is shown on touch; ensure the touch branch
uses isTouch and the same open/close logic used by the hover/path for desktop so
touch devices can access the outcome details.
In `@src/interaction/useInteractionMode.ts`:
- Line 40: The forEach callback implicitly returns subscriber(cachedState);
update the callback in useInteractionMode (the subscribers.forEach call) to a
block-bodied arrow so it does not return a value — replace the expression-bodied
arrow with a block-bodied one (e.g., subscribers.forEach((subscriber) => {
subscriber(cachedState); });) to satisfy the useIterableCallbackReturn rule and
avoid returning the subscriber call from the iterator.
---
Outside diff comments:
In `@hooks/useDeviceInfo.ts`:
- Around line 67-95: The effect only updates state on "resize", so when getInfo
(e.g., due to enableLongPress) changes the info stays stale; modify the
useEffect containing getInfo/update to call update() immediately (invoke update
once inside the effect) so state refreshes whenever the effect re-runs, and keep
the existing addEventListener/removeEventListener logic for "resize" as-is;
reference the useEffect, getInfo, and update identifiers to locate where to add
the immediate update() call.
---
Nitpick comments:
In `@components/brain/left-sidebar/web/WebDirectMessagesList.tsx`:
- Line 36: The tooltip gating currently uses enableLongPress from
useInteractionMode (assigned to isTouchDevice) which hides hover tooltips
incorrectly; change the logic to read the hover capability instead (e.g., const
{ enableHover: isHoverSupported } = useInteractionMode() or similar) and use
that boolean to decide whether to show/hide the tooltip in the component
(replace references to isTouchDevice where tooltip hiding is applied, e.g., the
tooltip hide condition around the tooltip render/props). Ensure you rename the
variable and update all uses so hover capability—not long-press—is the gating
signal.
In `@components/common/OverlappingAvatars.tsx`:
- Line 82: The code currently derives isTouchDevice from useInteractionMode()
via the enableLongPress flag and uses it to disable tooltips; instead, base
tooltip visibility on the enableHoverUI flag from useInteractionMode. Update the
destructuring (in OverlappingAvatars.tsx where you call useInteractionMode) to
pull enableHoverUI (or add a const showTooltip = enableHoverUI) and replace uses
of isTouchDevice for tooltip gating (the logic currently at the showTooltip /
tooltip enablement around the render at lines ~109-110) so hover tooltips are
suppressed only when enableHoverUI is false.
In `@components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx`:
- Line 20: The code currently maps useInteractionMode().enableLongPress to
isTouch and uses that to suppress hover tooltips; replace that mapping to
useInteractionMode().enableHoverUI (e.g., const { enableHoverUI: isHoverEnabled
}) and update the tooltip logic (the block that forces click-only tooltip around
the current lines ~133-135) to check isHoverEnabled (or its negation) instead of
enableLongPress/isTouch so hover is gated by actual hover capability on hybrid
devices; update any variable names and references in
WaveSmallLeaderboardItemOutcomes to match the new identifier.
In `@components/waves/winners/podium/WavePodiumItemContentOutcomes.tsx`:
- Line 18: Rename the misleading local variable isTouch to reflect interaction
mode semantics: change the destructure const { enableLongPress: isTouch } =
useInteractionMode(); to use a clearer name such as enableLongPressMode or
isTouchInteraction (e.g., const { enableLongPress: isTouchInteraction } =
useInteractionMode();), then update all usages in WavePodiumItemContentOutcomes
(and any exported/derived identifiers) to the new name to keep intent clear and
prevent confusion with device capability.
In `@hooks/useDeviceInfo.ts`:
- Around line 40-47: The variable hasTouchScreen is incorrectly set from
enableLongPress (a mode) — split interaction mode vs hardware capability:
introduce a touch capability flag (e.g., hasTouchCapability) computed from
feature-detection (navigator.maxTouchPoints > 0 || 'ontouchstart' in window ||
nav.userAgentData?.mobile) and keep enableLongPress or a renamed isTouchMode as
the interaction-mode flag; then change iPadDesktopUA to use hasTouchCapability
(not enableLongPress) so iPad desktop UA detection uses actual hardware
capability rather than current pointer mode.
In `@hooks/useSidebarController.ts`:
- Line 55: The current useSidebarController.ts maps isTouchScreen from
enableLongPress (const { enableLongPress: isTouchScreen } =
useInteractionMode()), which semantically differs from WebSidebar.tsx that uses
!enableHoverUI; decide which semantics you want and make them consistent: if you
intend "touch-centric device" derive isTouchScreen from !enableHoverUI (e.g.,
const { enableHoverUI } = useInteractionMode(); const isTouchScreen =
!enableHoverUI), otherwise keep enableLongPress but add a comment clarifying it
means "user currently using touch"; update the other occurrences mentioned
around lines 86-89 similarly so all uses of isTouchScreen in this module follow
the same chosen definition.
In `@src/interaction/useInteractionMode.ts`:
- Around line 57-80: init() installs global listeners (pointerdown/pointermove
and media query "change") but never removes them, causing leaks in dev/tests;
add a teardown that removes the same listeners and restores initialized so
handlers aren't duplicated. Implement a subscription count or reference to the
teardown function in the module scope, increment when the first subscriber calls
init()/subscribe, and on unsubscribe decrement and if 0 run teardown: remove
window event listeners for handlePointerDown and handlePointerMove, call
removeEventListener("change", updateCapabilities) for each item in mediaQueries,
and reset initialized and any cached references (e.g., cachedState) as
appropriate; ensure typeof window checks remain. Use the existing symbols init,
handlePointerDown, handlePointerMove, mediaQueries, updateCapabilities,
updatePointerType, cachedState, and initialized to locate and wire up the
cleanup logic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
components/brain/content/BrainContentPinnedWaves.tsxcomponents/brain/left-sidebar/waves/BrainLeftSidebarWavePin.tsxcomponents/brain/left-sidebar/web/WebDirectMessagesList.tsxcomponents/brain/left-sidebar/web/WebUnifiedWavesListWaves.tsxcomponents/common/OverlappingAvatars.tsxcomponents/header/header-search/HeaderSearchModalItem.tsxcomponents/layout/sidebar/WebSidebar.tsxcomponents/profile-activity/list/items/utils/ProfileActivityLogItemValueWithCopy.tsxcomponents/user/identity/statements/consolidated-addresses/UserPageIdentityStatementsConsolidatedAddressesItem.tsxcomponents/user/identity/statements/utils/UserPageIdentityDeleteStatementButton.tsxcomponents/user/identity/statements/utils/UserPageIdentityStatementsStatement.tsxcomponents/utils/select/dropdown/CommonDropdownItemsMobileWrapper.tsxcomponents/waves/drops/DropMobileMenuHandler.tsxcomponents/waves/drops/WaveDrop.tsxcomponents/waves/drops/WaveDropContent.tsxcomponents/waves/drops/WaveDropReactions.tsxcomponents/waves/drops/participation/EndedParticipationDrop.tsxcomponents/waves/drops/participation/OngoingParticipationDrop.tsxcomponents/waves/drops/participation/ParticipationDropContent.tsxcomponents/waves/drops/winner/DefaultWinnerDrop.tsxcomponents/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsxcomponents/waves/winners/WaveWinnersSmallOutcome.tsxcomponents/waves/winners/podium/WavePodiumItemContentOutcomes.tsxhooks/useDeviceInfo.tshooks/useHasTouchInput.tshooks/useIsTouchDevice.tshooks/useSidebarController.tssrc/interaction/useInteractionMode.ts
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/profile-activity/list/items/utils/ProfileActivityLogItemValueWithCopy.tsx (1)
17-22:⚠️ Potential issue | 🟠 MajorUse
enableLongPressfromuseInteractionModeinstead of reintroducing touch heuristics.At Line 18–22,
navigator.maxTouchPoints/ontouchstartbypass the centralized interaction model and can keepisTouchScreentrue on hybrid devices even during mouse usage, which hides hover tooltip behavior unnecessarily.Proposed fix
- const { enableHoverUI } = useInteractionMode(); - const hasTouchCapability = - typeof navigator !== "undefined" && - (navigator.maxTouchPoints > 0 || - "ontouchstart" in (typeof window !== "undefined" ? window : ({} as Window))); - const isTouchScreen = !enableHoverUI || hasTouchCapability; + const { enableHoverUI, enableLongPress } = useInteractionMode(); + const isTouchInteraction = enableLongPress; @@ - isTouchScreen + isTouchInteraction ? "tw-block" : "tw-opacity-0 group-hover:tw-opacity-100" @@ - {!isTouchScreen && ( + {enableHoverUI && !isTouchInteraction && ( <Tooltip🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/profile-activity/list/items/utils/ProfileActivityLogItemValueWithCopy.tsx` around lines 17 - 22, The component is reintroducing touch heuristics (hasTouchCapability) instead of using the centralized interaction model; update the useInteractionMode destructure to include enableLongPress and replace the hasTouchCapability check so that isTouchScreen is computed from enableHoverUI and enableLongPress (e.g., isTouchScreen = !enableHoverUI || enableLongPress) and remove navigator/ontouchstart logic and the hasTouchCapability variable; adjust any downstream uses of isTouchScreen accordingly in ProfileActivityLogItemValueWithCopy.
🧹 Nitpick comments (1)
components/brain/left-sidebar/web/WebDirectMessagesList.tsx (1)
36-37: Optional: inlineenableHoverUIinstead of creating a pass-through alias.
isHoverSupportedcurrently mirrorsenableHoverUI1:1. You can simplify by usingenableHoverUIdirectly unless the alias improves readability for this file.♻️ Suggested simplification
- const { enableHoverUI } = useInteractionMode(); - const isHoverSupported = enableHoverUI; + const { enableHoverUI } = useInteractionMode();- {isHoverSupported && shouldRenderCreateDirectMessage && ( + {enableHoverUI && shouldRenderCreateDirectMessage && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/brain/left-sidebar/web/WebDirectMessagesList.tsx` around lines 36 - 37, The file defines a redundant alias isHoverSupported = enableHoverUI from useInteractionMode(); remove the pass-through alias and update all uses of isHoverSupported in WebDirectMessagesList.tsx to reference enableHoverUI directly (keep useInteractionMode() and the enableHoverUI destructure). This simplifies the code without changing behavior—search for isHoverSupported usages in the component and replace them with enableHoverUI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/common/OverlappingAvatars.tsx`:
- Around line 110-112: The tooltip gating currently uses showTooltipForItem
which only checks item.title; update that logic to treat a tooltip as present
when either item.tooltipContent or item.title is a non-empty string (e.g., check
item.tooltipContent !== undefined && item.tooltipContent !== "" || item.title
!== undefined && item.title !== "") so tooltipId (constructed from baseId and
index) is set whenever tooltipContent exists even if title is empty, and mirror
this same fix in the other tooltip rendering spot that uses item.tooltipContent
?? item.title so both the showTooltipForItem and the rendering block
consistently allow tooltipContent-only tooltips.
In `@components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx`:
- Around line 20-26: The click handler currently gates touch behavior only on
enableHoverUI from useInteractionMode, which misclassifies hybrid touch taps;
pull lastPointerType (and/or enableLongPress) from useInteractionMode and update
handleClick to treat touch pointers as non-hover triggers—e.g., change the check
to if (!isHoverEnabled || lastPointerType === "touch" || enableLongPress)
e.stopPropagation(); and apply the same conditional update to the other
occurrence (the block at the later 133-135 region) so tap interactions correctly
open/close the tooltip on hybrid devices.
- Around line 133-135: When isHoverEnabled is true the tooltip only binds
mouseenter/mouseleave so keyboard users can't open it; update the hover-enabled
branch where openEvents, closeEvents, and globalCloseEvents are set (the
expressions using isHoverEnabled) to include keyboard handlers: add focus: true
to openEvents and blur: true to closeEvents while leaving globalCloseEvents
as-is for that branch so Tab/focus opens the tooltip and blur closes it.
---
Duplicate comments:
In
`@components/profile-activity/list/items/utils/ProfileActivityLogItemValueWithCopy.tsx`:
- Around line 17-22: The component is reintroducing touch heuristics
(hasTouchCapability) instead of using the centralized interaction model; update
the useInteractionMode destructure to include enableLongPress and replace the
hasTouchCapability check so that isTouchScreen is computed from enableHoverUI
and enableLongPress (e.g., isTouchScreen = !enableHoverUI || enableLongPress)
and remove navigator/ontouchstart logic and the hasTouchCapability variable;
adjust any downstream uses of isTouchScreen accordingly in
ProfileActivityLogItemValueWithCopy.
---
Nitpick comments:
In `@components/brain/left-sidebar/web/WebDirectMessagesList.tsx`:
- Around line 36-37: The file defines a redundant alias isHoverSupported =
enableHoverUI from useInteractionMode(); remove the pass-through alias and
update all uses of isHoverSupported in WebDirectMessagesList.tsx to reference
enableHoverUI directly (keep useInteractionMode() and the enableHoverUI
destructure). This simplifies the code without changing behavior—search for
isHoverSupported usages in the component and replace them with enableHoverUI.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/brain/left-sidebar/waves/BrainLeftSidebarWavePin.tsxcomponents/brain/left-sidebar/web/WebDirectMessagesList.tsxcomponents/common/OverlappingAvatars.tsxcomponents/layout/sidebar/WebSidebar.tsxcomponents/profile-activity/list/items/utils/ProfileActivityLogItemValueWithCopy.tsxcomponents/user/identity/statements/consolidated-addresses/UserPageIdentityStatementsConsolidatedAddressesItem.tsxcomponents/waves/drops/WaveDropReactions.tsxcomponents/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsxcomponents/waves/winners/WaveWinnersSmallOutcome.tsxcomponents/waves/winners/podium/WavePodiumItemContentOutcomes.tsxhooks/useDeviceInfo.tshooks/useSidebarController.tssrc/interaction/useInteractionMode.ts
✅ Files skipped from review due to trivial changes (1)
- components/user/identity/statements/consolidated-addresses/UserPageIdentityStatementsConsolidatedAddressesItem.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- components/layout/sidebar/WebSidebar.tsx
- src/interaction/useInteractionMode.ts
- components/brain/left-sidebar/waves/BrainLeftSidebarWavePin.tsx
- components/waves/winners/WaveWinnersSmallOutcome.tsx
- components/waves/winners/podium/WavePodiumItemContentOutcomes.tsx
- hooks/useSidebarController.ts
Comment on lines
+110
to
+112
| const showTooltipForItem = | ||
| showTooltip && item.title !== undefined && item.title !== ""; | ||
| const tooltipId = showTooltipForItem ? `${baseId}-${index}` : undefined; |
There was a problem hiding this comment.
Tooltip rendering is incorrectly gated only by title.
At Line 110, showTooltipForItem ignores tooltipContent, but at Line 151 the tooltip content source is item.tooltipContent ?? item.title.
This drops valid tooltips when tooltipContent is provided without title.
Proposed fix
- const showTooltipForItem =
- showTooltip && item.title !== undefined && item.title !== "";
+ const hasTooltipText =
+ typeof item.title === "string" && item.title.trim() !== "";
+ const hasTooltipContent = item.tooltipContent != null;
+ const showTooltipForItem =
+ showTooltip && (hasTooltipContent || hasTooltipText);
const tooltipId = showTooltipForItem ? `${baseId}-${index}` : undefined;Also applies to: 141-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/common/OverlappingAvatars.tsx` around lines 110 - 112, The tooltip
gating currently uses showTooltipForItem which only checks item.title; update
that logic to treat a tooltip as present when either item.tooltipContent or
item.title is a non-empty string (e.g., check item.tooltipContent !== undefined
&& item.tooltipContent !== "" || item.title !== undefined && item.title !== "")
so tooltipId (constructed from baseId and index) is set whenever tooltipContent
exists even if title is empty, and mirror this same fix in the other tooltip
rendering spot that uses item.tooltipContent ?? item.title so both the
showTooltipForItem and the rendering block consistently allow
tooltipContent-only tooltips.
Comment on lines
+20
to
26
| const { enableHoverUI: isHoverEnabled } = useInteractionMode(); | ||
|
|
||
| const handleClick = (e: React.MouseEvent) => { | ||
| if (isTouch) { | ||
| if (!isHoverEnabled) { | ||
| e.stopPropagation(); | ||
| setIsOpen(!isOpen); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Hybrid touch behavior regresses when mode is derived only from enableHoverUI.
On hybrid devices, enableHoverUI can stay true while the active pointer is touch, so tap interactions won’t open/close the tooltip correctly (hover-only events remain active). Consider switching trigger mode using lastPointerType === "touch" (or enableLongPress) in addition to hover capability.
Suggested fix
- const { enableHoverUI: isHoverEnabled } = useInteractionMode();
+ const { enableHoverUI, lastPointerType } = useInteractionMode();
+ const useClickTrigger = !enableHoverUI || lastPointerType === "touch";
const handleClick = (e: React.MouseEvent) => {
- if (!isHoverEnabled) {
+ if (useClickTrigger) {
e.stopPropagation();
}
};
@@
- className={`tw-flex tw-items-center tw-rounded-lg tw-border tw-border-solid tw-border-iron-700/50 tw-bg-iron-900/60 tw-px-3 tw-py-1.5 tw-backdrop-blur-sm tw-transition-colors tw-duration-200 desktop-hover:hover:tw-border-iron-600/50 desktop-hover:hover:tw-bg-iron-800/60 ${!isHoverEnabled ? "tw-cursor-pointer" : ""}`}
+ className={`tw-flex tw-items-center tw-rounded-lg tw-border tw-border-solid tw-border-iron-700/50 tw-bg-iron-900/60 tw-px-3 tw-py-1.5 tw-backdrop-blur-sm tw-transition-colors tw-duration-200 desktop-hover:hover:tw-border-iron-600/50 desktop-hover:hover:tw-bg-iron-800/60 ${useClickTrigger ? "tw-cursor-pointer" : ""}`}
@@
- openEvents={isHoverEnabled ? { mouseenter: true } : { click: true }}
- closeEvents={isHoverEnabled ? { mouseleave: true } : { click: true }}
- globalCloseEvents={isHoverEnabled ? {} : { clickOutsideAnchor: true }}
+ openEvents={useClickTrigger ? { click: true } : { mouseenter: true }}
+ closeEvents={useClickTrigger ? { click: true } : { mouseleave: true }}
+ globalCloseEvents={useClickTrigger ? { clickOutsideAnchor: true } : {}}Also applies to: 133-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx`
around lines 20 - 26, The click handler currently gates touch behavior only on
enableHoverUI from useInteractionMode, which misclassifies hybrid touch taps;
pull lastPointerType (and/or enableLongPress) from useInteractionMode and update
handleClick to treat touch pointers as non-hover triggers—e.g., change the check
to if (!isHoverEnabled || lastPointerType === "touch" || enableLongPress)
e.stopPropagation(); and apply the same conditional update to the other
occurrence (the block at the later 133-135 region) so tap interactions correctly
open/close the tooltip on hybrid devices.
Comment on lines
+133
to
+135
| openEvents={isHoverEnabled ? { mouseenter: true } : { click: true }} | ||
| closeEvents={isHoverEnabled ? { mouseleave: true } : { click: true }} | ||
| globalCloseEvents={isHoverEnabled ? {} : { clickOutsideAnchor: true }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "WaveSmallLeaderboardItemOutcomes.tsx" | head -20Repository: 6529-Collections/6529seize-frontend
Length of output: 152
🏁 Script executed:
cat -n components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx | head -150Repository: 6529-Collections/6529seize-frontend
Length of output: 5823
🏁 Script executed:
cat -n components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx | tail -100Repository: 6529-Collections/6529seize-frontend
Length of output: 4274
🏁 Script executed:
# Check if focus/blur events are used elsewhere in the codebase with react-tooltip
rg "Tooltip" --type tsx --type ts -A 5 | grep -E "(focus|blur)" | head -20Repository: 6529-Collections/6529seize-frontend
Length of output: 106
🏁 Script executed:
# Search for react-tooltip documentation or comments about focus/blur
rg "focus|blur" --type tsx --type ts | grep -i tooltipRepository: 6529-Collections/6529seize-frontend
Length of output: 106
🌐 Web query:
react-tooltip focus blur keyboard events openEvents closeEvents
💡 Result:
In react-tooltip v5, the old events={['hover'|'click']} API is deprecated; use:
openEvents/closeEventsfor anchor element events (including focus/blur)globalCloseEventsfor keyboard/global close behaviors like Escape (react-tooltip.com)
Focus / blur (keyboard tabbing)
import { Tooltip } from 'react-tooltip';
<button data-tooltip-id="t1">Tab to me</button>
<Tooltip
id="t1"
content="Opens on focus, closes on blur"
openEvents={{ focus: true }}
closeEvents={{ blur: true }}
/>openEvents supports: mouseover, focus, mouseenter, click, dblclick, mousedown and closeEvents supports: mouseout, blur, mouseleave, click, dblclick, mouseup. (react-tooltip.com)
Keyboard “Escape to close”
<Tooltip
id="t1"
content="Press Esc to close"
globalCloseEvents={{ escape: true }}
/>globalCloseEvents includes escape, scroll, resize, clickOutsideAnchor. (react-tooltip.com)
Notes on “keyboard open”
There isn’t a dedicated “keyDown to open” option—tooltips typically open via focus (Tab) or via click (which can be triggered by Enter/Space on a button, depending on the element/browser). Configure those via openEvents={{ focus: true }} or openEvents={{ click: true }}. (react-tooltip.com)
Citations:
- 1: https://react-tooltip.com/docs/options?utm_source=openai
- 2: https://react-tooltip.com/docs/options?utm_source=openai
- 3: https://react-tooltip.com/docs/options?utm_source=openai
- 4: https://react-tooltip.com/docs/options?utm_source=openai
Add focus/blur keyboard event handlers to the hover-enabled branch for tooltip accessibility.
When isHoverEnabled is true, lines 133-134 only configure mouse events (mouseenter/mouseleave), preventing keyboard users who Tab to the button from opening the tooltip. According to react-tooltip v5 documentation, openEvents and closeEvents support keyboard events; add focus: true to openEvents and blur: true to closeEvents in the hover-enabled branch to ensure keyboard accessibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/waves/small-leaderboard/WaveSmallLeaderboardItemOutcomes.tsx`
around lines 133 - 135, When isHoverEnabled is true the tooltip only binds
mouseenter/mouseleave so keyboard users can't open it; update the hover-enabled
branch where openEvents, closeEvents, and globalCloseEvents are set (the
expressions using isHoverEnabled) to include keyboard handlers: add focus: true
to openEvents and blur: true to closeEvents while leaving globalCloseEvents
as-is for that branch so Tab/focus opens the tooltip and blur closes it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
maxTouchPoints,ontouchstart, ad-hocmatchMediachecks) and consolidate logic in one SSR-safe client module.Description
src/interaction/useInteractionMode.tswhich computes capabilities via media queries and trackslastPointerTypeglobally usingpointerdown/pointermove, and exposesenableHoverUI,enableLongPress,lastPointerTypeand raw fields (canHover,hasFinePointer,hoverNone).useHasTouchInput,useIsTouchDevice,navigator.maxTouchPoints,ontouchstart, and ad-hocmatchMedialogic across multiple components to consume the new hook instead (notable files updated include drops/waves, sidebars, header search, profile activity copy, and a number of UI helpers).hooks/useHasTouchInput.tsandhooks/useIsTouchDevice.tsas thin deprecated wrappers that delegate touseInteractionMode()to preserve backward compatibility while preventing direct low-level logic usage.enableHoverUIandenableLongPress, and ensured long-press gating follows the new rules (enable whenhover: noneor last pointer wastouch).Testing
maxTouchPoints/ontouchstart/matchMedia("(hover|"(pointer)remain outside the new module; scan validated the centralization. (succeeded)npm run typecheck/npx tsc) but environment lacked some type-definition dependencies (@testing-library/jest-dom,nodetypes), preventing a complete typecheck (failed due to missing deps).npx eslint,npx prettier) but local environment resolution for tooling/config made them fail in this environment (failed due to missing local dev-tool resolution).Codex Task
Summary by CodeRabbit