feat(lightbox): enhance VideoSlide to handle video load errors #733
Conversation
…llback to ogImage
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds video error reporting and fallback behavior across the lightbox flow: VideoPlayer now accepts an onError prop; VideoSlide reports failed video loads to LightBox which tracks Changes
Sequence DiagramsequenceDiagram
participant Client
participant LightBox
participant VideoSlide
participant VideoPlayer
participant WebEmbedSlide
Client->>LightBox: open lightbox (bookmark, isActive, zoomRef)
LightBox->>VideoSlide: render (bookmark, isActive, zoomRef, onVideoError)
VideoSlide->>VideoPlayer: render video with onError callback
VideoPlayer->>VideoPlayer: attempt to load/play video
alt Video loads successfully
VideoPlayer->>VideoSlide: playback proceeds
else Video fails to load/play
VideoPlayer->>VideoSlide: invoke onError()
VideoSlide->>LightBox: report video error (bookmarkId)
LightBox->>LightBox: add bookmarkId to videoErrorIds
LightBox->>VideoSlide: re-render => if bookmark.ogImage present
VideoSlide->>WebEmbedSlide: render fallback (ogImage) with zoomRef
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🤖 Fix all issues with AI agents
In @src/components/lightbox/LightboxRenderers.tsx:
- Line 1: This file uses client-only React hooks (useState, useEffect,
useCallback) but is missing the required "use client" directive; fix it by
adding the exact string "use client" as the first line of
src/components/lightbox/LightboxRenderers.tsx so the module is treated as a
Client Component and the hooks (used in the LightboxRenderers module) are
allowed.
- Around line 68-101: The VideoSlide component relies solely on VideoPlayer's
onError (handleVideoError) which misses silent stalls (e.g., 403), so add an
additional watchdog: when the video src (compute the same src logic) or isActive
changes, start a timeout (e.g., 8–12s) that calls setVideoError(true) if no
successful load/progress event occurs; clear the timeout on unmount, src change,
or when VideoPlayer reports success (expose/use a onReady or onProgress callback
or a playerRef and call a passed-in onLoaded/onProgress handler from VideoPlayer
to cancel the watchdog). Also keep the existing onError handler to immediately
setVideoError(true). Optionally, consider server-side URL validation before
rendering as an alternative.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/supabase-nextjs-rules.mdc)
**/*.{ts,tsx,js,jsx}: Must use@supabase/ssrpackage for creating browser and server clients in Next.js Supabase Auth implementation
Browser client must usecreateBrowserClientfrom@supabase/ssrwith NEXT_PUBLIC_SUPABASE_URL and NEXT_PUBLIC_SUPABASE_PUBLISHABLE_KEY
Server client must usecreateServerClientfrom@supabase/ssrwith proper cookie handling usinggetAllandsetAllmethods only
NEVER use individual cookie methods (get,set,remove) for Supabase client configuration
NEVER import from@supabase/auth-helpers-nextjsas it is deprecated
Server client cookiesetAllmethod must include try-catch error handling as it may be called from Server Components
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/supabase-use-realtime.mdc)
**/*.{js,ts,jsx,tsx}: Usebroadcastfor all realtime events (database changes via triggers, messaging, notifications, game state)
Usepresencesparingly for user state tracking (online status, user counters)
Use topic names that correlate with concepts and tables:scope:entity(e.g.,room:123:messages)
Include unsubscribe/cleanup logic in all implementations
Setprivate: truefor channels using database triggers or RLS policies
Give preference to use private channels over public channels (better security and control)
Implement proper error handling and reconnection logic
Do not usepostgres_changesfor new applications (single-threaded, doesn't scale well); migrate tobroadcast from databaseon existing applications if necessary
Do not create multiple subscriptions without proper cleanup
Do not use database functions (realtime.send,realtime.broadcast_changes) in client code
Use dedicated, granular topics to ensure messages are only sent to relevant listeners and improve scalability
Check if channel state is 'subscribed' before subscribing to prevent multiple subscriptions
Set auth withsupabase.realtime.setAuth()before subscribing to private channels
Configure broadcast options: useself: trueto receive your own broadcast messages andack: truefor server acknowledgment
Monitor channel states (SUBSCRIBED, TIMED_OUT, CLOSED, CHANNEL_ERROR) and rely on built-in automatic reconnection with exponential backoff
Use one channel per logical scope (e.g.,room:123) rather than nested scopes to optimize channel structure
Shard high-volume topics (e.g.,chat:shard:1,chat:shard:2) to distribute load and improve scalability
Enable enhanced logging by settinglog_level: 'info'in Supabase Realtime client configuration for debugging
Replacepostgres_changeswithbroadcastlisteners in migration from postgres_changes to realtime.broadcast_changes
Prefer generating code usingbroadcastoverpostgres_changesin all new implementations
Alw...
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.{js,ts,jsx,tsx,sql}
📄 CodeRabbit inference engine (.cursor/rules/supabase-use-realtime.mdc)
**/*.{js,ts,jsx,tsx,sql}: Use snake_case for event names:entity_action(e.g.,message_created)
Do not use generic event names like 'update' or 'change'; use specific event names followingentity_actionpattern
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/supabase-use-realtime.mdc)
**/*.{jsx,tsx}: Do not subscribe directly in render functions without state management
Use ReactuseRefto store channel references and prevent multiple subscriptions in useEffect hooks
Include cleanup logic in React useEffect return to remove channels and prevent memory leaks
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Only create abstractions when actually needed. Prefer clear function/variable names over inline comments. Avoid helper functions when a simple inline expression suffices.
Useknipto remove unused code when making large changes.
Don't unnecessarily addtry/catchblocks.
Add Sentry breadcrumbs for cache misses and state inconsistencies to aid debugging (Optimistic mutations).
Maximum 250 lines per file - split larger files into modules.
Only Functional Programming - Never Class Based Code.
Only Named exports - Never default exports.
Functions use camelCase naming convention.
Constants use UPPER_SNAKE_CASE naming convention.
Prefer type deduction over custom interfaces.
Use interface withpropsparameter for functions with 2 or more parameters.
Use type alias (type Child = Parent) when child props equal parent props instead of creating new interface.
Use Utility Types:Parameters<>,ReturnType<>,Pick<>,Awaited<>for type deduction.
UsefetchAPI; never use axios or older HTTP client alternatives.
Never useanytypes.
Never use@ts-ignoredirectives.
Never use CommonJS; use ES modules.
Never usevar; useconstorlet.
Use template literals instead of string concatenation.
Use optional chaining (?.) for safe property access.
Usefor...ofloops instead of traditional for loops or forEach.
Never useeval().
Never useargumentsobject; use rest parameters instead.
Never use enums; use const objects or union types instead.
Never use TypeScript namespaces; use ES modules instead.
TypeScript strict mode always enabled.
Seedocs/sentry_rules.mdfor Sentry implementation examples for error tracking, performance monitoring, and logging best practices.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Avoid massive JSX blocks - compose smaller components.
AvoiduseEffectunless absolutely needed.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/{src/components,src/pageComponents}/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/{src/components,src/pageComponents}/**/*.tsx: Use Base UI (@base-ui/react) as the primary library for new components including forms, combobox/select, and accessible primitives.
Use the Combobox pattern from/src/components/ui/recollect/comboboxwhich uses context for state management and match-sorter filtering.
Use the ScrollArea wrapped Base UI component at/src/components/ui/recollect/scroll-area.tsxwith fade/gutter support.
Useuse-category-multi-selecthook with Base UI Combobox and match-sorter for filtering in multi-select implementations (see/src/components/lightbox/category-multi-select.tsx).
Prefer React Aria components for legacy implementations only (still used in dashboard and lightbox); prefer Base UI for new implementations.
Use Ariakit (@ariakit/react) for specialized use cases only.
Components use PascalCase naming convention.
Use Tailwind CSS v4 withcn()for conditional classes.
Use Semantic HTML over ARIA roles; use native elements.
All interactive elements must be keyboard accessible.
Never usetabIndexgreater than 0 or on non-interactive elements.
Labels required for all form inputs.
Use meaningful alt text for images; avoid generic terms like 'image', 'picture', 'photo'.
Place hooks at top level of components with all dependencies declared.
Never use array indices as keys in React lists.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Mostly use built-in Tailwind values, occasionally allow dynamic values, rarely use globals.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/{src/components,src/pageComponents,src/app,src/pages}/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
For next/image above the fold: use
sync/eager/prioritysparingly.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/{src/app,src/components,src/pageComponents}/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use Server components by default; use
'use client'directive only when needed.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/*.{css,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Grid for layout and modern CSS features (nesting, container queries).
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
**/{src/components,src/pageComponents,src/app}/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use Error boundaries for graceful failure handling in React.
Files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
🧠 Learnings (5)
📚 Learning: 2026-01-02T17:26:26.505Z
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T17:26:26.505Z
Learning: Applies to **/{src/components,src/pageComponents,src/app}/**/*.tsx : Use Error boundaries for graceful failure handling in React.
Applied to files:
src/components/VideoPlayer.tsx
📚 Learning: 2025-12-16T11:52:03.069Z
Learnt from: navin-moorthy
Repo: timelessco/recollect PR: 694
File: src/components/lightbox/AddToCollectionDropdown.tsx:163-167
Timestamp: 2025-12-16T11:52:03.069Z
Learning: When reviewing code for react-aria-components usage, check for nightly build API changes in Select: replace selectedKeys/onSelectionChange with value/onChange. For single-select, value should be a scalar representing the item ID; for multi-select, value should be an array of item IDs and onChange should receive the updated array. Update any references in src/components/...tsx files accordingly (e.g., AddToCollectionDropdown.tsx) to use value and onChange with the appropriate types, and remove any remaining selectedKeys or onSelectionChange usage.
Applied to files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
📚 Learning: 2026-01-08T07:01:43.857Z
Learnt from: rogerantony-dev
Repo: timelessco/recollect PR: 732
File: src/components/toggleDarkMode.tsx:5-5
Timestamp: 2026-01-08T07:01:43.857Z
Learning: Within the repository where the settings modal is built using Ariakit Dialog (src/components/modal.tsx), prefer using Ariakit components (e.g., AriaDropdown) inside the modal to maintain accessibility, consistent styling, and reduce custom wiring. This guidance applies to TSX files under src/components that render modals or settings dialogs.
Applied to files:
src/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
📚 Learning: 2025-12-18T15:51:47.156Z
Learnt from: navin-moorthy
Repo: timelessco/recollect PR: 704
File: src/components/lightbox/category-multi-select.tsx:142-142
Timestamp: 2025-12-18T15:51:47.156Z
Learning: In lightbox-related components, maintain a stacking order where the lightbox uses z-index 9999. Any overlay or popover (e.g., category-multi-select positioner) that must appear above the lightbox should use a z-index of 10000 or higher. Apply this pattern to all TSX files under src/components/lightbox to ensure consistent layering and prevent overlays from appearing behind the lightbox.
Applied to files:
src/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsx
📚 Learning: 2025-11-25T08:33:53.554Z
Learnt from: CR
Repo: timelessco/recollect PR: 0
File: .cursor/rules/supabase-use-realtime.mdc:0-0
Timestamp: 2025-11-25T08:33:53.554Z
Learning: Applies to **/*.{jsx,tsx} : Use React `useRef` to store channel references and prevent multiple subscriptions in useEffect hooks
Applied to files:
src/components/lightbox/LightboxRenderers.tsx
🧬 Code graph analysis (2)
src/components/lightbox/LightBox.tsx (1)
src/components/lightbox/LightboxRenderers.tsx (1)
VideoSlide(68-101)
src/components/lightbox/LightboxRenderers.tsx (2)
src/components/VideoPlayer.tsx (1)
VideoPlayer(4-25)src/utils/constants.ts (1)
tweetType(237-237)
🔇 Additional comments (2)
src/components/VideoPlayer.tsx (1)
4-24: LGTM! Clean error callback addition.The optional
onErrorprop is correctly typed and passed through toReactPlayer, enabling the video error handling mechanism used by the lightbox fallback.src/components/lightbox/LightBox.tsx (1)
133-139: LGTM! Consistent zoom reference propagation.The
zoomRefis correctly passed toVideoSlide, consistent with howImageSlide(line 147) andWebEmbedSlide(line 160) receive the zoom reference.
|
This PR handles video load failures with graceful fallback to preview images. Testing Instructions
or please use [email protected] if you want to test directly/ if you have a account which has an expired twitter video use that itself directly DemoScreen.Recording.2026-01-09.at.10.03.37.AM.mov |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/lightbox/LightboxRenderers.tsx`:
- Around line 73-83: VideoSlide's videoError state can persist across bookmark
changes causing stale fallbacks; add an effect that resets videoError (via
setVideoError) whenever the bookmark prop (or bookmark.id/unique key) changes so
each new slide starts with videoError=false. Locate the VideoSlide component and
add a useEffect that watches the bookmark (or a stable unique bookmark
identifier) and calls setVideoError(false) when it changes.
| export const VideoSlide = ({ bookmark, isActive, zoomRef }: SlideProps) => { | ||
| const [videoError, setVideoError] = useState(false); | ||
|
|
||
| const handleVideoError = useCallback(() => { | ||
| setVideoError(true); | ||
| }, []); | ||
|
|
||
| // If video failed to load and we have an ogImage, show the image instead | ||
| if (videoError && bookmark?.ogImage) { | ||
| return <WebEmbedSlide bookmark={bookmark} zoomRef={zoomRef} />; | ||
| } |
There was a problem hiding this comment.
Reset videoError when bookmark changes to prevent stale fallback.
The videoError state persists across bookmark changes. If the carousel reuses this component instance when navigating slides, a previous video's error state could incorrectly trigger the fallback for a different video that hasn't failed.
🐛 Proposed fix using useEffect to reset state
export const VideoSlide = ({ bookmark, isActive, zoomRef }: SlideProps) => {
const [videoError, setVideoError] = useState(false);
+ // Reset error state when bookmark changes
+ useEffect(() => {
+ setVideoError(false);
+ }, [bookmark?.id]);
+
const handleVideoError = useCallback(() => {
setVideoError(true);
}, []);🤖 Prompt for AI Agents
In `@src/components/lightbox/LightboxRenderers.tsx` around lines 73 - 83,
VideoSlide's videoError state can persist across bookmark changes causing stale
fallbacks; add an effect that resets videoError (via setVideoError) whenever the
bookmark prop (or bookmark.id/unique key) changes so each new slide starts with
videoError=false. Locate the VideoSlide component and add a useEffect that
watches the bookmark (or a stable unique bookmark identifier) and calls
setVideoError(false) when it changes.
There was a problem hiding this comment.
actually its working fine, its re rendering even without the useeffect
There was a problem hiding this comment.
@rogerantony-dev, thank you for clarifying! If the lightbox creates fresh component instances for each slide (likely through proper keying or conditional rendering), then each VideoSlide would indeed start with a clean videoError = false state, and no explicit reset is needed. Appreciate you testing and confirming the behavior works correctly! 👍
…anced bookmark data
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 (2)
src/components/lightbox/hooks/useLightboxLogic.ts (1)
51-110: Explicitly mark errored videos as image slides.When
hasVideoErroris true,isVideobecomes false buttype/data.typecan still end upundefined(mediaType is still video). If any zoom or renderer behavior expects animagetype, this can bypass the intended fallback. Consider forcing the slide type toIMAGE_TYPE_PREFIXon error.🐛 Suggested fix
const isVideo = !hasVideoError && (bookmark?.type?.startsWith(VIDEO_TYPE_PREFIX) || Boolean(bookmark?.meta_data?.video_url) || Boolean(bookmark?.meta_data?.additionalVideos?.[0])); + + const slideType = hasVideoError + ? IMAGE_TYPE_PREFIX + : isVideo + ? VIDEO_TYPE_PREFIX + : isImage + ? IMAGE_TYPE_PREFIX + : undefined; return { src: bookmark?.url, // Set slide type for lightbox to handle appropriately - type: isVideo - ? VIDEO_TYPE_PREFIX - : isImage - ? IMAGE_TYPE_PREFIX - : undefined, + type: slideType, // Embed bookmark data in slide for plugin access data: { bookmark, - type: isVideo - ? VIDEO_TYPE_PREFIX - : isImage - ? IMAGE_TYPE_PREFIX - : undefined, + type: slideType, },src/components/lightbox/LightboxRenderers.tsx (1)
77-103: Avoid a truthy guard onbookmark.id.A valid
idof0would skip the callback. The type check alone is enough.🔧 Suggested tweak
const handleVideoError = useCallback(() => { - if (bookmark?.id && typeof bookmark.id === "number") { + if (typeof bookmark?.id === "number") { onVideoError?.(bookmark.id); } }, [bookmark?.id, onVideoError]);
🤖 Fix all issues with AI agents
In `@src/components/lightbox/LightBox.tsx`:
- Around line 143-170: The fallback should not require bookmark.ogImage; update
the conditional that chooses between VideoSlide and WebEmbedSlide to use only
the video error flag (hasVideoError) so any video failure renders WebEmbedSlide
(which handles iframe/loader when no image exists). Locate the logic around
VideoSlide/WebEmbedSlide selection (references: VideoSlide, WebEmbedSlide,
hasVideoError computed from videoErrorIds and handleVideoError) and remove the
dependency on bookmark.ogImage; apply the same change to the second occurrence
of this pattern later in the file (the other conditional using videoErrorIds).
🧹 Nitpick comments (1)
src/components/lightbox/hooks/useLightboxLogic.ts (1)
37-43: Use a props-object signature foruseLightboxSlides.Now that the hook takes two arguments, prefer a
{ bookmarks, videoErrorIds }props object with a dedicated interface and alphabetical destructuring to keep call sites self-documenting. Update call sites accordingly.♻️ Suggested refactor
+interface UseLightboxSlidesProps { + bookmarks: SingleListData[] | undefined; + videoErrorIds?: Set<number>; +} + export const useLightboxSlides = ( - bookmarks: SingleListData[] | undefined, - videoErrorIds?: Set<number>, -) => { + { bookmarks, videoErrorIds }: UseLightboxSlidesProps, +) => {As per coding guidelines, for functions with 2 or more parameters, use the props object pattern with a
FunctionNamePropsinterface, destructure props alphabetically on the first line.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.