Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
📝 WalkthroughWalkthroughThis pull request consolidates touch device detection into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/memes/drops/MemesLeaderboardDropCard.tsx (1)
1-38: Add"use client"directive—this component uses theuseDeviceInfohook.In Next.js App Router, only Client Components can call React hooks. Since
useDeviceInfois a custom hook from a Client Component andMemesLeaderboardDropCardcalls it on line 33, the component must be marked as a client component. Without this directive, the build will fail.Proposed fix
+ "use client"; + import type { ReactNode } from "react"; import React from "react"; import type { ExtendedDrop } from "@/helpers/waves/drop.helpers"; import useDeviceInfo from "@/hooks/useDeviceInfo";__tests__/components/utils/tooltip/UserProfileTooltipWrapper.test.tsx (1)
75-88: Fix thedoMockmodule id to match the component import. The component imports@/hooks/useDeviceInfo(line 4 of UserProfileTooltipWrapper.tsx), and while the relative path../../../../hooks/useDeviceInforesolves to the same physical file, Jest matches modules by their identifiers. Since the earlier mock on line 38 uses the alias path, thisdoMockmust also use@/hooks/useDeviceInfoto properly override it.🧩 Proposed fix
- jest.doMock('../../../../hooks/useDeviceInfo', () => ({ + jest.doMock('@/hooks/useDeviceInfo', () => ({ __esModule: true, default: mockUseDeviceInfo, }));__tests__/components/brain/left-sidebar/waves/UnifiedWavesList.test.tsx (1)
73-79: Remove the duplicateshouldUseTouchUIkey.The object literal defines
shouldUseTouchUItwice, which is ignored/overwritten and can fail lint. Keep a single entry.🧹 Proposed fix
useDeviceInfoMock.mockReturnValue({ isApp: true, isMobileDevice: false, hasTouchScreen: false, shouldUseTouchUI: false, - shouldUseTouchUI: false, isAppleMobile: false, } as DeviceInfo);components/waves/drops/WaveDropContent.tsx (1)
1-7: Add"use client"to enable client hooks in this App Router component.This module calls
useDeviceInfo()(a client hook), but lacks a"use client"directive. In Next.js App Router, client hook directives are not inherited through imports—each file using a client hook must declare it explicitly. Without this directive, the build will fail.Proposed fix
+ "use client"; + import React from "react"; import type { ApiDrop } from "@/generated/models/ApiDrop"; import WaveDropPart from "./WaveDropPart";
🤖 Fix all issues with AI agents
In `@components/waves/drops/WaveDrop.tsx`:
- Around line 176-179: Replace the mixed detection logic that computes hasTouch
using useIsMobileDevice() and shouldUseTouchUI with a single source of truth
from useDeviceInfo(): remove useIsMobileDevice() and derive hasTouch from the
hasTouchScreen property returned by useDeviceInfo (i.e., const { hasTouchScreen
} = useDeviceInfo(); const hasTouch = hasTouchScreen;), leaving
useCompactMode()/compact unchanged; update any references to shouldUseTouchUI or
isMobile in WaveDrop.tsx to use hasTouchScreen/hasTouch instead.
🧹 Nitpick comments (9)
components/waves/drops/WaveDropMobileMenu.tsx (2)
71-91: MemoizeextractTextFromDropresult to avoid repeated computation.This function is called directly in the render (line 298) on every render cycle. Since it processes
drop.partswith multiple regex operations, consider memoizing the result.♻️ Suggested refactor
- const extractTextFromDrop = (): string => { + const extractedText = useMemo(() => { if (!drop.parts || drop.parts.length === 0) { return ""; } const textParts = drop.parts .map((part) => { if (!part.content) return ""; let text = part.content; text = text.replace(/\[([^\]]+)\]\([^\)]+\)/g, "$1"); text = text.replace(/\*\*([^\*]+)\*\*/g, "$1"); text = text.replace(/\*([^\*]+)\*/g, "$1"); text = text.replace(/`([^`]+)`/g, "$1"); text = text.replace(/#{1,6}\s+/g, ""); text = text.replace(/\n{3,}/g, "\n\n"); return text.trim(); }) .filter((text) => text.length > 0); return textParts.join("\n\n"); - }; + }, [drop.parts]);Then update usages:
- Line 97-98:
const text = extractedText; if (!text) return;- Line 298:
{extractedText && (
100-104: Add error handling for clipboard write failure.The promise rejection from
writeText()is not handled. If clipboard access is denied or fails, the user gets no feedback.♻️ Suggested fix
if (navigator?.clipboard?.writeText) { - navigator.clipboard.writeText(text).then(() => { - setCopiedText(true); - setTimeout(() => setCopiedText(false), 2000); - }); + navigator.clipboard.writeText(text) + .then(() => { + setCopiedText(true); + setTimeout(() => setCopiedText(false), 2000); + }) + .catch(() => { + // Fallback on failure + fallbackCopyToClipboard(text); + }); }Note: The existing
copyToClipboardfunction has the same pattern. Consider addressing both together if you refactor.components/waves/drops/participation/ParticipationDropContent.tsx (1)
27-28: Consider removing the redundant intermediate variable.The assignment
const hasTouch = shouldUseTouchUI;creates an unnecessary alias. You could either destructure with a rename or useshouldUseTouchUIdirectly.♻️ Suggested simplification
- const { shouldUseTouchUI } = useDeviceInfo(); - const hasTouch = shouldUseTouchUI; + const { shouldUseTouchUI: hasTouch } = useDeviceInfo();components/waves/drops/ArtistSubmissionBadge.tsx (2)
39-40: Redundant condition – inconsistent with other components.The
!isMobile && !shouldUseTouchUIcheck appears redundant.shouldUseTouchUIshould already account for mobile scenarios. Other components in this PR (e.g.,WebSidebarNavItem) use only!shouldUseTouchUIfor the same tooltip-hiding logic.Consider simplifying for consistency:
♻️ Suggested simplification
- const isMobile = useIsMobileDevice(); const { shouldUseTouchUI } = useDeviceInfo(); const id = useId(); const uniqueTooltipId = `${tooltipId}-${id}`; const [isTooltipOpen, setIsTooltipOpen] = React.useState(false); if (submissionCount === 0) return null; const dataTooltipId = - !isMobile && !shouldUseTouchUI ? uniqueTooltipId : undefined; + !shouldUseTouchUI ? uniqueTooltipId : undefined;And similarly on line 77:
- {!isMobile && !shouldUseTouchUI && ( + {!shouldUseTouchUI && (If there's a specific edge case requiring both checks, please document it with a comment.
7-8: Remove unused import ifisMobilecheck is eliminated.If you simplify the tooltip conditions to use only
shouldUseTouchUI, theuseIsMobileDeviceimport on line 7 can be removed.__tests__/components/waves/drops/WaveDropsAll.test.tsx (1)
38-46: Consider addingshouldUseTouchUIto the base mock for consistency.The base mock at lines 38-46 doesn't include
shouldUseTouchUI, whilesetupMocks()(line 262) does provide it. SincebeforeEachcallssetupMocks(), this works in practice, but adding the field to the base mock would improve consistency and prevent subtle issues if tests are added that don't usesetupMocks().Suggested fix
jest.mock('@/hooks/useDeviceInfo', () => ({ __esModule: true, default: jest.fn(() => ({ isAppleMobile: false, isMobileDevice: false, hasTouchScreen: false, + shouldUseTouchUI: false, isApp: false })) }));__tests__/components/waves/drops/WaveDropReactions.test.tsx (1)
347-382: Consider removing the redundant mock reset at line 381.The mock reset at line 381 is unnecessary since
beforeEachalready callsjest.clearAllMocks()and the base mock defaults toshouldUseTouchUI: false. This cleanup is harmless but adds noise.Suggested cleanup
const reactionButton = screen.getAllByRole("button")[0]; expect(reactionButton).toBeInTheDocument(); - - mockUseDeviceInfo.mockReturnValue({ shouldUseTouchUI: false }); });components/waves/drops/participation/OngoingParticipationDrop.tsx (1)
43-45: Consolidate device hooks for better detection logic.The component uses both
useIsMobileDevice()anduseDeviceInfo(), butuseDeviceInfoalready providesisMobileDevicewith more comprehensive detection (including modern userAgentData API and Capacitor app handling). Consolidate them into a single hook call:Suggested consolidation
- const isMobile = useIsMobileDevice(); - const { shouldUseTouchUI } = useDeviceInfo(); + const { shouldUseTouchUI, isMobileDevice: isMobile } = useDeviceInfo();This eliminates the redundant hook and improves detection accuracy by using the more sophisticated device identification logic.
components/waves/drops/winner/DefaultWinnerDrop.tsx (1)
6-8: Consolidate mobile/touch detection into a single hook.You’re now calling two device hooks, which can diverge.
useDeviceInfoalready exposesisMobileDevice, so you can avoid duplicate detection and keep a single source of truth.♻️ Suggested refactor
-import useIsMobileDevice from "@/hooks/isMobileDevice"; import useDeviceInfo from "@/hooks/useDeviceInfo"; @@ - const isMobile = useIsMobileDevice(); - const { shouldUseTouchUI } = useDeviceInfo(); - const hasTouch = shouldUseTouchUI || isMobile; + const { shouldUseTouchUI, isMobileDevice } = useDeviceInfo(); + const hasTouch = shouldUseTouchUI || isMobileDevice; @@ - {!isMobile && showReplyAndQuote && ( + {!isMobileDevice && showReplyAndQuote && (Also applies to: 108-110, 237-237
| const isMobile = useIsMobileDevice(); | ||
| const hasTouch = useIsTouchDevice() || isMobile; | ||
| const { shouldUseTouchUI } = useDeviceInfo(); | ||
| const hasTouch = shouldUseTouchUI || isMobile; | ||
| const compact = useCompactMode(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the hooks and their implementations
find . -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "useIsMobileDevice\|useDeviceInfo" | head -20Repository: 6529-Collections/6529seize-frontend
Length of output: 1628
🏁 Script executed:
# Search for hook definitions
rg "export.*useIsMobileDevice|export.*useDeviceInfo" -t tsx -t tsRepository: 6529-Collections/6529seize-frontend
Length of output: 106
🏁 Script executed:
# Also check for the file structure
fd -t f -e ts -e tsx | grep -i "hook\|device" | head -20Repository: 6529-Collections/6529seize-frontend
Length of output: 625
🏁 Script executed:
# Find the exact hook implementations
rg "useIsMobileDevice|useDeviceInfo" --type-list | head -5Repository: 6529-Collections/6529seize-frontend
Length of output: 468
🏁 Script executed:
# Search more broadly for these hooks
rg "useIsMobileDevice|useDeviceInfo" -g "*.ts" -g "*.tsx" -lRepository: 6529-Collections/6529seize-frontend
Length of output: 7722
🏁 Script executed:
# Check the WaveDrop.tsx file to understand context
cat -n components/waves/drops/WaveDrop.tsx | sed -n '170,190p'Repository: 6529-Collections/6529seize-frontend
Length of output: 931
🏁 Script executed:
# Examine the useDeviceInfo implementation
cat -n hooks/useDeviceInfo.tsRepository: 6529-Collections/6529seize-frontend
Length of output: 5107
🏁 Script executed:
# Examine the isMobileDevice implementation
cat -n hooks/isMobileDevice.tsRepository: 6529-Collections/6529seize-frontend
Length of output: 674
🏁 Script executed:
# Check how hasTouch is used in WaveDrop.tsx
rg "hasTouch" components/waves/drops/WaveDrop.tsx -B 2 -A 2Repository: 6529-Collections/6529seize-frontend
Length of output: 1330
🏁 Script executed:
# Check if there are any viewport/media query listeners in useIsMobileDevice or if it updates on resize
rg "matchMedia|resize|viewport" hooks/isMobileDevice.tsRepository: 6529-Collections/6529seize-frontend
Length of output: 61
🏁 Script executed:
# Verify how hasTouch is actually used - check for touch event listeners
rg "onTouchStart|onTouchEnd|@media|matchMedia" components/waves/drops/WaveDrop.tsx -A 2Repository: 6529-Collections/6529seize-frontend
Length of output: 200
🏁 Script executed:
# Check if there are tests that clarify the intended behavior
cat -n __tests__/hooks/useIsMobileDevice.test.tsRepository: 6529-Collections/6529seize-frontend
Length of output: 1093
🏁 Script executed:
# Double-check if useIsMobileDevice is updated on any browser events
grep -r "useIsMobileDevice" hooks/ -A 15 -B 2Repository: 6529-Collections/6529seize-frontend
Length of output: 917
🏁 Script executed:
# Verify the actual touch event handlers and their guards
cat -n components/waves/drops/WaveDrop.tsx | sed -n '195,225p'Repository: 6529-Collections/6529seize-frontend
Length of output: 1257
🏁 Script executed:
# Check if useIsMobileDevice updates on window resize or any other event
rg "isMobile|setIsMobile" hooks/isMobileDevice.tsRepository: 6529-Collections/6529seize-frontend
Length of output: 173
🏁 Script executed:
# Verify the actual full implementation of useDeviceInfo's shouldUseTouchUI logic more carefully
cat -n hooks/useDeviceInfo.ts | sed -n '50,70p'Repository: 6529-Collections/6529seize-frontend
Length of output: 1106
🏁 Script executed:
# Check if there are any other references to useIsMobileDevice to see if it's used elsewhere for touch detection
rg "hasTouch|useIsMobileDevice.*touch|isMobile.*touch" components/ -B 2 -A 2 | head -50Repository: 6529-Collections/6529seize-frontend
Length of output: 3644
🏁 Script executed:
# Check the useDeviceInfo test to see if it clarifies the intended behavior
cat -n __tests__/hooks/useDeviceInfo.test.ts | head -100Repository: 6529-Collections/6529seize-frontend
Length of output: 5151
🏁 Script executed:
# Check if there are other usages of hasTouch = shouldUseTouchUI || isMobile pattern
rg "hasTouch.*shouldUseTouchUI.*isMobile|shouldUseTouchUI.*isMobile" -B 2 -A 2Repository: 6529-Collections/6529seize-frontend
Length of output: 8932
🏁 Script executed:
# Look at similar components that use touch detection to see the pattern
cat -n components/waves/winners/drops/DefaultWaveWinnerDrop.tsx | sed -n '1,50p'Repository: 6529-Collections/6529seize-frontend
Length of output: 3135
Use hasTouchScreen instead of mixing isMobile with shouldUseTouchUI.
The useIsMobileDevice hook detects only the user agent and doesn't respond to viewport changes. However, combining it with shouldUseTouchUI mixes two different detection strategies. For consistency and accuracy, derive hasTouch solely from useDeviceInfo using hasTouchScreen, which is based on actual touch capability (maxTouchPoints + runtime touch event detection) rather than user agent alone.
Suggested change
- const isMobile = useIsMobileDevice();
- const { shouldUseTouchUI } = useDeviceInfo();
- const hasTouch = shouldUseTouchUI || isMobile;
+ const { hasTouchScreen } = useDeviceInfo();
+ const hasTouch = hasTouchScreen;📝 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 isMobile = useIsMobileDevice(); | |
| const hasTouch = useIsTouchDevice() || isMobile; | |
| const { shouldUseTouchUI } = useDeviceInfo(); | |
| const hasTouch = shouldUseTouchUI || isMobile; | |
| const compact = useCompactMode(); | |
| const { hasTouchScreen } = useDeviceInfo(); | |
| const hasTouch = hasTouchScreen; | |
| const compact = useCompactMode(); |
🤖 Prompt for AI Agents
In `@components/waves/drops/WaveDrop.tsx` around lines 176 - 179, Replace the
mixed detection logic that computes hasTouch using useIsMobileDevice() and
shouldUseTouchUI with a single source of truth from useDeviceInfo(): remove
useIsMobileDevice() and derive hasTouch from the hasTouchScreen property
returned by useDeviceInfo (i.e., const { hasTouchScreen } = useDeviceInfo();
const hasTouch = hasTouchScreen;), leaving useCompactMode()/compact unchanged;
update any references to shouldUseTouchUI or isMobile in WaveDrop.tsx to use
hasTouchScreen/hasTouch instead.
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 0
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)
45-72:shouldUseTouchUIignoresuserAgentData.mobile, creating inconsistency withisMobileDeviceon UA-reduced browsers.On modern browsers that reduce the UA string, the classic UA regex may fail to detect mobile devices while
userAgentData.mobilecorrectly identifies them. This causesshouldUseTouchUIto be false whileisMobileDeviceis true—an inconsistency that impacts touch UI logic. Consolidate mobile detection to useuserAgentData.mobileas the primary source, consistent with howisMobileDeviceis calculated.🔧 Suggested fix
- let shouldUseTouchUI = false; - if (!(hasFinePointer || hasHover) && maxTouchPoints > 0) { - const ua = nav.userAgent ?? ""; - const isMobileUA = /Android|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(ua); - shouldUseTouchUI = isMobileUA; - } - - const ua = nav.userAgent; - const uaDataMobile = nav.userAgentData?.mobile; + const ua = nav.userAgent ?? ""; + const uaDataMobile = nav.userAgentData?.mobile; + const isMobileUA = + uaDataMobile ?? + /Android|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(ua); + + let shouldUseTouchUI = false; + if (!(hasFinePointer || hasHover) && maxTouchPoints > 0) { + shouldUseTouchUI = isMobileUA; + }
Signed-off-by: prxt6529 <prxt@6529.io>
|
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.