From 7ea810771d8404f3bbdc1da198888cf2dfd08de7 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 11 Mar 2026 09:06:37 +1100 Subject: [PATCH] fix: polish mobile lightbox swipe playback --- components/images/image-lightbox.test.tsx | 84 ++++++++++++++++++++++- components/images/image-lightbox.tsx | 41 +++++++---- components/studio/layout/studio-shell.tsx | 1 + hooks/use-media-player.test.ts | 80 ++++++++++++++++++++- hooks/use-media-player.ts | 62 ++++++++++++++--- 5 files changed, 242 insertions(+), 26 deletions(-) diff --git a/components/images/image-lightbox.test.tsx b/components/images/image-lightbox.test.tsx index c23653f..0ded364 100644 --- a/components/images/image-lightbox.test.tsx +++ b/components/images/image-lightbox.test.tsx @@ -176,12 +176,14 @@ vi.mock("@/components/ui/media-player", () => ({ url, alt, contentType, + controls = true, onLoadedMetadata, onLoad, }: { url: string; alt: string; contentType?: string; + controls?: boolean; onLoadedMetadata?: ReactEventHandler; onLoad?: ReactEventHandler; }) => { @@ -193,6 +195,8 @@ vi.mock("@/components/ui/media-player", () => ({ src={url} aria-label={alt} data-testid="video-player" + data-controls={controls ? "true" : "false"} + controls={controls} onLoadedMetadata={onLoadedMetadata} /> ); @@ -428,6 +432,17 @@ describe("ImageLightbox - Prompt Library Integration", () => { expect(onClose).toHaveBeenCalled(); }); + it("closes when the lightbox whitespace is clicked", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + + render(); + + await user.click(screen.getByTestId("lightbox-surface")); + + expect(onClose).toHaveBeenCalledTimes(1); + }); + it("passes onClose as onInsertComplete to PromptLibrary", async () => { const user = userEvent.setup(); const onClose = vi.fn(); @@ -869,6 +884,44 @@ describe("ImageLightbox - Mobile Swipe Navigation", () => { }); }); + it("accepts swipe gestures from the outer lightbox area", () => { + const onNext = vi.fn(); + + render( + , + ); + + const swipeRegion = screen.getByTestId("lightbox-swipe-region"); + fireEvent.pointerDown(swipeRegion, { + pointerId: 1, + pointerType: "touch", + isPrimary: true, + clientX: 24, + clientY: 120, + }); + fireEvent.pointerUp(swipeRegion, { + pointerId: 1, + pointerType: "touch", + isPrimary: true, + clientX: 20, + clientY: 0, + }); + + return waitFor(() => { + expect(onNext).toHaveBeenCalledTimes(1); + }); + }); + it("navigates to the previous media on downward touch swipe", () => { const onPrevious = vi.fn(); @@ -907,6 +960,35 @@ describe("ImageLightbox - Mobile Swipe Navigation", () => { }); }); + it("hides native video controls during mobile swipe navigation", () => { + const mockVideo = { + url: "https://example.com/test-video.mp4", + prompt: "A beautiful video", + model: "veo", + contentType: "video/mp4", + }; + + render( + , + ); + + expect(screen.getByTestId("video-player")).toHaveAttribute( + "data-controls", + "false", + ); + }); + it("drags the media with the finger before release", () => { render( { onClose={vi.fn()} mediaNavigation={{ hasNext: false, - hasPrevious: false, + hasPrevious: true, onNext: vi.fn(), onPrevious: vi.fn(), }} diff --git a/components/images/image-lightbox.tsx b/components/images/image-lightbox.tsx index 86dfe40..c16147f 100644 --- a/components/images/image-lightbox.tsx +++ b/components/images/image-lightbox.tsx @@ -51,6 +51,7 @@ interface ImageLightboxProps { mediaNavigation?: { hasNext: boolean; hasPrevious: boolean; + hideVideoControls?: boolean; onNext: () => void; onPrevious: () => void; }; @@ -269,6 +270,15 @@ export function ImageLightbox({ handleCloseAttempt(); }, [handleCloseAttempt]); + const handleLightboxSurfaceClick = React.useCallback( + (event: React.MouseEvent) => { + if (event.target === event.currentTarget) { + handleBackdropCloseAttempt(); + } + }, + [handleBackdropCloseAttempt], + ); + // ========================================== // Pane action handlers (used in compare mode) // ========================================== @@ -448,9 +458,17 @@ export function ImageLightbox({ ...swipeGestureHandlers } = swipeNavigationHandlers; const isSwipeInteractionActive = isSwipeDragging || isSwipeAnimating; + const showMobileSwipeVideoControls = mediaNavigation?.hideVideoControls !== true; const lightboxBackdropStyle = isSwipeNavigationEnabled ? swipeOverlayStyle : { backgroundColor: "rgba(0, 0, 0, 0.8)" }; + const lightboxSwipeRegionProps = isSwipeNavigationEnabled + ? { + "data-testid": "lightbox-swipe-region", + style: { touchAction: swipeTouchAction as React.CSSProperties["touchAction"] }, + ...swipeGestureHandlers, + } + : {}; return ( <> @@ -472,14 +490,14 @@ export function ImageLightbox({ className="w-full h-full backdrop-blur-md cursor-default flex items-center justify-center animate-in fade-in duration-150" style={lightboxBackdropStyle} > -
+
{isVideo ? ( -
+
) : ( -
+
0, + hideVideoControls: true, onNext: handleNextLightboxMedia, onPrevious: handlePreviousLightboxMedia, }; diff --git a/hooks/use-media-player.test.ts b/hooks/use-media-player.test.ts index 5f0d6cc..72e01f0 100644 --- a/hooks/use-media-player.test.ts +++ b/hooks/use-media-player.test.ts @@ -87,7 +87,7 @@ describe("useMediaPlayer", () => { describe("handleError", () => { it("sets hasError to true on error", () => { const { result } = renderHook(() => - useMediaPlayer({ url: "https://example.com/video.mp4", isVideo: true }) + useMediaPlayer({ url: "https://example.com/image.jpg", isVideo: false }) ) expect(result.current.hasError).toBe(false) @@ -104,8 +104,8 @@ describe("useMediaPlayer", () => { const onError = vi.fn() const { result } = renderHook(() => useMediaPlayer({ - url: "https://example.com/video.mp4", - isVideo: true, + url: "https://example.com/image.jpg", + isVideo: false, onError, }) ) @@ -166,6 +166,80 @@ describe("useMediaPlayer", () => { expect(result.current.hasError).toBe(false) expect(onError).not.toHaveBeenCalled() }) + + it("does not surface transient video errors during source swaps", async () => { + vi.useFakeTimers() + + const onError = vi.fn() + const { result } = renderHook(() => + useMediaPlayer({ + url: "https://example.com/video.mp4", + isVideo: true, + onError, + }) + ) + + const mockVideo = { + src: "https://example.com/video.mp4", + currentSrc: "https://example.com/video.mp4", + error: null, + networkState: HTMLMediaElement.NETWORK_LOADING, + } as unknown as HTMLVideoElement + ;(result.current.videoRef as React.MutableRefObject).current = mockVideo + + act(() => { + result.current.handleError({ + currentTarget: mockVideo, + } as unknown as React.SyntheticEvent) + }) + + expect(result.current.hasError).toBe(false) + + await act(async () => { + vi.advanceTimersByTime(500) + }) + + expect(result.current.hasError).toBe(false) + expect(onError).not.toHaveBeenCalled() + vi.useRealTimers() + }) + + it("surfaces persistent video errors after a grace period", async () => { + vi.useFakeTimers() + + const onError = vi.fn() + const { result } = renderHook(() => + useMediaPlayer({ + url: "https://example.com/video.mp4", + isVideo: true, + onError, + }) + ) + + const mockVideo = { + src: "https://example.com/video.mp4", + currentSrc: "https://example.com/video.mp4", + error: { code: 4 } as MediaError, + networkState: HTMLMediaElement.NETWORK_NO_SOURCE, + } as unknown as HTMLVideoElement + ;(result.current.videoRef as React.MutableRefObject).current = mockVideo + + act(() => { + result.current.handleError({ + currentTarget: mockVideo, + } as unknown as React.SyntheticEvent) + }) + + expect(result.current.hasError).toBe(false) + + await act(async () => { + vi.advanceTimersByTime(500) + }) + + expect(result.current.hasError).toBe(true) + expect(onError).toHaveBeenCalledTimes(1) + vi.useRealTimers() + }) }) describe("handleVideoLoadedData", () => { diff --git a/hooks/use-media-player.ts b/hooks/use-media-player.ts index 272fd05..69ce3a5 100644 --- a/hooks/use-media-player.ts +++ b/hooks/use-media-player.ts @@ -35,7 +35,7 @@ export interface UseMediaPlayerReturn { /** Handler for media load events */ handleLoad: (e: React.SyntheticEvent) => void /** Handler for media error events */ - handleError: () => void + handleError: (e?: React.SyntheticEvent) => void /** Handler for video metadata availability */ handleVideoLoadedMetadata: (e: React.SyntheticEvent) => void /** Handler for video-specific loadeddata event */ @@ -92,6 +92,7 @@ export function useMediaPlayer({ const autoplayAttemptVersionRef = React.useRef(0) const hasDispatchedLoadRef = React.useRef(false) const retryTimeoutRef = React.useRef(null) + const errorTimeoutRef = React.useRef(null) currentUrlRef.current = url @@ -102,6 +103,13 @@ export function useMediaPlayer({ } }, []) + const clearErrorTimeout = React.useCallback(() => { + if (errorTimeoutRef.current !== null) { + window.clearTimeout(errorTimeoutRef.current) + errorTimeoutRef.current = null + } + }, []) + const isCurrentMediaElement = React.useCallback((element: HTMLImageElement | HTMLVideoElement | null) => { const src = getMediaSource(element) if (!src) return true @@ -116,13 +124,15 @@ export function useMediaPlayer({ hasDispatchedLoadRef.current = false autoplayAttemptVersionRef.current += 1 clearRetryTimeout() - }, [clearRetryTimeout, url]) + clearErrorTimeout() + }, [clearErrorTimeout, clearRetryTimeout, url]) React.useEffect(() => { return () => { clearRetryTimeout() + clearErrorTimeout() } - }, [clearRetryTimeout]) + }, [clearErrorTimeout, clearRetryTimeout]) const dispatchLoad = React.useCallback((e: React.SyntheticEvent) => { const element = e.currentTarget as HTMLImageElement | HTMLVideoElement | null @@ -130,29 +140,65 @@ export function useMediaPlayer({ return } + clearErrorTimeout() + if (hasError) { + setHasError(false) + } setIsLoading(false) if (!hasDispatchedLoadRef.current) { hasDispatchedLoadRef.current = true onLoad?.(e) } - }, [isCurrentMediaElement, onLoad]) + }, [clearErrorTimeout, hasError, isCurrentMediaElement, onLoad]) const handleLoad = React.useCallback((e: React.SyntheticEvent) => { dispatchLoad(e) }, [dispatchLoad]) - const handleError = React.useCallback(() => { - const video = videoRef.current - if (video && !isCurrentMediaElement(video)) { + const handleError = React.useCallback((e?: React.SyntheticEvent) => { + const element = (e?.currentTarget as HTMLImageElement | HTMLVideoElement | undefined) ?? videoRef.current + + if (element && !isCurrentMediaElement(element)) { return } clearRetryTimeout() + + if (isVideo) { + const video = (element instanceof HTMLVideoElement ? element : videoRef.current) + if (!video) { + return + } + + clearErrorTimeout() + errorTimeoutRef.current = window.setTimeout(() => { + errorTimeoutRef.current = null + + if (!isCurrentMediaElement(video)) { + return + } + + const hasPersistentVideoError = + video.error !== null || + video.networkState === HTMLMediaElement.NETWORK_NO_SOURCE + + if (!hasPersistentVideoError) { + return + } + + setIsLoading(false) + setHasError(true) + onError?.() + }, 450) + return + } + + clearErrorTimeout() setIsLoading(false) setHasError(true) onError?.() - }, [clearRetryTimeout, isCurrentMediaElement, onError]) + }, [clearErrorTimeout, clearRetryTimeout, isCurrentMediaElement, isVideo, onError]) const handleVideoLoadedMetadata = React.useCallback((e: React.SyntheticEvent) => { dispatchLoad(e)