Fix VideoPlayer in Next docs #1085

joshfarrant · 2025-07-03T15:12:44Z

Summary

Fixes VideoPlayer in Next docs by ensuring VideoPlayer ref isn't overriden by null ref passed by Next docs. I'm not entirely sure why, but the Next docs pass a ref of null to rendered components.

The VideoPlayer intentionally doesn't accept a ref prop as it instead exposes a useVideo() context to facilitate interaction with the video player. A ref is available through that context if required.

Because of this, we can explicitly override any refs provided directly to VideoPlayer and resolve the issue with the Next docs.

What should reviewers focus on?

Check that the VideoPlayer functions in the Next docs

Contributor checklist

All new and existing CI checks pass
Tests prove that the feature works and covers both happy and unhappy paths
Any drop in coverage, breaking changes or regressions have been documented above
UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
All developer debugging and non-functional logging has been removed
Related issues have been referenced in the PR description

Reviewer checklist

Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
Check that tests prove the feature works and covers both happy and unhappy paths
Check that there aren't other open Pull Requests for the same update/change

changeset-bot · 2025-07-03T15:12:48Z

🦋 Changeset detected

Latest commit: 3d225f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
@primer/react-brand	Patch
@primer/brand-docs	Patch
@primer/brand-css	Patch
@primer/brand-primitives	Patch
@primer/brand-e2e	Patch
@primer/brand-fonts	Patch
@primer/brand-config	Patch
@primer/brand-storybook	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-07-03T15:14:57Z

🟢 No design token changes found

github-actions · 2025-07-03T15:21:14Z

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

Copilot

Pull Request Overview

This PR ensures that VideoPlayer’s internal ref isn’t overridden by a null ref passed from Next.js, and updates the changeset for this fix.

Refactor: move the ref prop after the spread to guarantee it overrides any incoming ref
Changelog: add a patch-level changeset entry for this internal refactoring

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
packages/react/src/VideoPlayer/VideoPlayer.tsx	Moved the `ref` prop after the spread so it always wins
.changeset/chilled-bottles-hug.md	Added a patch changeset for the internal refactoring

Comments suppressed due to low confidence (2)

packages/react/src/VideoPlayer/VideoPlayer.tsx:73

Consider adding a unit or integration test that passes a null ref prop (simulating Next.js behavior) and verifies that the internal ref is still attached correctly.

        <video title={title} controls={false} className={clsx(styles.VideoPlayer, className)} {...rest} ref={ref}>

.changeset/chilled-bottles-hug.md:5

The changeset description could be more specific by mentioning that this patch fixes a ref override issue when Next.js passes a null ref.

Minor internal refactoring to `VideoPlayer` component

rezrah · 2025-07-07T13:10:34Z

packages/react/src/VideoPlayer/VideoPlayer.tsx

@@ -70,7 +70,7 @@ const Root = ({
  return (
    <div className={styles.VideoPlayer__container} ref={fullscreenRef}>
      <div className={styles.VideoPlayer__overlayContainer}>
-        <video ref={ref} title={title} controls={false} className={clsx(styles.VideoPlayer, className)} {...rest}>
+        <video title={title} controls={false} className={clsx(styles.VideoPlayer, className)} {...rest} ref={ref}>


@joshfarrant can you add a test for this to ensure the ref doesn't get overridden?

I don't think I can unfortunately.

We'd be testing the internals of React at this point and I'm not sure there's a way to actually assert that.

Might be worth discussing this a bit further as — if I understand the underlying issue correctly — we're working around a react-live bug/functionality here.

I've just tested in react-live's demo and the ref prop isn't being set to null there, so maybe there's a bug in our implementation. I'll put this PR on hold for a while and look into it a bit further

rezrah · 2025-07-07T13:19:37Z

.changeset/chilled-bottles-hug.md

+'@primer/react-brand': patch
+---
+
+Minor internal refactoring to `VideoPlayer` component


Could there be a scenario where someone was passing a ref as a prop, which now won't work because we're no longer spreading it in?

I don't believe that's possible no. The VideoPlayer doesn't implement forwardRef, so ref is dropped as a prop before it ever reaches the component. That means it's not been possible for someone to pass in a ref before this point.

Given that, I'm not 100% sure why props.ref is ending up as null (rather than undefined) when rendered in the docs.

The only thing that I think could be causing it is that react-live doesn't implement that same React behaviour of dropping the ref prop, which results in it getting through to the component.

In that case, the only environment this change should affect is in our docs.

rezrah · 2025-07-09T12:03:37Z

@joshfarrant FYI this change, combined with the recent Doctocat updates seems to have fixed the VideoPlayer previews 🙌 https://primer-e8d998b27d-26139705.drafts.github.io/brand/components/VideoPlayer/

Once you're confident this change won't lead to a downstream regression in the library, let's ship it.

joshfarrant temporarily deployed to github-pages July 3, 2025 15:21 — with GitHub Actions Inactive

joshfarrant force-pushed the joshfarrant/fix-videoplayer-in-next-docs branch from 3ede5c0 to f2e1a6b Compare July 3, 2025 15:27

joshfarrant marked this pull request as ready for review July 3, 2025 15:29

joshfarrant requested review from Copilot and rezrah July 3, 2025 15:29

Copilot AI reviewed Jul 3, 2025

View reviewed changes

joshfarrant mentioned this pull request Jul 3, 2025

GA of Next.js docs #1083

Merged

9 tasks

joshfarrant temporarily deployed to github-pages July 3, 2025 15:34 — with GitHub Actions Inactive

joshfarrant added 2 commits July 3, 2025 17:02

ensure ref is not overridden by null ref passed by next docs

fcb694f

add changeset

e0acd4c

joshfarrant force-pushed the joshfarrant/fix-videoplayer-in-next-docs branch from f2e1a6b to e0acd4c Compare July 3, 2025 16:02

joshfarrant temporarily deployed to github-pages July 3, 2025 16:06 — with GitHub Actions Inactive

rezrah reviewed Jul 7, 2025

View reviewed changes

Merge branch 'main' into joshfarrant/fix-videoplayer-in-next-docs

3d225f6

joshfarrant marked this pull request as draft July 9, 2025 11:36

rezrah deployed to github-pages July 9, 2025 11:38 — with GitHub Actions View deployment

joshfarrant marked this pull request as ready for review July 9, 2025 15:27

rezrah approved these changes Jul 9, 2025

View reviewed changes

joshfarrant merged commit 61e9adc into main Jul 10, 2025
18 checks passed

joshfarrant deleted the joshfarrant/fix-videoplayer-in-next-docs branch July 10, 2025 06:42

primer-css mentioned this pull request Jul 9, 2025

Release tracking #1091

Merged

github-actions bot mentioned this pull request Jul 15, 2025

New release available "@primer/[email protected]" tspascoal-primer/brand-releases#5

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix VideoPlayer in Next docs #1085

Fix VideoPlayer in Next docs #1085

Uh oh!

joshfarrant commented Jul 3, 2025 •

edited

Loading

Uh oh!

changeset-bot bot commented Jul 3, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 3, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 3, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

rezrah Jul 7, 2025

Uh oh!

joshfarrant Jul 7, 2025

Uh oh!

joshfarrant Jul 7, 2025 •

edited

Loading

Uh oh!

rezrah Jul 7, 2025

Uh oh!

joshfarrant Jul 7, 2025

Uh oh!

rezrah commented Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

Fix VideoPlayer in Next docs #1085

Fix VideoPlayer in Next docs #1085

Uh oh!

Conversation

joshfarrant commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What should reviewers focus on?

Contributor checklist

Reviewer checklist

Uh oh!

changeset-bot bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 No design token changes found

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 No visual differences found

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

rezrah Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

joshfarrant Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

joshfarrant Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rezrah Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

joshfarrant Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

rezrah commented Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

joshfarrant commented Jul 3, 2025 •

edited

Loading

changeset-bot bot commented Jul 3, 2025 •

edited

Loading

github-actions bot commented Jul 3, 2025 •

edited

Loading

github-actions bot commented Jul 3, 2025 •

edited

Loading

joshfarrant Jul 7, 2025 •

edited

Loading