[CORE-8124] 2.4.y.z/lead-ytb-vid-fail-trunk #2597
Conversation
…8124/lead-ytb-vid-fail-trunk
There was a problem hiding this comment.
Pull request overview
This pull request addresses a bug where YouTube/Vimeo video overlays (Plyr controls) fail to load on subsequent visits to the same topic. The issue was caused by Angular's change detection not re-rendering the video embed container when structurally similar HTML was set after cleanup.
Changes:
- Enhanced video player lifecycle management with proper cleanup and re-initialization using
ChangeDetectorRefto force DOM updates - Improved state management with new flags (
videoNeedsInit,plyrNeedsInit,plyrInitialized) andngAfterViewCheckedlifecycle hook - Added comprehensive error handling and logging for video playback failures
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 22 comments.
| File | Description |
|---|---|
| projects/v3/src/app/components/topic/topic.component.ts | Added lifecycle management with AfterViewChecked, forced change detection in ngOnChanges, enhanced cleanup with try-catch error handling, improved Plyr initialization with duplicate checks, added memory cleanup with destroy$ subject, enhanced video error handler with detailed logging and user notifications |
| projects/v3/src/app/components/topic/topic.component.html | Updated video element with template reference, new conditional rendering based on videoSrc, changed preload strategy to 'auto', added controlsList, playsinline attributes, and fallback content for unsupported browsers |
| projects/v3/src/app/components/topic/topic.component.scss | Added CSS rule to hide Plyr poster overlay using ::ng-deep |
| docs/components/topicComponent.md | Added comprehensive documentation of the video playback bug, root cause analysis, solution implementation details, testing checklists, and future improvements |
Comments suppressed due to low confidence (1)
projects/v3/src/app/components/topic/topic.component.ts:163
- The 'destroy$' Subject is properly completed in 'ngOnDestroy', but the 'continueAction$' Subject is only completed (line 163) without calling 'next()' first. While this should work, it's a best practice to call 'next()' before 'complete()' to signal completion to any pending subscribers, ensuring they can clean up properly.
ngOnDestroy(): void {
this.destroy$.next();
this.destroy$.complete();
this.sharedService.stopPlayingVideos();
this.topicService.clearTopic();
this.cleanupMedia();
this.cleanupSub.unsubscribe();
this.continueAction$.complete();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (this.continueAction$) { | ||
| this.continueAction$.next(topic); | ||
| } |
There was a problem hiding this comment.
The condition 'if (this.continueAction$)' is checking if a Subject exists. Since 'continueAction$' is initialized in the class as a private field (line 37: 'private continueAction$ = new Subject()'), it will always be truthy unless it's explicitly set to null or undefined elsewhere. This null check is unnecessary and adds no value.
| if (this.continueAction$) { | |
| this.continueAction$.next(topic); | |
| } | |
| this.continueAction$.next(topic); |
| // store plyr instance reference for cleanup | ||
| (embedVideo as any).plyr = plyrInstance; |
There was a problem hiding this comment.
The Plyr instance is stored directly on the embedVideo element using '(embedVideo as any).plyr = plyrInstance'. This approach bypasses TypeScript's type system and could lead to memory leaks if the element is removed from the DOM without proper cleanup. Consider using a WeakMap to store Plyr instances keyed by their corresponding elements, which would provide better type safety and automatic cleanup when elements are garbage collected.
| ngAfterViewChecked(): void { | ||
| // initialize native video when it becomes available in the DOM | ||
| if (this.videoNeedsInit && this.topicVideo?.nativeElement) { | ||
| this.videoNeedsInit = false; | ||
|
|
||
| requestAnimationFrame(() => { | ||
| const videoEl = this.topicVideo?.nativeElement; | ||
| if (videoEl && this.videoSrc) { | ||
| videoEl.src = this.videoSrc; | ||
| videoEl.load(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The 'ngAfterViewChecked' lifecycle hook runs on every change detection cycle, which can be very frequent. Using 'requestAnimationFrame' inside this hook for every check could lead to performance issues and potential memory leaks if the animations queue up. Consider using a flag to track whether initialization has already been scheduled to prevent multiple 'requestAnimationFrame' callbacks from being queued.
No description provided.