fix(courses): prevent autoplay loop on completed video lessons

[syed-tp]

• Avoid triggering completion on initial load when seek position is near the end.
• Reset previously completed videos to play from the beginning.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

Claude finished @syed-tp's task in 3m 59s —— View job

---

PR Review Summary

✅ What's good

• Root cause is correctly diagnosed and clearly documented in design.md/proposal.md: a resumed video seeking to (or near) its saved end position immediately re-fires onComplete, which loops autoplay-next.
• Nice touch proactively adding an OpenSpec change (openspec/changes/fix-autoplay-next-lesson-loop/) and updating the base spec even though this is a bug fix and OpenSpec isn't strictly required for it.
• Resetting initialPos to 0.0 for completed lessons (video_lesson_detail_screen.dart:150-152, video_lesson_viewer.dart:179-181) is a simple, correct fix for the "replay a completed lesson" case and matches the design doc.
• No SDK boundary, design-system/Material-widget, accessibility, motion, navigation, or generated-file issues found in this diff — the change is scoped entirely to playback state logic.

🔴 CRITICAL — Must fix before merge

1. _shouldIgnoreInitialCompletion guard can permanently suppress genuine completion. In packages/courses/lib/widgets/lesson_detail/custom_video_player.dart, the guard is set (line ~216) whenever the initial/resume seek target is within 2s of the video's duration — this covers not just "completed" lessons (which are now forced to start at 0.0) but also in-progress lessons whose lastWatchedDuration happens to be near the end (e.g. user closed the app 1s before finishing). The reset condition (lines 226-230) only clears the flag when currentPos < duration - 2.0. If playback simply continues forward from a near-end resume point to real completion, currentPos never dips back below that threshold, so the flag is never cleared — meaning widget.onComplete (line 253) is silently never called for the rest of that session. The lesson would never be marked complete and autoplay-next would never fire, even though the user genuinely finished watching it.
   • This isn't just a theoretical edge case — it's the exact scenario decision feat(navigation-shell): implement unified routing architecture #2 in design.md targets (seek-on-load near the end), just without the "did the user actually finish it this time" distinction.
   • Suggested fix: track whether the video has made forward progress since resume (e.g. only ignore completion until the next seek/resume, or clear the guard once the position has advanced past the original target seek by some margin) rather than relying solely on "did position dip below duration-2".
   • Fix this →

🟡 WARNING — Should fix

1. No test coverage for this stateful logic. custom_video_player.dart has no existing test file, and this PR doesn't add one despite introducing a non-trivial state machine (_hasSeekedToInitial / _shouldIgnoreInitialCompletion / seek detection). A test that drives the controller listener through "resume near end → keep playing → reach duration" would have caught the CRITICAL issue above. Consider extracting the completion/seek logic into a small testable class or adding a widget/controller-driven test.
2. duration > 5.0 guard excludes short videos from the fix. For videos ≤5s, the near-end guard never applies, so the original autoplay-loop bug could still reproduce for very short clips. If short lessons exist in this product, worth confirming this is an acceptable non-goal (design.md doesn't call this out explicitly).

🔵 SUGGESTION — Nice to have

1. The magic numbers 5.0 and 2.0 (lines 215, 228) are repeated with no named constant — extracting const _nearEndThresholdSeconds = 2.0 / _minDurationForGuard = 5.0 would make the intent (and future tuning) clearer.
2. restorePlayback() (line ~125) resets _hasSeekedToInitial, _isPlayingTracker, etc. but not _shouldIgnoreInitialCompletion; it happens to self-correct on the next listener tick in most cases, but explicitly resetting it alongside the other flags would make the state transitions easier to reason about.

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 2m 56s —— View job

---

PR Review Summary

(Re-review after the latest push — commit 3bed99f addresses the CRITICAL issue raised in the previous automated review.)

✅ What's good

• The previously-flagged CRITICAL bug is now fixed: custom_video_player.dart:230-237 resets _shouldIgnoreInitialCompletion based on movement relative to _initialSeekPos (currentPos > _initialSeekPos + 0.1 or < _initialSeekPos - 0.5) instead of the old currentPos < duration - 2.0 check. Since forward playback moves currentPos past _initialSeekPos + 0.1 almost immediately, a genuinely in-progress lesson resumed near its end will now still fire onComplete when actually finished — the "permanently suppress genuine completion" scenario from the last review no longer reproduces.
• The short-video exclusion (duration > 5.0) that previously let clips ≤5s fall through the guard has also been replaced with a scaled threshold (duration > 2.0 ? 2.0 : duration * 0.5), closing that gap too.
• initialPos reset to 0.0 for completed lessons (video_lesson_detail_screen.dart:150-153, video_lesson_viewer.dart:179-182) is correct and matches design.md.
• OpenSpec change (openspec/changes/fix-autoplay-next-lesson-loop/) and base spec update are thorough and consistent with the implementation, even though this is a bug fix and not strictly required.
• No SDK boundary, design-system/Material-widget, accessibility, motion, navigation, provider, or data-layer issues — the change is scoped entirely to playback state logic in courses, importing only package:core/data/data.dart.

🔴 CRITICAL — Must fix before merge

None found.

🟡 WARNING — Should fix

1. Still no test coverage for this stateful logic. custom_video_player.dart has no test file, and this fix required two iterations to get right (the first attempt had a real regression that a listener-driven test would have caught). Consider extracting the seek/completion state machine (_hasSeekedToInitial / _shouldIgnoreInitialCompletion / _initialSeekPos) into a small pure/testable class, or adding a controller-driven widget test covering: (a) resume near end → keep playing → reach duration → onComplete fires, (b) resume near end → no further progress → onComplete does not fire, (c) resume near end → seek backward → play to end → onComplete fires.
2. _shouldIgnoreInitialCompletion is never explicitly cleared when a fresh initial seek is not near-end (custom_video_player.dart:207-224). The if (targetSeek >= duration - nearEndThreshold) branch only ever sets the flag to true; there's no corresponding else { _shouldIgnoreInitialCompletion = false; }. In the current call graph this looks self-correcting in practice (the flag only survives paused/unchanged position, and _pendingSeekPosition is derived from that same unchanged position), but it's a latent trap if restorePlayback()/_onPlayerCreated is ever reused across a position change that isn't naturally covered by the forward/backward reset checks. Recommend explicitly resetting _shouldIgnoreInitialCompletion = false in the non-near-end branch (and in restorePlayback() alongside the other flags at line ~125) so the state machine doesn't rely on incidental invariants.

🔵 SUGGESTION — Nice to have

1. The threshold constants (2.0, 0.1, 0.5) in custom_video_player.dart:217-233 are still magic numbers. Naming them (e.g. _nearEndThresholdSeconds, _forwardProgressMarginSeconds, _backwardSeekMarginSeconds) would make the intent and future tuning clearer.
2. restorePlayback() (line ~125) resets _hasSeekedToInitial, _isPlayingTracker, _currentIntervalStart, _lastPosition but not _shouldIgnoreInitialCompletion/_initialSeekPos — see WARNING feat(navigation-shell): implement unified routing architecture #2 above for why explicitly resetting these too would make the transitions easier to reason about.

Verdict

APPROVED — the blocking issue from the previous review is resolved. The test-coverage gap (WARNING #1) is worth addressing given this is a second attempt at the same bug class, but it's not a functional blocker.