feat(course-progress): add resume playback, completion tracking, and progress bars - #147
Conversation
…progress bars - Add useCourseProgress hook with localStorage write-behind and throttled API writes - Add useAllCourseProgress for batched progress on course listing pages - Extend VidPlayerBox with startTime, onTimeUpdate, and onEnded props - Add resume/start-over UX on course detail page with progress indicator - Add progress bars and completion badges to CourseCard for owned courses - Graceful degradation: works via localStorage when backend endpoints unavailable - Completion marked at >=90% watched or on player ended event Closes Deen-Bridge#108
|
@BountySpaghetti is attempting to deploy a commit to the Deen Bridge Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds persistent course progress tracking with localStorage and backend synchronization, resume and start-over playback controls, completion handling, and ownership-aware progress indicators and actions on course cards. ChangesCourse Progress Experience
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
Actionable comments posted: 11
🧹 Nitpick comments (3)
hooks/useCourseProgress.js (3)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
apiCheckedRefis written but never read.It's assigned in the
finallyblock and nowhere else consumed. Either drop it or wire it into the loading/fallback decision so the intent is explicit.🤖 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 `@hooks/useCourseProgress.js` at line 72, Remove the unused apiCheckedRef declaration and its assignments from the course progress hook, unless the loading/fallback logic is updated to read it explicitly. Keep the existing behavior unchanged and avoid retaining a write-only ref.
242-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the duplicated resume condition.
The same three-part predicate is repeated for
resumeTimeandresumeLabel, and the magic30/90would be easier to tune as named constants (90also duplicatesCOMPLETION_THRESHOLD * 100).♻️ Suggested tidy-up
+ const canResume = + !progress.completed && + progress.positionSeconds > MIN_RESUME_SECONDS && + progress.percent < COMPLETION_THRESHOLD * 100; + return { progress, loading, reportProgress, resetProgress, - resumeTime: - !progress.completed && progress.positionSeconds > 30 && progress.percent < 90 - ? progress.positionSeconds - : null, - resumeLabel: - !progress.completed && progress.positionSeconds > 30 && progress.percent < 90 - ? `Resume from ${formatTime(progress.positionSeconds)}` - : null, + resumeTime: canResume ? progress.positionSeconds : null, + resumeLabel: canResume + ? `Resume from ${formatTime(progress.positionSeconds)}` + : null, };🤖 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 `@hooks/useCourseProgress.js` around lines 242 - 249, In the progress mapping logic of useCourseProgress, hoist the shared resume eligibility predicate into a named local value and reuse it for both resumeTime and resumeLabel. Replace the magic 30-second and 90-percent thresholds with named constants, reusing COMPLETION_THRESHOLD for the percent comparison where available, while preserving the current eligibility behavior.
224-235: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove
visibilitychangetodocument; addpagehideas a fallback.
visibilitychangebelongs ondocument, andwindow.addEventListener("visibilitychange", ...)is less reliable in Safari/WebKit. Usingdocument.addEventListener(...)keeps the listener on the documented target, andpagehidecovers cases where the page is backgrounded or unloaded without a final visibility event, which matters here because it’s the last chance to flush progress.🤖 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 `@hooks/useCourseProgress.js` around lines 224 - 235, Update the effect around handleVisibility to register and clean up visibilitychange on document instead of window, and add a pagehide listener that calls flushNow as a fallback. Preserve the existing hidden-state behavior and ensure both listeners are removed during cleanup before the final flush.
🤖 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.
Inline comments:
In `@app/dashboard/courses/`[courseId]/CourseDetailPageClient.jsx:
- Around line 34-45: Destructure the loading state from useCourseProgress as
progressLoading, then gate the resume banner and elapsed-time/progress display
blocks on !progressLoading so they remain hidden or show their loading
placeholder until progress data resolves. Keep the existing progress and resume
behavior unchanged after loading completes.
- Around line 60-68: Update handleResume to remount the player or explicitly
seek through the player ref when enabling resume, ensuring VidPlayerBox applies
resumeTime as clipStartTime. Prefer incrementing playerKey consistently with
handleStartOver if no seek API is already available.
In `@components/atoms/dashboard/vid-player-box.jsx`:
- Around line 32-36: The ended handler in
components/atoms/dashboard/vid-player-box.jsx lines 32-36 must pass
player.state.duration to onEnded. Update the onEnded consumer in
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx lines 54-58 to
accept that duration, prefer it over progress.durationSeconds, and use an
explicit completion path when no duration is available instead of calling
reportProgress(0, 0).
- Around line 19-45: Throttle PlayerProgressTracker time-update forwarding to at
most once per second using lastReportRef, and store onEnded in a ref so the
effect does not resubscribe on callback changes; update the effect dependencies
accordingly while preserving cleanup. In hooks/useCourseProgress.js lines
179-192, conditionally skip setProgress and writeLocalProgress when the floored
positionSeconds and derived percent match currentDataRef.current.
- Line 62: Update the video player setup in VidPlayerBox so clipStartTime no
longer receives startTime or clips the source. Preserve the original media
duration, and seek to startTime once the player is ready using the existing
player-ready lifecycle and refs; ensure reportProgress() continues receiving the
full duration while playback resumes at the requested offset.
In `@components/molecules/dashboard/cards/courseCard.jsx`:
- Around line 21-27: The course card’s inline hasPurchased calculation can
diverge from the shared ownership state and incorrectly control the price badge.
Replace the user.purchasedCourses/enrolledCourses logic in the course card with
the existing useHasCourse(course?._id) source of truth, or hoist a shared
ownership set from the dashboard courses page alongside useAllCourseProgress and
pass it through the card props; preserve the existing price-badge behavior.
- Around line 91-95: Supply accessible labels for every new Progress instance:
in components/molecules/dashboard/cards/courseCard.jsx lines 91-95, label the
bar with the course title and watched percentage; in
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx lines 190-198, add
the course-completion label to the elapsed-time bar and apply the same treatment
to the resume-banner bar at line 138. Update each Progress call site without
changing its existing values or styling.
- Around line 29-30: Update the isCompleted calculation in the course card to
require hasPurchased in addition to progress?.completed, matching the ownership
gate used by showProgress. Preserve the existing false fallback when there is no
qualifying completion state so unowned courses continue showing purchase-related
UI consistently.
In `@hooks/useCourseProgress.js`:
- Around line 53-57: Update formatTime to handle non-finite or invalid seconds
before performing arithmetic, returning a sane fallback label instead of
rendering NaN or Infinity. Also adjust its formatting for durations of one hour
or more so the output includes hours while preserving the existing
minute-and-second format for shorter durations; keep the
CourseDetailPageClient.jsx caller unchanged.
- Around line 97-139: Add a cancelled guard to the fetch effect containing
fetchFromApi: initialize a cancellation flag, prevent the resolved handler from
updating state or writing progress when cancelled, and set the flag in the
effect cleanup when userId or courseId changes or the component unmounts. Mirror
the cancellation behavior used by useAllCourseProgress while preserving the
existing loading and apiCheckedRef handling for the active request.
- Around line 257-265: Replace the boolean fetchedRef guard in the
userId-dependent progress-loading effect with a ref that stores the fetched user
id, and only skip when it matches the current userId. Assign
fetchedForRef.current = userId at the existing point where fetchedRef.current is
set true, so account switches trigger a fresh fetch and do not retain the
previous user's progress.
---
Nitpick comments:
In `@hooks/useCourseProgress.js`:
- Line 72: Remove the unused apiCheckedRef declaration and its assignments from
the course progress hook, unless the loading/fallback logic is updated to read
it explicitly. Keep the existing behavior unchanged and avoid retaining a
write-only ref.
- Around line 242-249: In the progress mapping logic of useCourseProgress, hoist
the shared resume eligibility predicate into a named local value and reuse it
for both resumeTime and resumeLabel. Replace the magic 30-second and 90-percent
thresholds with named constants, reusing COMPLETION_THRESHOLD for the percent
comparison where available, while preserving the current eligibility behavior.
- Around line 224-235: Update the effect around handleVisibility to register and
clean up visibilitychange on document instead of window, and add a pagehide
listener that calls flushNow as a fallback. Preserve the existing hidden-state
behavior and ensure both listeners are removed during cleanup before the final
flush.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 61effc17-fa0b-4730-aa4b-217d32e134a3
📒 Files selected for processing (5)
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsxapp/dashboard/courses/page.jsxcomponents/atoms/dashboard/vid-player-box.jsxcomponents/molecules/dashboard/cards/courseCard.jsxhooks/useCourseProgress.js
| const { | ||
| progress, | ||
| reportProgress, | ||
| resetProgress, | ||
| resumeTime, | ||
| resumeLabel, | ||
| } = useCourseProgress(course?._id); | ||
|
|
||
| const [useResume, setUseResume] = useState(false); | ||
| const [playerKey, setPlayerKey] = useState(0); | ||
|
|
||
| const effectiveStartTime = useResume && resumeTime ? resumeTime : 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Surface the hook's loading state.
useCourseProgress returns loading, but it isn't destructured here. Until the GET /api/progress/courses round-trip resolves, progress.percent is 0, so the page renders as "no progress" and then the resume banner + progress bar pop in and shift the player down. Gating those blocks on !loading (or rendering a small skeleton) avoids the layout jump.
🛠️ Suggested wiring
const {
progress,
+ loading: progressLoading,
reportProgress,
resetProgress,
resumeTime,
resumeLabel,
} = useCourseProgress(course?._id);Then guard the resume banner and the elapsed-time block with !progressLoading.
As per path instructions, this is a Next.js 15 App Router file and I should flag "missing loading/error states for data fetching".
🤖 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 34
- 45, Destructure the loading state from useCourseProgress as progressLoading,
then gate the resume banner and elapsed-time/progress display blocks on
!progressLoading so they remain hidden or show their loading placeholder until
progress data resolves. Keep the existing progress and resume behavior unchanged
after loading completes.
Source: Path instructions
| const handleResume = () => { | ||
| setUseResume(true); | ||
| }; | ||
|
|
||
| const handleStartOver = () => { | ||
| resetProgress(); | ||
| setUseResume(false); | ||
| setPlayerKey((k) => k + 1); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Vidstack react MediaPlayer clipStartTime prop reactive change after mount seek
💡 Result:
In the Vidstack React MediaPlayer, the clipStartTime prop is intended to define a playback range boundary [1]. While you can change the clipStartTime prop after the component has mounted by updating the state passed to the MediaPlayer, this does not automatically trigger a seek operation to that new start time [2]. To achieve a reactive update where the player seeks to the new start time when the clipStartTime prop changes, you must manually coordinate the seek operation using the player instance [2][3]. Implementation Strategy: 1. Access the Player Instance: Use the useMediaPlayer hook within a child component of the MediaPlayer to retrieve the player instance [3][4]. 2. Detect Changes: Use a useEffect hook to watch for changes to the clipStartTime variable [5]. 3. Trigger Seek: Inside the effect, use the player instance or a MediaRemoteControl to set the currentTime to the new clipStartTime [3]. Example approach: import { useMediaPlayer, useEffect } from '@vidstack/react'; function ClipController({ newStartTime }) { const player = useMediaPlayer; useEffect( => { if (player) { // Seek to the new start time player.currentTime = newStartTime; } }, [newStartTime, player]); return null; } Note that Vidstack includes internal logic (such as boundTime) that constrains playback within the range defined by clipStartTime and clipEndTime [1][6]. If you are building complex trimming interfaces, be aware that these props fundamentally alter the player's perception of the timeline and duration [2][6]. In some scenarios, manual intervention via event listeners like onMediaSeekRequest may be necessary to bypass default bounding behavior if it conflicts with your specific UI requirements [7].
Citations:
- 1: https://github.com/vidstack/player/blob/main/packages/vidstack/src/core/api/player-state.ts
- 2: Can we loop the video in a selected time range? vidstack/player#1369
- 3: https://vidstack.io/docs/player/core-concepts/state-management/
- 4: https://vidstack.io/docs/player/api/hooks/use-media-player/
- 5: https://vidstack.io/docs/player/getting-started/architecture/
- 6: https://github.com/vidstack/player/blob/main/packages/vidstack/src/core/state/media-state-manager.ts
- 7: The currentTime set to a value > 0 and < 1s is not working, it will always be 0. vidstack/player#1531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx'
# Show the relevant region with line numbers.
sed -n '1,220p' "$file" | nl -ba | sed -n '1,220p'
# Find player-related state and props in the file.
echo
rg -n "playerKey|useResume|resumeTime|clipStartTime|currentTime|MediaPlayer|handleResume|handleStartOver|resetProgress" "$file"Repository: Deen-Bridge/dnb-frontend
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx'
echo '--- relevant file section ---'
sed -n '1,240p' "$file" | cat -n
echo
echo '--- player/start-time references ---'
rg -n "playerKey|useResume|resumeTime|clipStartTime|currentTime|MediaPlayer|handleResume|handleStartOver|resetProgress|startTime" "$file"Repository: Deen-Bridge/dnb-frontend
Length of output: 11269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='components/atoms/dashboard/vid-player-box.jsx'
echo '--- file outline ---'
wc -l "$file"
echo
sed -n '1,260p' "$file" | cat -n
echo
echo '--- startTime / MediaPlayer references ---'
rg -n "startTime|clipStartTime|currentTime|MediaPlayer|useMediaPlayer|seek|playerKey|key=" "$file"Repository: Deen-Bridge/dnb-frontend
Length of output: 3119
Resume needs a remount or explicit seek
handleResume only flips useResume, but VidPlayerBox passes startTime into MediaPlayer as clipStartTime. The player stays mounted, so Resume won’t jump to resumeTime; Start Over works because it increments playerKey. Bump playerKey in handleResume too, or seek the player ref when enabling resume.
🤖 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 60
- 68, Update handleResume to remount the player or explicitly seek through the
player ref when enabling resume, ensuring VidPlayerBox applies resumeTime as
clipStartTime. Prefer incrementing playerKey consistently with handleStartOver
if no seek API is already available.
| function PlayerProgressTracker({ onTimeUpdate, onEnded }) { | ||
| const player = useMediaPlayer(); | ||
| const lastReportRef = useRef(0); | ||
|
|
||
| useEffect(() => { | ||
| if (!player) return; | ||
|
|
||
| const timeSub = player.on('time-update', (e) => { | ||
| if (onTimeUpdate) { | ||
| onTimeUpdate(e.currentTime, e.duration); | ||
| } | ||
| }); | ||
|
|
||
| const endedSub = player.on('ended', () => { | ||
| if (onEnded) { | ||
| onEnded(); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
| timeSub(); | ||
| endedSub(); | ||
| }; | ||
| }, [player, onTimeUpdate, onEnded]); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Progress reporting is unthrottled end-to-end. The PR aims to "throttle progress writes", but the only throttle is the 15s gate on the backend PUT. Vidstack's time-update fires several times per second and every one of those events currently causes a React state update, a synchronous JSON.stringify + localStorage.setItem, and — because the state change invalidates the parent's handleEnded — a full teardown and re-attach of both player event subscriptions. The intended throttle already has a home: the unused lastReportRef.
components/atoms/dashboard/vid-player-box.jsx#L19-L45: uselastReportRefto droptime-updateevents that arrive within ~1s of the previous forwarded one, soonTimeUpdateis called at most once per second. Also droponEndedfrom the effect dep array (keep it in a ref) so the subscriptions attach once per player rather than once per tick.hooks/useCourseProgress.js#L179-L192: makesetProgressandwriteLocalProgressconditional — skip both whenMath.floor(positionSeconds)and the derivedpercentare unchanged fromcurrentDataRef.current, so idle ticks cost nothing.
📍 Affects 2 files
components/atoms/dashboard/vid-player-box.jsx#L19-L45(this comment)hooks/useCourseProgress.js#L179-L192
🤖 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 19 - 45, Throttle
PlayerProgressTracker time-update forwarding to at most once per second using
lastReportRef, and store onEnded in a ref so the effect does not resubscribe on
callback changes; update the effect dependencies accordingly while preserving
cleanup. In hooks/useCourseProgress.js lines 179-192, conditionally skip
setProgress and writeLocalProgress when the floored positionSeconds and derived
percent match currentDataRef.current.
| const endedSub = player.on('ended', () => { | ||
| if (onEnded) { | ||
| onEnded(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The onEnded callback carries no duration, so completion-on-end can silently fail. The player knows the authoritative duration when ended fires but discards it; the consumer therefore has to reconstruct it from React state, and that value can still be 0, in which case isCompleted(0, 0) returns false and the finished course is never marked complete.
components/atoms/dashboard/vid-player-box.jsx#L32-L36: invokeonEnded(player.state.duration)so the real duration reaches the consumer.app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx#L54-L58: accept thedurationargument, prefer it overprogress.durationSeconds, and fall back to an explicit completion path rather thanreportProgress(0, 0)when no duration is available.
📍 Affects 2 files
components/atoms/dashboard/vid-player-box.jsx#L32-L36(this comment)app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx#L54-L58
🤖 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 32 - 36, The
ended handler in components/atoms/dashboard/vid-player-box.jsx lines 32-36 must
pass player.state.duration to onEnded. Update the onEnded consumer in
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx lines 54-58 to
accept that duration, prefer it over progress.durationSeconds, and use an
explicit completion path when no duration is available instead of calling
reportProgress(0, 0).
| playsInline | ||
| title={data?.title} | ||
| poster={data?.thumbnail} | ||
| clipStartTime={startTime || undefined} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Vidstack clipStartTime clipEndTime reported duration timeline behavior
💡 Result:
In Vidstack, clipStartTime and clipEndTime are properties used to shorten media by defining the boundaries of playback [1][2]. Their behavior regarding the timeline, duration, and state management is defined as follows: Duration Behavior: When clipStartTime and clipEndTime are set, the player updates the media duration to match the new clipped length [1][2]. Seeking, progress tracking, and other time-based UI components (such as chapters) are recalculated to reflect this clipped duration [1]. For example, if a 60-second video is clipped to start at 30 seconds and end at 60 seconds, the player's reported duration becomes 30 seconds, and seeking to 30s moves the playhead to the end of the media [1]. Timeline and State Behavior: - Internal URI Fragments: Vidstack internally uses Media URI Fragments (e.g., /video.mp4#t=30,60) to efficiently load and constrain the media between the specified start and end times [1][2]. - Seeking: Seeking operations are relative to the clipped duration [1]. - Reporting: The duration property (available on the player component) reflects the total length of the clipped segment [1][3]. - Implementation Notes: While Vidstack handles the clipping logic, there have been reported edge cases in specific versions where the UI timeline or progress thumb might behave unexpectedly (e.g., getting "stuck" or resetting) depending on how and when the clipping properties are updated during playback [4]. Usage: These properties can be passed directly as attributes or props to the MediaPlayer component [2]: - If you require looping within a custom range rather than permanent clipping, Vidstack does not provide a built-in range-loop feature; this is typically handled by subscribing to time updates and programmatically resetting currentTime when the end time is reached [5].
Citations:
- 1: https://vidstack.io/docs/player/core-concepts/loading/
- 2: Player Release 1.10@next: It’s Time for the Big One (Jan 2024) vidstack/player#1116
- 3: https://vidstack.io/docs/player/components/core/player/
- 4: Current time is incorrect at end of clipped video vidstack/player#1342
- 5: Can we loop the video in a selected time range? vidstack/player#1369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="components/atoms/dashboard/vid-player-box.jsx"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant lines =="
nl -ba "$file" | sed -n '1,180p'Repository: Deen-Bridge/dnb-frontend
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="components/atoms/dashboard/vid-player-box.jsx"
echo "== lines 1-140 =="
awk 'NR>=1 && NR<=140 { printf("%4d %s\n", NR, $0) }' "$file"Repository: Deen-Bridge/dnb-frontend
Length of output: 2587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "onTimeUpdate|calcPercent|reportProgress|VidPlayerBox" components . --glob '!**/node_modules/**'Repository: Deen-Bridge/dnb-frontend
Length of output: 3238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
"app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx"
"hooks/useCourseProgress.js"
)
for file in "${files[@]}"; do
echo "== $file =="
awk 'NR>=1 && NR<=260 { printf("%4d %s\n", NR, $0) }' "$file"
echo
doneRepository: Deen-Bridge/dnb-frontend
Length of output: 20434
🌐 Web query:
Vidstack clipStartTime currentTime duration time-update relative to clipped duration docs
💡 Result:
In Vidstack Player, the relationship between playback time and clipping is managed by distinguishing between the "real" playback time (raw provider time) and the "clipped" playback time [1][2]. When using clipStartTime and clipEndTime, Vidstack automatically adjusts the player state to reflect the clipped duration [3][4]. Here is how the relevant time properties behave: currentTime: The currentTime property represents the playback time relative to the clipped duration [1][5]. If you have a 60-second video and set a clipStartTime of 30 seconds, the player will treat the 30-second mark of the source as 0 seconds in the clipped timeline [1][3]. Setting this value will seek the media to the corresponding position within the clipped window [1][5]. time-update: The time-update event is dispatched when the playback time changes [2]. The detail provided in this event contains the currentTime value, which is relative to the clipped duration [2][6]. This is the recommended event to listen to if you want to track the playback position within the clipped range [2]. time-change: If you require the "real" or "raw" time (the actual position in the original source media without accounting for clipping), you should listen to the time-change event instead [2]. The time-change event provides the absolute provider time [2]. Underlying Logic: The internal state management calculates currentTime using the following logic [1]: If clipStartTime > 0, currentTime is derived by subtracting the clipStartTime from the realCurrentTime (clamped to the clipped duration) [1]. The duration property is automatically updated to match the clipped length (e.g., if you clip a 1-minute video to 30 seconds, the duration will be 30 seconds) [3][4]. This approach allows you to work with clipped media as if it were a standalone file, while the underlying MediaRemoteControl or state hooks provide the necessary tools to manipulate playback within those bounds [7][8][3].
Citations:
- 1: https://github.com/vidstack/player/blob/main/packages/vidstack/src/core/api/player-state.ts
- 2: https://github.com/vidstack/player/blob/main/packages/vidstack/src/core/api/media-events.ts
- 3: https://vidstack.io/docs/player/core-concepts/loading/
- 4: Player Release 1.10@next: It’s Time for the Big One (Jan 2024) vidstack/player#1116
- 5: https://vidstack.io/docs/player/components/core/player/
- 6: https://github.com/vidstack/player/blob/main/packages/vidstack/src/core/state/media-state-manager.ts
- 7: https://vidstack.io/docs/player/core-concepts/state-management/
- 8: https://vidstack.io/docs/player/api/classes/media-remote-control/
components/atoms/dashboard/vid-player-box.jsx:62 — clipStartTime is the wrong primitive for resume here. Vidstack reports time-update.currentTime and duration relative to the clipped window, so reportProgress() will store a shortened duration and undercount course progress after resume. Seek to startTime once the player is ready instead of clipping the source.
🤖 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` at line 62, Update the video
player setup in VidPlayerBox so clipStartTime no longer receives startTime or
clips the source. Preserve the original media duration, and seek to startTime
once the player is ready using the existing player-ready lifecycle and refs;
ensure reportProgress() continues receiving the full duration while playback
resumes at the requested offset.
| const showProgress = hasPurchased && progress && progress.percent > 0; | ||
| const isCompleted = progress?.completed || false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
isCompleted skips the hasPurchased gate, so the CTA can contradict the card.
showProgress requires ownership, but isCompleted is derived from progress?.completed alone. For a course the user doesn't own that still has a progress entry (stale localStorage from a lapsed enrolment, or a backend row), line 139 renders "Review Course" while the completion badge at line 65 stays hidden and the price badge at line 116 is shown — a card that simultaneously says "buy this" and "you finished this".
🛠️ Proposed fix
const showProgress = hasPurchased && progress && progress.percent > 0;
- const isCompleted = progress?.completed || false;
+ const isCompleted = Boolean(hasPurchased && progress?.completed);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const showProgress = hasPurchased && progress && progress.percent > 0; | |
| const isCompleted = progress?.completed || false; | |
| const showProgress = hasPurchased && progress && progress.percent > 0; | |
| const isCompleted = Boolean(hasPurchased && progress?.completed); |
🤖 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/molecules/dashboard/cards/courseCard.jsx` around lines 29 - 30,
Update the isCompleted calculation in the course card to require hasPurchased in
addition to progress?.completed, matching the ownership gate used by
showProgress. Preserve the existing false fallback when there is no qualifying
completion state so unowned courses continue showing purchase-related UI
consistently.
| {showProgress && ( | ||
| <div className="mb-3"> | ||
| <Progress value={progress.percent} className="h-1.5" /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Every new Progress instance lacks an accessible name. components/ui/progress.jsx renders a Radix root with role="progressbar" and aria-valuenow but no label, so assistive tech announces a bare number with no indication of what is progressing. Each new call site needs to supply its own aria-label.
-
components/molecules/dashboard/cards/courseCard.jsx#L91-L95: addaria-label={${course.title}: ${progress.percent}% watched}to the card progress bar. -
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx#L190-L198: addaria-label={Course progress: ${progress.percent}% complete}to the elapsed-time bar, and the same treatment to the resume-banner bar at line 138.
📍 Affects 2 files
components/molecules/dashboard/cards/courseCard.jsx#L91-L95(this comment)app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx#L190-L198
🤖 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/molecules/dashboard/cards/courseCard.jsx` around lines 91 - 95,
Supply accessible labels for every new Progress instance: in
components/molecules/dashboard/cards/courseCard.jsx lines 91-95, label the bar
with the course title and watched percentage; in
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsx lines 190-198, add
the course-completion label to the elapsed-time bar and apply the same treatment
to the resume-banner bar at line 138. Update each Progress call site without
changing its existing values or styling.
| function formatTime(seconds) { | ||
| const m = Math.floor(seconds / 60); | ||
| const s = Math.floor(seconds % 60); | ||
| return `${m}:${s.toString().padStart(2, "0")}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard formatTime against non-finite input (and consider hours).
CourseDetailPageClient.jsx line 194 renders formatTime(progress.durationSeconds) directly. Before any time-update fires, or if Vidstack reports Infinity/NaN duration (common for streams that haven't loaded metadata), the UI shows NaN:NaN / Infinity:NaN. A one-line guard keeps the label sane. Videos over an hour also render as 75:03, which reads oddly.
🛠️ Proposed hardening
function formatTime(seconds) {
- const m = Math.floor(seconds / 60);
- const s = Math.floor(seconds % 60);
- return `${m}:${s.toString().padStart(2, "0")}`;
+ if (!Number.isFinite(seconds) || seconds < 0) return "0:00";
+ const h = Math.floor(seconds / 3600);
+ const m = Math.floor((seconds % 3600) / 60);
+ const s = Math.floor(seconds % 60);
+ const mm = h > 0 ? m.toString().padStart(2, "0") : `${m}`;
+ return h > 0
+ ? `${h}:${mm}:${s.toString().padStart(2, "0")}`
+ : `${mm}:${s.toString().padStart(2, "0")}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function formatTime(seconds) { | |
| const m = Math.floor(seconds / 60); | |
| const s = Math.floor(seconds % 60); | |
| return `${m}:${s.toString().padStart(2, "0")}`; | |
| } | |
| function formatTime(seconds) { | |
| if (!Number.isFinite(seconds) || seconds < 0) return "0:00"; | |
| const h = Math.floor(seconds / 3600); | |
| const m = Math.floor((seconds % 3600) / 60); | |
| const s = Math.floor(seconds % 60); | |
| const mm = h > 0 ? m.toString().padStart(2, "0") : `${m}`; | |
| return h > 0 | |
| ? `${h}:${mm}:${s.toString().padStart(2, "0")}` | |
| : `${mm}:${s.toString().padStart(2, "0")}`; | |
| } |
🤖 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 `@hooks/useCourseProgress.js` around lines 53 - 57, Update formatTime to handle
non-finite or invalid seconds before performing arithmetic, returning a sane
fallback label instead of rendering NaN or Infinity. Also adjust its formatting
for durations of one hour or more so the output includes hours while preserving
the existing minute-and-second format for shorter durations; keep the
CourseDetailPageClient.jsx caller unchanged.
| async function fetchFromApi() { | ||
| if (backendAvailable === false) { | ||
| setLoading(false); | ||
| return; | ||
| } | ||
| try { | ||
| const res = await axiosInstance.get("/api/progress/courses"); | ||
| if (res.data?.success && Array.isArray(res.data.progress)) { | ||
| backendAvailable = true; | ||
| const entry = res.data.progress.find( | ||
| (p) => p.courseId?.toString?.() === courseId.toString() | ||
| ); | ||
| if (entry) { | ||
| const pct = calcPercent(entry.positionSeconds, entry.durationSeconds); | ||
| const done = entry.completed || isCompleted(entry.positionSeconds, entry.durationSeconds); | ||
| const fromApi = { | ||
| percent: done ? 100 : pct, | ||
| positionSeconds: entry.positionSeconds, | ||
| durationSeconds: entry.durationSeconds, | ||
| completed: done, | ||
| }; | ||
| setProgress(fromApi); | ||
| currentDataRef.current = { | ||
| positionSeconds: entry.positionSeconds, | ||
| durationSeconds: entry.durationSeconds, | ||
| completed: done, | ||
| }; | ||
| completedRef.current = done; | ||
| writeLocalProgress(userId, courseId, currentDataRef.current); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| if (err?.response?.status === 404 || err?.response?.status === 405) { | ||
| backendAvailable = false; | ||
| } | ||
| } finally { | ||
| setLoading(false); | ||
| apiCheckedRef.current = true; | ||
| } | ||
| } | ||
|
|
||
| fetchFromApi(); | ||
| }, [userId, courseId]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a cancellation guard to this fetch effect.
useAllCourseProgress correctly uses a cancelled flag, but this effect has none. If courseId changes (client navigation between course pages) or the component unmounts while GET /api/progress/courses is in flight, the resolved handler still runs setProgress/writeLocalProgress using the captured userId/courseId, so the previous course's position can be painted onto the newly mounted course — which then feeds resumeTime and the resume banner. Mirroring the sibling hook keeps both paths consistent.
🛠️ Proposed fix
useEffect(() => {
if (!userId || !courseId) {
setLoading(false);
return;
}
+
+ let cancelled = false;
const local = readLocalProgress(userId, courseId);
@@
async function fetchFromApi() {
if (backendAvailable === false) {
- setLoading(false);
+ if (!cancelled) setLoading(false);
return;
}
try {
const res = await axiosInstance.get("/api/progress/courses");
+ if (cancelled) return;
if (res.data?.success && Array.isArray(res.data.progress)) {
@@
} finally {
- setLoading(false);
- apiCheckedRef.current = true;
+ if (!cancelled) setLoading(false);
+ apiCheckedRef.current = true;
}
}
fetchFromApi();
+
+ return () => {
+ cancelled = true;
+ };
}, [userId, courseId]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function fetchFromApi() { | |
| if (backendAvailable === false) { | |
| setLoading(false); | |
| return; | |
| } | |
| try { | |
| const res = await axiosInstance.get("/api/progress/courses"); | |
| if (res.data?.success && Array.isArray(res.data.progress)) { | |
| backendAvailable = true; | |
| const entry = res.data.progress.find( | |
| (p) => p.courseId?.toString?.() === courseId.toString() | |
| ); | |
| if (entry) { | |
| const pct = calcPercent(entry.positionSeconds, entry.durationSeconds); | |
| const done = entry.completed || isCompleted(entry.positionSeconds, entry.durationSeconds); | |
| const fromApi = { | |
| percent: done ? 100 : pct, | |
| positionSeconds: entry.positionSeconds, | |
| durationSeconds: entry.durationSeconds, | |
| completed: done, | |
| }; | |
| setProgress(fromApi); | |
| currentDataRef.current = { | |
| positionSeconds: entry.positionSeconds, | |
| durationSeconds: entry.durationSeconds, | |
| completed: done, | |
| }; | |
| completedRef.current = done; | |
| writeLocalProgress(userId, courseId, currentDataRef.current); | |
| } | |
| } | |
| } catch (err) { | |
| if (err?.response?.status === 404 || err?.response?.status === 405) { | |
| backendAvailable = false; | |
| } | |
| } finally { | |
| setLoading(false); | |
| apiCheckedRef.current = true; | |
| } | |
| } | |
| fetchFromApi(); | |
| }, [userId, courseId]); | |
| let cancelled = false; | |
| async function fetchFromApi() { | |
| if (backendAvailable === false) { | |
| if (!cancelled) setLoading(false); | |
| return; | |
| } | |
| try { | |
| const res = await axiosInstance.get("/api/progress/courses"); | |
| if (cancelled) return; | |
| if (res.data?.success && Array.isArray(res.data.progress)) { | |
| backendAvailable = true; | |
| const entry = res.data.progress.find( | |
| (p) => p.courseId?.toString?.() === courseId.toString() | |
| ); | |
| if (entry) { | |
| const pct = calcPercent(entry.positionSeconds, entry.durationSeconds); | |
| const done = entry.completed || isCompleted(entry.positionSeconds, entry.durationSeconds); | |
| const fromApi = { | |
| percent: done ? 100 : pct, | |
| positionSeconds: entry.positionSeconds, | |
| durationSeconds: entry.durationSeconds, | |
| completed: done, | |
| }; | |
| setProgress(fromApi); | |
| currentDataRef.current = { | |
| positionSeconds: entry.positionSeconds, | |
| durationSeconds: entry.durationSeconds, | |
| completed: done, | |
| }; | |
| completedRef.current = done; | |
| writeLocalProgress(userId, courseId, currentDataRef.current); | |
| } | |
| } | |
| } catch (err) { | |
| if (err?.response?.status === 404 || err?.response?.status === 405) { | |
| backendAvailable = false; | |
| } | |
| } finally { | |
| if (!cancelled) setLoading(false); | |
| apiCheckedRef.current = true; | |
| } | |
| } | |
| fetchFromApi(); | |
| return () => { | |
| cancelled = true; | |
| }; |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 117-117: Avoid using the initial state variable in setState
Context: setProgress(fromApi)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 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 `@hooks/useCourseProgress.js` around lines 97 - 139, Add a cancelled guard to
the fetch effect containing fetchFromApi: initialize a cancellation flag,
prevent the resolved handler from updating state or writing progress when
cancelled, and set the flag in the effect cleanup when userId or courseId
changes or the component unmounts. Mirror the cancellation behavior used by
useAllCourseProgress while preserving the existing loading and apiCheckedRef
handling for the active request.
| const fetchedRef = useRef(false); | ||
|
|
||
| const userId = user?._id; | ||
|
|
||
| useEffect(() => { | ||
| if (!userId || fetchedRef.current) { | ||
| if (!userId) setLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
fetchedRef isn't keyed to userId, so progress can persist across an account switch.
The effect depends on userId, but fetchedRef.current is a plain boolean. If userId changes while this component stays mounted (logout → login, or a refreshUser that yields a different id), the guard short-circuits and progressMap keeps the previous user's percentages on the course grid. Storing the id you fetched for makes the guard match the dependency.
🛠️ Proposed fix
- const fetchedRef = useRef(false);
+ const fetchedForRef = useRef(null);
@@
- if (!userId || fetchedRef.current) {
+ if (!userId || fetchedForRef.current === userId) {
if (!userId) setLoading(false);
return;
}(and set fetchedForRef.current = userId; where fetchedRef.current = true; is today)
🤖 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 `@hooks/useCourseProgress.js` around lines 257 - 265, Replace the boolean
fetchedRef guard in the userId-dependent progress-loading effect with a ref that
stores the fetched user id, and only skip when it matches the current userId.
Assign fetchedForRef.current = userId at the existing point where
fetchedRef.current is set true, so account switches trigger a fresh fetch and do
not retain the previous user's progress.
2 participants
Summary
Build course progress tracking UI: persist playback position and completion per course, offer "Resume where you left off" on the course detail page, and show progress bars on course cards.
Closes #108
What's Changed
New:
hooks/useCourseProgress.jsuseCourseProgress(courseId)— reads/writes progress for a single courseuseAllCourseProgress()— batched fetch for course listing grids (one API call, not N)reportProgress(positionSeconds, durationSeconds)— throttled writes (~15s intervals, plus on pause/unload viavisibilitychange)dnb:progress:<userId>:<courseId>so progress survives offline/failed PUTscompleted: truewhen position/duration ≥ 0.9 (90%) or on player ended event; completion is stickyGET /api/progress/coursesreturns 404/405, operates on localStorage only silently (feature-detects once per session)Extended:
components/atoms/dashboard/vid-player-box.jsxstartTime,onTimeUpdate, andonEndedpropsPlayerProgressTrackersub-component with vidstack'suseMediaPlayerto subscribe totime-updateandendedeventsclipStartTimeon MediaPlayer for resume playbackfiles.vidstack.iodemo URLs (they 404 for real courses)Updated:
app/dashboard/courses/[courseId]/CourseDetailPageClient.jsxuseCourseProgresshook to the player viaonTimeUpdate/onEndedcallbacksUpdated:
components/molecules/dashboard/cards/courseCard.jsxprogressprop (percent, completed)Updated:
app/dashboard/courses/page.jsxuseAllCourseProgress()at page level and passes progress data down toCourseCardFrontend Contract (for backend pairing)
lessonIdisnullfor today's single-video courses; the shape supports lessons arrays later.Acceptance Criteria
npm run lintandnpm run buildpassSummary by CodeRabbit