From 34094961f2e4c85fb0b4d06cfcdd67288e51f03f Mon Sep 17 00:00:00 2001 From: Mason Fox Date: Thu, 16 Apr 2026 08:29:08 -0400 Subject: [PATCH] fix: properly merge PR #415 tap & hold feature via cherry-pick PR #415 was marked as merged but commits never made it to develop branch. Cherry-picked the three feature commits to restore functionality: - Add enterSelectModeWithSelection to useBookListView - Integrate useLongPress hook in BookListItem - Wire up long-press handlers in DraggableBookList and shelves page - Add keyboard accessibility (Space/Enter keys) - Add comprehensive test coverage (22 tests) Note: hooks/useLongPress.ts already existed on develop from commit 9f2ca50 and matches the final PR version, so it was kept unchanged. Original commits: - b88e3fc feat: implement tap & hold to select shelf items - 990c8aa fix: implement @review feedback (iteration 1) - f623bb4 fix: add onTouchCancel handler and Enter key accessibility support Resolves #414 Fixes #415 Co-authored-by: OpenCode --- __tests__/hooks/useLongPress.test.ts | 636 +++++++++++++++++++++++++ app/shelves/[id]/page.tsx | 2 + components/Books/BookListItem.tsx | 27 ++ components/Books/DraggableBookList.tsx | 8 +- hooks/useBookListView.ts | 7 + 5 files changed, 679 insertions(+), 1 deletion(-) create mode 100644 __tests__/hooks/useLongPress.test.ts diff --git a/__tests__/hooks/useLongPress.test.ts b/__tests__/hooks/useLongPress.test.ts new file mode 100644 index 00000000..8a82408f --- /dev/null +++ b/__tests__/hooks/useLongPress.test.ts @@ -0,0 +1,636 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useLongPress } from "@/hooks/useLongPress"; + +describe("useLongPress", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + }); + + describe("Basic Functionality", () => { + it("should return event handlers", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress)); + + expect(result.current).toHaveProperty("onMouseDown"); + expect(result.current).toHaveProperty("onMouseUp"); + expect(result.current).toHaveProperty("onMouseLeave"); + expect(result.current).toHaveProperty("onMouseMove"); + expect(result.current).toHaveProperty("onTouchStart"); + expect(result.current).toHaveProperty("onTouchEnd"); + expect(result.current).toHaveProperty("onTouchCancel"); + expect(result.current).toHaveProperty("onTouchMove"); + }); + + it("should use default delay and tolerance", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress)); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(499); + }); + expect(onLongPress).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(1); + }); + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + + it("should accept custom delay and tolerance", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 1000, tolerance: 20 }) + ); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(999); + }); + expect(onLongPress).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(1); + }); + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + }); + + describe("Mouse Events", () => { + it("should trigger callback after delay on mouse down", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + + it("should only respond to left mouse button", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + // Right-click (button = 2) + const rightClickEvent = { + button: 2, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(rightClickEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should cancel on mouse up before delay", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + act(() => { + result.current.onMouseUp(); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should cancel on mouse leave", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + act(() => { + result.current.onMouseLeave(); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should cancel on movement beyond tolerance", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(startEvent); + }); + + // Move beyond tolerance (> 10px) + const moveEvent = { + clientX: 112, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseMove(moveEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should NOT cancel on movement within tolerance", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(startEvent); + }); + + // Move within tolerance (< 10px) + const moveEvent = { + clientX: 105, + clientY: 103, + } as React.MouseEvent; + + act(() => { + result.current.onMouseMove(moveEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + }); + + describe("Touch Events", () => { + it("should trigger callback after delay on touch start", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + touches: [{ clientX: 100, clientY: 100 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + + it("should ignore multi-touch events", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + touches: [ + { clientX: 100, clientY: 100 }, + { clientX: 200, clientY: 200 }, + ], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should cancel on touch end before delay", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + touches: [{ clientX: 100, clientY: 100 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + act(() => { + result.current.onTouchEnd(); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should cancel on touch cancel (browser-interrupted touch)", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + touches: [{ clientX: 100, clientY: 100 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + act(() => { + result.current.onTouchCancel(); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should cancel on touch move beyond tolerance", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + touches: [{ clientX: 100, clientY: 100 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(startEvent); + }); + + // Move beyond tolerance (> 10px) + const moveEvent = { + touches: [{ clientX: 100, clientY: 112 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchMove(moveEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should NOT cancel on touch move within tolerance", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + touches: [{ clientX: 100, clientY: 100 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(startEvent); + }); + + // Move within tolerance (< 10px) + const moveEvent = { + touches: [{ clientX: 105, clientY: 103 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchMove(moveEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + + it("should ignore touch move with multiple touches", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + touches: [{ clientX: 100, clientY: 100 }], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchStart(startEvent); + }); + + // Multi-touch move + const moveEvent = { + touches: [ + { clientX: 105, clientY: 103 }, + { clientX: 200, clientY: 200 }, + ], + } as unknown as React.TouchEvent; + + act(() => { + result.current.onTouchMove(moveEvent); + }); + + // Should still trigger because multi-touch move was ignored + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + }); + + describe("Memory Management", () => { + it("should clear timeout on unmount", () => { + const onLongPress = vi.fn(); + const { result, unmount } = renderHook(() => + useLongPress(onLongPress, { delay: 500 }) + ); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + // Unmount before timeout completes + unmount(); + + act(() => { + vi.advanceTimersByTime(250); + }); + + // Callback should NOT fire after unmount + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should clear existing timeout when starting new long-press", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const firstEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(firstEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + // Start a new long-press before first one completes + const secondEvent = { + button: 0, + clientX: 200, + clientY: 200, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(secondEvent); + }); + + act(() => { + vi.advanceTimersByTime(250); + }); + + // First timeout should be cleared, so callback not fired yet + expect(onLongPress).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(250); + }); + + // Second timeout completes (total 500ms since second start) + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + }); + + describe("Edge Cases", () => { + it("should handle rapid press and release", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + const mockEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + // Rapid press and release 10 times + for (let i = 0; i < 10; i++) { + act(() => { + result.current.onMouseDown(mockEvent); + }); + + act(() => { + vi.advanceTimersByTime(50); + }); + + act(() => { + result.current.onMouseUp(); + }); + } + + // No callbacks should fire + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should not fire if no position set on move", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => useLongPress(onLongPress, { delay: 500 })); + + // Call onMouseMove without calling onMouseDown first + const moveEvent = { + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseMove(moveEvent); + }); + + // Should not crash or cause issues + expect(onLongPress).not.toHaveBeenCalled(); + }); + + it("should handle movement exactly at tolerance boundary", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(startEvent); + }); + + // Move exactly 10px (at boundary) + const moveEvent = { + clientX: 110, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseMove(moveEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + // Should NOT cancel (tolerance is exclusive) + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + + it("should handle diagonal movement correctly", () => { + const onLongPress = vi.fn(); + const { result } = renderHook(() => + useLongPress(onLongPress, { delay: 500, tolerance: 10 }) + ); + + const startEvent = { + button: 0, + clientX: 100, + clientY: 100, + } as React.MouseEvent; + + act(() => { + result.current.onMouseDown(startEvent); + }); + + // Diagonal move: 8px X, 8px Y (within tolerance individually but not combined) + const moveEvent = { + clientX: 108, + clientY: 108, + } as React.MouseEvent; + + act(() => { + result.current.onMouseMove(moveEvent); + }); + + act(() => { + vi.advanceTimersByTime(500); + }); + + // Should still trigger (both deltas < 10) + expect(onLongPress).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/app/shelves/[id]/page.tsx b/app/shelves/[id]/page.tsx index 4619e93f..ec4b47f1 100644 --- a/app/shelves/[id]/page.tsx +++ b/app/shelves/[id]/page.tsx @@ -375,6 +375,7 @@ export default function ShelfDetailPage() { isSelectMode={listView.isSelectMode} selectedBookIds={listView.selectedBookIds} onToggleSelection={listView.toggleBookSelection} + onEnterSelectModeWithSelection={listView.enterSelectModeWithSelection} renderActions={!listView.isSelectMode ? (book, index) => ( listView.toggleBookSelection(book.id)} + onLongPress={() => listView.enterSelectModeWithSelection(book.id)} actions={ !listView.isSelectMode ? ( void; + onLongPress?: () => void; } export const BookListItem = memo(function BookListItem({ @@ -41,6 +43,7 @@ export const BookListItem = memo(function BookListItem({ isSelectMode = false, isSelected = false, onToggleSelection, + onLongPress, }: BookListItemProps) { const [imageError, setImageError] = useState(false); @@ -60,8 +63,30 @@ export const BookListItem = memo(function BookListItem({ } }; + const handleKeyDown = (e: React.KeyboardEvent) => { + // Allow Space and Enter keys to trigger long-press selection when not in select mode + // role="button" per ARIA spec requires both Space and Enter to activate + if ((e.key === ' ' || e.key === 'Enter') && !isSelectMode && onLongPress) { + e.preventDefault(); + onLongPress(); + } + }; + + // Long-press handlers (only active when NOT in select mode) + const longPressHandlers = useLongPress( + () => { + if (!isSelectMode && onLongPress) { + onLongPress(); + } + }, + { delay: 500, tolerance: 10 } + ); + return (
{/* Checkbox for select mode */} diff --git a/components/Books/DraggableBookList.tsx b/components/Books/DraggableBookList.tsx index e6059170..bf948dbb 100644 --- a/components/Books/DraggableBookList.tsx +++ b/components/Books/DraggableBookList.tsx @@ -46,6 +46,7 @@ interface DraggableBookListProps { isSelectMode?: boolean; selectedBookIds?: Set; onToggleSelection?: (bookId: number) => void; + onEnterSelectModeWithSelection?: (bookId: number) => void; } interface SortableBookItemProps { @@ -54,9 +55,10 @@ interface SortableBookItemProps { isSelectMode?: boolean; isSelected?: boolean; onToggleSelection?: () => void; + onLongPress?: () => void; } -function SortableBookItem({ book, actions, isSelectMode = false, isSelected = false, onToggleSelection }: SortableBookItemProps) { +function SortableBookItem({ book, actions, isSelectMode = false, isSelected = false, onToggleSelection, onLongPress }: SortableBookItemProps) { const { attributes, listeners, @@ -101,6 +103,7 @@ function SortableBookItem({ book, actions, isSelectMode = false, isSelected = fa isSelectMode={isSelectMode} isSelected={isSelected} onToggleSelection={onToggleSelection} + onLongPress={onLongPress} />
@@ -116,6 +119,7 @@ export function DraggableBookList({ isSelectMode = false, selectedBookIds = new Set(), onToggleSelection, + onEnterSelectModeWithSelection, }: DraggableBookListProps) { const [activeId, setActiveId] = useState(null); const [localBooks, setLocalBooks] = useState(books); @@ -190,6 +194,7 @@ export function DraggableBookList({ isSelectMode={isSelectMode} isSelected={selectedBookIds.has(book.id)} onToggleSelection={() => onToggleSelection?.(book.id)} + onLongPress={() => onEnterSelectModeWithSelection?.(book.id)} /> ))} @@ -214,6 +219,7 @@ export function DraggableBookList({ isSelectMode={isSelectMode} isSelected={selectedBookIds.has(book.id)} onToggleSelection={() => onToggleSelection?.(book.id)} + onLongPress={() => onEnterSelectModeWithSelection?.(book.id)} /> ))} diff --git a/hooks/useBookListView.ts b/hooks/useBookListView.ts index 0cdeca55..72d5e17c 100644 --- a/hooks/useBookListView.ts +++ b/hooks/useBookListView.ts @@ -94,6 +94,12 @@ export function useBookListView({ books, initialFilter = "" }: UseBookListViewOp setSelectedBookIds(new Set()); }, []); + // Enter select mode and select a specific book (for long-press to select) + const enterSelectModeWithSelection = useCallback((bookId: number) => { + setIsSelectMode(true); + setSelectedBookIds(new Set([bookId])); + }, []); + return { // Mobile detection isMobile, @@ -111,5 +117,6 @@ export function useBookListView({ books, initialFilter = "" }: UseBookListViewOp toggleSelectAll, clearSelection, exitSelectMode, + enterSelectModeWithSelection, }; }