feat(#127): drive video player from real course data - #151
Conversation
- Replace hardcoded files.vidstack.io demo subtitles/thumbnails with real course captions, sourced only from data.subtitles/data.chapters - Add chapter markers (VTT or array) with graceful degrade when absent - Persist playback rate/volume/muted to localStorage across courses - Add clear error state for missing/broken video instead of blank player - Add discoverable keyboard-shortcuts help and a focusable, labelled player - Lazy-load the player component to keep Vidstack off the initial bundle Progress/resume tracking intentionally left to Deen-Bridge#108.
|
@dair7 is attempting to deploy a commit to the Deen Bridge Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe course page now lazy-loads VidPlayerBox client-side. The player adds keyboard shortcut help, persisted playback preferences, chapter tracks, remounting for media changes, and explicit missing-video or playback-error states. ChangesCourse video player
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CourseDetailPageClient
participant VidPlayerBox
participant LocalStorage
participant MediaPlayer
CourseDetailPageClient->>VidPlayerBox: load client-only player
VidPlayerBox->>LocalStorage: read playback preferences
VidPlayerBox->>MediaPlayer: configure media, storage, and tracks
MediaPlayer-->>VidPlayerBox: emit playback error when loading fails
VidPlayerBox-->>CourseDetailPageClient: render player or error state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx (1)
20-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNice bundle-size win, but no fallback if the chunk fails to load.
Deferring Vidstack to a lazy import with a spinner is a solid call. However,
next/dynamicdoesn't provide a built-in error UI — if the chunk request fails (flaky network, ad-blocker, deploy-time cache mismatch), theimport()rejection will surface as an uncaught render error with no user-facing fallback here.Consider wrapping the dynamic component in a lightweight error boundary so learners get an actionable message instead of a blank/broken player area.
As per path instructions, "Flag ... missing loading/error states for data fetching" for
app/**files.💡 Sketch of a minimal error boundary wrapper
class PlayerLoadBoundary extends React.Component { state = { hasError: false }; static getDerivedStateFromError() { return { hasError: true }; } render() { if (this.state.hasError) { return ( <div className="w-full h-full flex items-center justify-center rounded-xl bg-black/90 text-white/80 text-sm"> Couldn't load the video player. Please refresh the page. </div> ); } return this.props.children; } }Then wrap the
<VidPlayerBox .../>usage (Line 196) with<PlayerLoadBoundary>...</PlayerLoadBoundary>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/dashboard/courses/`[courseId]/CourseDetailPageClient.jsx around lines 20 - 34, Wrap the VidPlayerBox usage with a lightweight PlayerLoadBoundary error boundary so rejected dynamic imports render an actionable player-area fallback instead of propagating a render error. Add the boundary’s error state and fallback UI near the VidPlayerBox definition, while preserving the existing dynamic loading spinner and normal player rendering.Source: Path instructions
components/atoms/dashboard/vid-player-box.jsx (1)
206-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid error/remount handling; consider logging the underlying media error for observability.
Hook ordering, the missing-video guard, and the remount-on-media-change
keystrategy all look correct. One small improvement:onError={() => setHasPlaybackError(true)}discards the error detail entirely, so there's no way to diagnose why a video failed (bad URL, CORS, codec, etc.) from logs/monitoring.Capturing the event detail would make future support/debugging much easier without changing user-facing behavior.
♻️ Suggested logging addition
- onError={() => setHasPlaybackError(true)} + onError={(detail) => { + console.error('VidPlayerBox media error:', detail); + setHasPlaybackError(true); + }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/atoms/dashboard/vid-player-box.jsx` around lines 206 - 263, Update the MediaPlayer onError handler in VidPlayerBox to capture the underlying media error or event detail and log it for observability, while still calling setHasPlaybackError(true) so the existing user-facing error behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/dashboard/courses/`[courseId]/CourseDetailPageClient.jsx:
- Around line 20-34: Wrap the VidPlayerBox usage with a lightweight
PlayerLoadBoundary error boundary so rejected dynamic imports render an
actionable player-area fallback instead of propagating a render error. Add the
boundary’s error state and fallback UI near the VidPlayerBox definition, while
preserving the existing dynamic loading spinner and normal player rendering.
In `@components/atoms/dashboard/vid-player-box.jsx`:
- Around line 206-263: Update the MediaPlayer onError handler in VidPlayerBox to
capture the underlying media error or event detail and log it for observability,
while still calling setHasPlaybackError(true) so the existing user-facing error
behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32e67972-5860-4f6d-b7c1-bb36d550cbb3
📒 Files selected for processing (2)
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsxcomponents/atoms/dashboard/vid-player-box.jsx
1 participant
Progress/resume tracking intentionally left to #108.
Closes #127
Summary by CodeRabbit
New Features
Performance