Conversation
📝 WalkthroughWalkthroughTouch-device detection is unified across the wave drop component hierarchy by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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 |
|
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)
components/waves/drops/participation/ParticipationDropContent.tsx (1)
4-41: Add an optionalhasTouchprop to avoid a TS prop mismatch.
OngoingParticipationDropnow passeshasTouch, butParticipationDropContentPropsdoesn’t accept it, which will fail type-checking and ignore the parent-derived value. Accept it and preferhasTouch ?? useIsTouchDevice().🐛 Proposed fix
interface ParticipationDropContentProps { readonly drop: ExtendedDrop; readonly activePartIndex: number; readonly setActivePartIndex: (index: number) => void; readonly onLongPress: () => void; readonly onDropContentClick?: ((drop: ExtendedDrop) => void) | undefined; readonly onQuoteClick: (drop: ApiDrop) => void; readonly setLongPressTriggered: (triggered: boolean) => void; readonly isCompetitionDrop?: boolean | undefined; + readonly hasTouch?: boolean | undefined; } export default function ParticipationDropContent({ drop, activePartIndex, setActivePartIndex, onLongPress, onDropContentClick, onQuoteClick, setLongPressTriggered, isCompetitionDrop = false, + hasTouch, }: ParticipationDropContentProps) { - const hasTouch = useIsTouchDevice(); + const isTouchDevice = useIsTouchDevice(); + const effectiveHasTouch = hasTouch ?? isTouchDevice; return ( <div className="tw-mt-1"> <WaveDropContent drop={drop} activePartIndex={activePartIndex} setActivePartIndex={setActivePartIndex} onLongPress={onLongPress} onDropContentClick={onDropContentClick} onQuoteClick={onQuoteClick} setLongPressTriggered={setLongPressTriggered} isCompetitionDrop={isCompetitionDrop} - hasTouch={hasTouch} + hasTouch={effectiveHasTouch} /> </div> ); }
🧹 Nitpick comments (1)
__tests__/components/waves/drops/WaveDropPart.test.tsx (1)
82-84: Consider adding test coverage forhasTouch: false.The default props now include
hasTouch: true, which is appropriate for testing touch behaviors. However, there's no test coverage verifying that long press handling is disabled whenhasTouchisfalse. This would ensure the component correctly gates touch-specific behaviors on non-touch devices.Example test to add
it("does not trigger long press when hasTouch is false", () => { render(<WaveDropPart {...defaultProps} hasTouch={false} />); const containers = screen.getAllByRole("button"); const container = containers.find((el) => el.className.includes("tw-cursor-pointer") )!; fireEvent.touchStart(container, { touches: [{ clientX: 100, clientY: 100 }], }); act(() => { jest.advanceTimersByTime(500); }); expect(mockSetLongPressTriggered).not.toHaveBeenCalledWith(true); expect(mockOnLongPress).not.toHaveBeenCalled(); });

Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.