Fix/005 redgifs playback#3
Merged
Merged
Conversation
…race
Instruments the 17-dep useEffect at the heart of VideoPlayer with:
- An effect run counter (effectRunCountRef) that increments on every
fire so we can see how fast the loop spins.
- A dep-comparator that logs which of the 17 deps changed between
consecutive fires, using Object.is for reference equality.
Output format: [VideoPlayer trace] effect run #N — changed deps: { ... }.
This is investigation scaffolding for issue 005 (RedGIFs lightbox
playback runaway loop). The original hypothesis ranked four candidate
unstable deps; this trace will identify the actual one without
guessing. The instrumentation will be removed in a follow-up commit
after the fix lands so the diff for the real fix stays readable on
its own.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ls is the unstable dep Re-investigated under branch fix/005-redgifs-playback with the temp effect-run-counter + dep-comparator instrumentation now live on VideoPlayer.jsx (commit 6b4a0e9). The trace produced ~7900 effect fires in 5 seconds with sanitizedCompanionAudioUrls as the only changed dep across every fire. Key updates to the issue file: - loadedmetadata DOES fire (the original "never reaches loadedmetadata" line is wrong; the loop is downstream of metadata). - All four original hypotheses are eliminated by trace evidence. - Real cause is a fifth hypothesis not on the original list: the destructure default `companionAudioUrls = []` in VideoPlayer.jsx:28 evaluates a fresh array literal on every function invocation. When the caller (LightboxModal's RedGIFs and prebuffer branches) omits the prop, sanitizedCompanionAudioUrls becomes a new ref every render, invalidating the 17-dep useEffect and triggering the loop. - The regular v.redd.it HLS path is unaffected because LightboxModal passes a memoized companionAudioUrls to that VideoPlayer. The targeted fix lands in a separate commit on this branch after maintainer approval. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nt as destructure default Refs issues/005-redgifs-playback-loop.md. Default-expression literals in JavaScript evaluate fresh on every function invocation, so `companionAudioUrls = []` on the destructure produced a new array reference on every render of VideoPlayer. When the caller omits the prop, the default ran each render: useMemo on lines 56-59 saw an unstable dep via Object.is, the memo factory re-ran and produced a new filtered array, and the 17-dep useEffect saw a new sanitizedCompanionAudioUrls reference on every fire. The cleanup-resetup cycle looped at ~1500 fires/sec. Two call sites in LightboxModal.jsx omit companionAudioUrls and were affected: the RedGIFs embed branch (line 596) and the hidden prebuffer VideoPlayer (line 647). Both surfaces are fixed by this change, since both relied on VideoPlayer's destructure default. Hoisting the default to a module-level const gives the prop a single stable reference shared across all invocations; useMemo's deps now stabilise; the effect stops re-firing. Diff is exactly two lines: one new module const, one substitution in the destructure default. No other changes to VideoPlayer.jsx; the narrow off-limits suspension for issue 011/005 work stays narrow. See issues/005-redgifs-playback-loop.md for the full 2026-05-26 investigation evidence and the elimination of the original four hypotheses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reverts the instrumentation added in 6b4a0e9. The dep-comparator and effect-run counter served their purpose: they identified sanitizedCompanionAudioUrls as the unstable dep (~1500 fires/sec), eliminating all four original hypotheses in issue 005 and pointing at the destructure default. The fix landed in a4b9975. Pattern itself (effect-run counter + Object.is dep comparator) is preserved in git history at 6b4a0e9 for any future trace work. Lint warnings return to the pre-instrumentation baseline of 39. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(issue 005)
Records the lesson from the 2026-05-26 investigation of issue 005:
when a React component destructures a prop with a non-primitive
default literal (`= []`, `= {}`, `= () => {}`), JS evaluates that
default expression on every call, producing a new reference per
render. If that value feeds a useMemo or useEffect dep array, the
consumer invalidates on every render even though the semantic value
is unchanged. Fix: hoist the default to a module-level const.
Includes the canonical triage pattern (effect-run counter +
Object.is dep comparator) that uncovered the root cause, so future
investigations can follow the same playbook instead of rediscovering
it.
Future-Claude can grep memory.md for "destructure-default-non-primitive"
when triaging similar render loops.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
❌ Deploy Preview for nightfeed failed.
|
There was a problem hiding this comment.
Pull request overview
Fixes the RedGIFs lightbox playback runaway loop by stabilizing a React hook dependency that was being invalidated every render when a prop was omitted.
Changes:
- Hoists
companionAudioUrls’s default to a module-level constant inVideoPlayerto avoid a fresh array reference on every render. - Documents the root cause and the general “non-primitive destructure default” pitfall in the issue write-up and project memory.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| memory.md | Adds a documented pattern explaining how non-primitive destructure defaults can thrash hook deps and trigger render loops. |
| issues/005-redgifs-playback-loop.md | Adds investigation evidence and the identified root cause for issue 005. |
| frontend/src/components/VideoPlayer.jsx | Stabilizes companionAudioUrls default reference to prevent useMemo/useEffect dependency churn. |
Comment on lines
+24
to
+32
| **Actual root cause:** | ||
|
|
||
| `frontend/src/components/VideoPlayer.jsx:28` destructures the prop with a default value: | ||
|
|
||
| ```js | ||
| companionAudioUrls = [], | ||
| ``` | ||
|
|
||
| JavaScript evaluates default-expression `[]` fresh on every function invocation, so when a caller omits `companionAudioUrls`, the destructure produces a brand-new array reference on every render of VideoPlayer. |
|
|
||
| When a React component destructures a prop with a default value that is a non-primitive literal (`= []`, `= {}`, `= () => {}`, etc.), JavaScript evaluates that default expression on every function invocation. If the caller omits the prop, the destructure produces a brand-new reference every render. If that destructured value then feeds into a `useMemo` or `useEffect` dependency array, the consumer's `Object.is` check fails on every render and the memo factory or effect re-runs even though the semantic value is unchanged. Downstream `setState` calls in event handlers (e.g., `video.load()` firing `loadedmetadata` → `setVideoMetrics`) can close the loop and produce a runaway render cycle. | ||
|
|
||
| **Caught in the wild as issue 005:** `frontend/src/components/VideoPlayer.jsx:28` had `companionAudioUrls = []`. `LightboxModal.jsx`'s RedGIFs embed branch and prebuffer branch both omitted the prop. The default produced a fresh `[]` per render. The downstream `useMemo` invalidated, `sanitizedCompanionAudioUrls` became a new reference each render, the 17-dep `useEffect` re-fired ~1500x/sec, and `video.load()` in the cleanup spammed cancelled requests at `/api/external/redgifs/:id/stream`. The regular `v.redd.it` HLS path was unaffected because `LightboxModal` did pass a memoised `companionAudioUrls` to that VideoPlayer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.