Skip to content

fix: track playing state for videoJS player #599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 15, 2025
Merged

Conversation

junhyoung-ryu
Copy link
Collaborator

@junhyoung-ryu junhyoung-ryu commented May 15, 2025

User description

  • Overlay was not disappearing for videoJS player. This PR fixes that

PR Type

Bug fix, Enhancement


Description

  • Simplify player container styling globally

  • Import usePlayerStore in VideoJSPlayer

  • Set playing state on loadedmetadata

  • Remove redundant isMobile check


Changes walkthrough 📝

Relevant files
Enhancement
player.tsx
Simplify player container styling                                               

apps/app/components/welcome/featured/player.tsx

  • Removed conditional isMobile class logic
  • Always apply w-full h-full container styling
  • +1/-1     
    Bug fix
    videojs-player.tsx
    Add playing state tracking in VideoJSPlayer                           

    apps/app/components/welcome/featured/videojs-player.tsx

  • Imported usePlayerStore hook
  • Retrieved setIsPlaying from player store
  • Wrapped loadedmetadata event to call setIsPlaying(true) then apply
    styles
  • +6/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    vercel bot commented May 15, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    pipelines-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2025 4:04am

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    State management

    setIsPlaying(true) is only called on loadedmetadata without resetting on pause or ended. Consider handling pause, ended, or cleanup events to accurately track playback state.

    player.on("loadedmetadata", () => {
      setIsPlaying(true);
      applyCustomStyles();
    });
    Import path

    Importing usePlayerStore from "./player" may not reference the correct module. Verify that the hook is exported from this path.

    import { usePlayerStore } from "./player";

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use play/pause for state

    Track playback using actual video events instead of metadata load. Replace the
    loadedmetadata handler for playback state with play and pause events and keep
    metadata styling separate.

    apps/app/components/welcome/featured/videojs-player.tsx [298-301]

    -player.on("loadedmetadata", () => {
    +player.on("play", () => {
       setIsPlaying(true);
    -  applyCustomStyles();
     });
    +player.on("pause", () => {
    +  setIsPlaying(false);
    +});
    +player.on("loadedmetadata", applyCustomStyles);
    Suggestion importance[1-10]: 7

    __

    Why: Tracking playback with play/pause events gives a more accurate isPlaying state and cleanly separates metadata styling, improving player behavior.

    Medium
    Possible issue
    Cleanup event listeners

    Avoid memory leaks by unregistering event listeners on unmount. Store the handler in
    a variable and call player.off in the component cleanup.

    apps/app/components/welcome/featured/videojs-player.tsx [302]

    -player.on("resize", applyCustomStyles);
    +const resizeHandler = applyCustomStyles;
    +player.on("resize", resizeHandler);
    +// ...
    +// In cleanup:
    +player.off("resize", resizeHandler);
    Suggestion importance[1-10]: 6

    __

    Why: Unregistering the resize listener in a cleanup prevents potential memory leaks when the component unmounts, enhancing resource management.

    Low

    @junhyoung-ryu junhyoung-ryu merged commit 339895a into main May 15, 2025
    6 checks passed
    @junhyoung-ryu junhyoung-ryu deleted the jun/videojs branch May 15, 2025 04:04
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant