fix(react): memoize Provider context value#1541
Conversation
|
Someone is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
|
| Name | Link |
|---|---|
| 🔨 Latest commit | 9e29157 |
|
Hi @mav2287! I created the PR with your changes, as external PRs are currently restricted. Would you mind: 1. Updating the branch with upstream main, 2. Applying only that change and discarding the rest, 3. Updating the PR description? Let me know if that works! Otherwise, I can pick up your contribution from where you left off. |
The `Provider` returned by `createPlayer()` was building a fresh context value object on every render. Every `useContext(PlayerContext)` consumer therefore re-rendered on any parent re-render — even when none of the value's members changed. Combined with callback-ref usage in media components (`HlsVideo`, `DashVideo`, etc.), this caused the underlying media to detach and re-attach on every parent state change. The most visible symptom: `onEnded` fires, parent state updates, the video rewinds to t=0 and plays again. Wrap the value object in `useMemo` keyed on `[store, media, container, popupGroup]`. `setMedia` and `setContainer` are `useState` setters and are identity-stable; `store` and `popupGroup` are also initialized once via `useState(() => ...)` and never change for the lifetime of the Provider, but they're listed in the dep array to satisfy Biome's `useExhaustiveDependencies`. Adds a regression test that asserts context value identity is preserved across forced parent re-renders. Signed-off-by: James <james@BlainWebDesign.com>
mav2287
force-pushed
the
fix/1296-provider-value-attach-ref-churn
branch
from
22df048 to
9e29157
Compare
|
@R-Delfino95 done — branch rebased and reduced to just the Diff: 2 files changed, 58 insertions(+), 7 deletions(-).
Test results in
The Commit is DCO-signed. I don't have permission to edit the PR title/body myself (only you can as the PR author). When you get a chance, would you mind updating them to reflect the single-change scope? Happy to draft a new description if helpful — let me know. Thanks again for picking this up on our behalf! |
R-Delfino95
changed the title
attachMediaElement refs
mihar-22
left a comment
mihar-22
left a comment
There was a problem hiding this comment.
Thank you! Just some comment clean up would be great :)
| // Without memoization, the Provider rebuilds its context value on every | ||
| // render. Every `useContext(PlayerContext)` consumer then re-renders on | ||
| // any parent re-render, which (combined with callback-ref usage in | ||
| // media components) detaches and re-attaches the underlying media on | ||
| // every parent state change. At end-of-stream this rewinds the video. |
There was a problem hiding this comment.
We can delete this, safe to assume common React knowledge and inferred by the test title.
| // Memoize the context value so its identity only changes when one of its | ||
| // members actually changes. Without this, every render of a component | ||
| // above the Provider rebuilds the value object, which forces every | ||
| // `useContext(PlayerContext)` consumer to re-render even when nothing | ||
| // it depends on changed. `setMedia` and `setContainer` are `useState` | ||
| // setters and are identity-stable; `store` and `popupGroup` are also | ||
| // initialized once via `useState(() => ...)` and never change for the | ||
| // lifetime of the Provider. They're listed in the dep array to satisfy | ||
| // `useExhaustiveDependencies`. See #1296. |
There was a problem hiding this comment.
We can delete this, usage of useMemo already makes it clear and the rest should be common React knowledge.
4 participants
Related to #1296
What changed
Diff: 2 files changed, 58 insertions(+), 7 deletions(-).
packages/react/src/player/create-player.tsx— Provider context value wrapped inuseMemokeyed on[store, media, container, popupGroup].setMediaandsetContainerare omitted (identity-stableuseStatesetters).popupGroup(added onmainsince the original PR was opened) is included in both the value and the dep array.packages/react/src/player/tests/create-player.test.tsx— one regression test added: mounts ausePlayerContext()consumer, forces parent re-renders three times, asserts context value identity is preserved (toBe).Test results in
packages/react:main)pnpm typecheck(root)pnpm linton the 2 changed filesThe
attachMediaElementWeakMapfix has been fully discarded. Worth noting that #1376 has since landed onmain, which introduceduseAttachMedia(auseCallback-based hook keyed on[media]), so the ref-identity stability that fix #2 was guarding against is now handled at the hook level — fix #2 isn't just unwanted, it's obsolete against currentmain.Note
Low Risk
Low risk: scoped to React context value memoization and adds a regression test; behavior should be unchanged except fewer unnecessary re-renders and attach/detach churn.
Overview
Fixes
createPlayer().Providerto memoize thePlayerContextvalue viauseMemo, so consumers don’t re-render (and trigger media detach/reattach) on unrelated parent renders (regression #1296).Adds a test asserting the context value identity remains stable across repeated parent re-renders.
Reviewed by Cursor Bugbot for commit 9e29157. Bugbot is set up for automated code reviews on this repo. Configure here.