From 6b4a0e9cf2b7b6e724a9d03caf91a79ae78c2371 Mon Sep 17 00:00:00 2001 From: Astro Date: Tue, 26 May 2026 19:19:13 +1000 Subject: [PATCH 1/5] chore(VideoPlayer): add temporary effect-run logging for root-cause trace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- frontend/src/components/VideoPlayer.jsx | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/frontend/src/components/VideoPlayer.jsx b/frontend/src/components/VideoPlayer.jsx index 2afa624..7c828f8 100644 --- a/frontend/src/components/VideoPlayer.jsx +++ b/frontend/src/components/VideoPlayer.jsx @@ -46,6 +46,9 @@ const VideoPlayer = forwardRef(function VideoPlayer( const hlsRef = useRef(null); const dashPlayerRef = useRef(null); const timeoutRef = useRef(null); + // TEMP issue 005 root-cause trace — remove after diagnosis + const effectRunCountRef = useRef(0); + const prevDepsRef = useRef(null); const [reloadNonce, setReloadNonce] = useState(0); const [videoMetrics, setVideoMetrics] = useState({ ready: false, isPortrait: true }); const directUrlOnly = Boolean(mp4Url && !hlsUrl && !dashUrl); @@ -97,6 +100,29 @@ const VideoPlayer = forwardRef(function VideoPlayer( }), []); useEffect(() => { + // TEMP issue 005 root-cause trace — remove after diagnosis + effectRunCountRef.current += 1; + const currentDeps = { + mp4Url, hlsUrl, dashUrl, sanitizedCompanionAudioUrls, hasAudio, sourceKind, + posterUrl, autoPlay, loop, prebufferOnly, allowUnmutedAutoplay, + preferredMuted, onMutedChange, onDiagnostics, reloadNonce, + shouldUseCompanionAudio, directUrlOnly + }; + if (prevDepsRef.current === null) { + console.log(`[VideoPlayer trace] effect run #${effectRunCountRef.current} (initial mount)`); + } else { + const prev = prevDepsRef.current; + const changed = {}; + for (const k of Object.keys(currentDeps)) { + if (!Object.is(prev[k], currentDeps[k])) { + changed[k] = { from: prev[k], to: currentDeps[k] }; + } + } + console.log(`[VideoPlayer trace] effect run #${effectRunCountRef.current} — changed deps:`, changed); + } + prevDepsRef.current = currentDeps; + // END TEMP + const video = videoRef.current; const companionAudio = companionAudioRef.current; if (!video) return undefined; From 4d104e45af410fbea73215273296de16bc68b563 Mon Sep 17 00:00:00 2001 From: Astro Date: Tue, 26 May 2026 19:33:09 +1000 Subject: [PATCH 2/5] docs(005): update with 2026-05-26 evidence; sanitizedCompanionAudioUrls 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 --- issues/005-redgifs-playback-loop.md | 44 +++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/issues/005-redgifs-playback-loop.md b/issues/005-redgifs-playback-loop.md index daf03cb..1585e3b 100644 --- a/issues/005-redgifs-playback-loop.md +++ b/issues/005-redgifs-playback-loop.md @@ -4,6 +4,50 @@ None. Standalone bug, parked from session work on 2026-05-08. +## 2026-05-26 evidence (root cause identified) + +Re-investigated under branch `fix/005-redgifs-playback` with temporary instrumentation on the 17-dep `useEffect` in `frontend/src/components/VideoPlayer.jsx`. The instrumentation logs an effect run counter and which deps changed between consecutive fires. + +**Observed:** + +- The loop fires at roughly 1500 effect runs per second (~7900 fires in a 5-second capture). +- `loadedmetadata` DOES fire (duration appears in the native controls bar), contra the original "never reaches `loadedmetadata`" assumption above. The loop is downstream of metadata, not upstream. +- The **only** dep that changes between fires is `sanitizedCompanionAudioUrls`. All 16 other deps (including `preferredMuted`, `onDiagnostics`, `mp4Url`, `sourceKind`, etc.) are stable. + +**Cross-reference against the original four hypotheses:** + +- Hypothesis #1 (`preferredMuted` feedback loop via `volumechange` → `onMutedChange` → parent `setPersistMuted`): **eliminated**. `preferredMuted` never appears in the changed-dep set. +- Hypothesis #2 (inline-computed prop with new ref per render): **partially right direction, wrong location**. The fresh literal is not in the parent's JSX; it's the default value in VideoPlayer's own destructure. +- Hypothesis #3 (`onDiagnostics` instability): **eliminated**. LightboxModal does not pass `onDiagnostics`, and the dep is stable in the trace. +- Hypothesis #4 (`current` reference instability): **eliminated**. Other deps tied to `current` (`mp4Url`, `sourceKind`, `posterUrl`) are stable in the trace. + +**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. + +`frontend/src/components/LightboxModal.jsx:596` — the RedGIFs embed `` call site — does **not** pass `companionAudioUrls`. The regular video call site at LightboxModal.jsx:576-591 does pass it (as a memoized prop on lines 157-160), which is why the v.redd.it HLS path does not loop. + +The unstable `companionAudioUrls` reference flows into `useMemo` (VideoPlayer.jsx:55-58): + +```js +const sanitizedCompanionAudioUrls = useMemo( + () => (Array.isArray(companionAudioUrls) ? companionAudioUrls.filter(Boolean) : []), + [companionAudioUrls] +); +``` + +React's `Object.is` check on `[companionAudioUrls]` fails every render, the memo factory re-runs, and `.filter(Boolean)` produces yet another fresh array. `sanitizedCompanionAudioUrls` is therefore a new reference every render, which invalidates the 17-dep `useEffect`. Cleanup runs `video.removeAttribute('src')` + `video.load()`, events fire, `setVideoMetrics` / diagnostics setState trigger a re-render, and the loop closes. + +The same path is also active for the prebuffer `` at LightboxModal.jsx:647, which also omits `companionAudioUrls`. The fix below resolves both surfaces at once. + +This is a **fifth hypothesis** — destructure-default-array-literal — that was not on the original list. + ## What's broken When a RedGIFs-embed post is opened in the lightbox, the `