Fix view tracking video ref to prevent stale references

[builder-io-integration]

Summary

Fixes anonymous/all-viewer view-count undercounting on Clips share pages by giving useViewTracking a correct dependency on the actual <video> element lifecycle, instead of a hand-rolled ref-identity workaround. Also fixes the same bug on embed.$shareId.tsx, which the original fix missed, and adds lifecycle tests.

Problem

useViewTracking received an inline videoRef getter-object recreated on every render of r.$recordingId.tsx / share.$shareId.tsx / embed.$shareId.tsx. Because that object was in the effect's dependency array, the tracking effect tore down and re-ran on every unrelated parent render. Since the video was already playing, no new native play event fired to restart the heartbeat interval that accumulates watch time — so watch time silently stayed near zero and views never crossed the 5s/75% counted-view threshold, for every viewer (anonymous and logged-in alike).

An initial fix memoized the ref object and layered manual "has this identity changed" bookkeeping into the hook to decide when to reattach. That approach grew fragile across several follow-up commits (StrictMode remounts, cross-session recordingId/durationMs staleness during cleanup, iframe-open dedup surviving unmount) and, as flagged in review, still had an unresolved bug: post() read videoRef.current live during a session's cleanup, which could already point at a different video (or null) by the time that cleanup ran.

Solution

Redesigned per review feedback: VideoPlayer now accepts an onVideoElementChange callback wired to the real <video> ref, so callers can hold the actual HTMLVideoElement in React state. useViewTracking now takes that element directly (videoEl) and depends on [recordingId, videoEl, trackOpenWithoutVideo, disabled] in a normal useEffect — React's own dependency comparison decides when to reattach, and each effect closure captures its own video/recordingId, so a cleanup's final flush always describes the session it belonged to. durationMs is deliberately excluded from the deps (it can load asynchronously without needing a reattach) and is read through a ref kept in sync every render.

Key Changes

• hooks/use-view-tracking.ts: rewritten to accept videoEl: HTMLVideoElement | null and use a standard effect dependency array instead of manual attachment-identity refs.
• components/player/video-player.tsx: added onVideoElementChange prop, wired via a callback ref alongside the existing internal videoRef.
• routes/r.$recordingId.tsx, routes/share.$shareId.tsx, routes/embed.$shareId.tsx: now hold the video element in useState and pass it to useViewTracking, instead of the unstable inline ref-getter object. embed.$shareId.tsx had the identical bug and was previously unaddressed.
• hooks/use-view-tracking.test.tsx (new): covers rerenders while playing, video replacement (Edit → Done), recordingId changes on a reused element, async Loom trackOpenWithoutVideo activation, unmount/final-flush attribution, and React StrictMode's dev mount/cleanup/remount cycle.

---

  

This PR was created by Tim Milazzo (tmilazzo@builder.io)

---

To clone this PR locally use the Github CLI with command gh pr checkout 2318

You can tag me at @BuilderIO for anything you want me to fix or change

[github-actions]

Visual recap — skipped

The visual recap job did not run for this pull request. This is informational only and does not block the PR.

Recap skipped for cb3d008: bot author (type=Bot).

[steve8708]

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

[builder-io-integration]

There was a problem with your request, please try again later. Error id: baffc841944245399e9733643cef4243

[steve8708]

@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable)

[builder-io-integration]

There was a problem with your request, please try again later. Error id: cc4e45b3e9f94796986e3b8f0866d99b

[builder-io-integration]

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Incremental Code Review Summary

The latest commit fixes both previously reported issues: iframe-open deduplication is cleared on unmount, and durationMsRef is updated only after the previous session’s cleanup so final completion percentages use the correct denominator. Those prior review threads were resolved before this review. The remaining change is standard risk because it manages shared listener/session state across Clips recording, share, and embed routes.

New Finding

• 🟡 Final flush can read the next session’s video element — while the old session’s recording ID and duration are now preserved, post() still resolves videoRef.current from the live options ref. During a video/recording transition, cleanup can therefore derive timestamp and end-state from the newly mounted video, or drop the old session’s final flush if the new player is temporarily absent. Capture the attached video element for each session’s cleanup/post path.

The prior remount, recording identity, duration, StrictMode, and iframe-open lifecycle fixes otherwise look correct. Browser verification was attempted, but executor sessions lacked Chrome automation tools and authenticated owner routes lacked test credentials; the dev server was healthy.

[builder-io-integration]

🟡 Use the attached video for the previous session flush

post() now uses the correct captured recording ID and session-scoped duration, but it still resolves videoRef.current from optsRef.current. When cleanup runs during a video/recording transition, that live ref can point to the new video (changing timestampMs/scrubbedToEnd) or be null while the old session still needs its final flush. Capture the video element when attaching the session and use that captured element for cleanup/post events.

Additional Info

Identified by incremental code review and independently confirmed by the browser-test planner's static trace. Live browser verification was blocked by missing Chrome automation tools and unavailable authenticated test credentials.

[builder-io-integration]

This looks like it was generated against an earlier commit (ff4534f95) — the redesign pushed right after in cb3d008e0 removed optsRef/videoRef entirely from this file.

Current code: post() reads video.currentTime/video.duration from const video = videoEl (line 132), a plain closure variable captured once per effect invocation — not a live ref lookup. Since the effect now depends on [recordingId, videoEl, trackOpenWithoutVideo, disabled], a session transition creates an entirely new closure with its own video, and the outgoing session's cleanup still closes over its own original video/recordingId, so there's no way for it to read a different session's element. grep -n \"optsRef\\|videoRef\" use-view-tracking.ts on the current HEAD returns zero matches.

See the full context in my PR comment on this redesign and the accompanying use-view-tracking.test.tsx (6 lifecycle tests, all passing) if useful.

[steve8708]

@builderio-bot My gut check: yes, we should fix the underlying problem—but I would not merge [PR #2318](#2318) as it stands.

The bug is real, though the original explanation is imprecise. The inline videoRef object changes on every parent render, causing useViewTracking to repeatedly tear down its effect. During playback, that cleanup clears the progress timer; because the video is already playing, no new play event restarts it. Watch time can therefore be undercounted.

• The latest head still has an unresolved correctness bug: cleanup for video A reads the live ref, which may already point to video B—or null—so A’s final tracking event can be corrupted or dropped.
• It grew from a small memoization change into a manual 92-line lifecycle state machine with several successive repair commits. This behavior is subtle enough that it needs focused tests.
• No tracking-hook lifecycle tests were added. Generic CI is green, but it doesn’t exercise these cases.
• It misses embed.$shareId.tsx, which has the same inline-ref bug and is arguably the most important anonymous-viewing surface.
• The PR description is now stale and substantially understates the final change.

My preferred fix would expose the actual HTMLVideoElement lifecycle from VideoPlayer, then have useViewTracking depend on that element plus the recording/session identity. That lets cleanup naturally retain the correct video and recording instead of juggling live refs and manual attachment bookkeeping.

I’d require tests covering rerenders while playing, video replacement/Edit → Done, recording changes, async Loom activation, unmount/final flush, and StrictMode.

So the non-technical person surfaced a legitimate issue. The technical solution just isn’t ready or complete yet.

[builder-io-integration]

Agreed on every point — this was the right call not to merge as-is, and I redesigned it instead of patching further.

The videoRef.current live-read bug (real, unresolved): confirmed. post() was reading videoRef.current fresh via optsRef.current instead of the video element captured when that session's listeners were attached. During a session transition, that meant a cleanup's final flush could read a different video's currentTime/duration (or null) rather than its own. This is now structurally impossible — post() closes over the actual video element passed to that specific effect invocation.

The lifecycle state machine (real complexity problem): agreed, hasAttachedRef/attachedVideoRef/attachedRecordingIdRef/attachedTrackOpenRef was reimplementing what useEffect's dependency array already does correctly, and every "fix" was a symptom of that. Implemented your suggested approach instead:

• VideoPlayer now exposes the real HTMLVideoElement lifecycle via a new onVideoElementChange callback prop, wired through a callback ref alongside its existing internal videoRef.
• The three routes hold that element in useState (updates only when the DOM node actually changes — mount/unmount/swap — never on unrelated renders).
• useViewTracking now takes videoEl: HTMLVideoElement | null directly and depends on [recordingId, videoEl, trackOpenWithoutVideo, disabled] in one ordinary useEffect. React's dependency comparison — not hand-rolled bookkeeping — decides when to reattach, and each effect closure naturally captures its own video/recordingId, so a cleanup's flush can't ever describe a session it doesn't belong to.
• durationMs is deliberately excluded from the deps (it can resolve asynchronously without warranting a reattach) and is read through a small ref kept in sync every render — the one remaining "live ref" in the file, and it's justified since it's read-only data, not something whose identity gates attach/detach.
• The openTrackedRecordingRef StrictMode-dedup guard survives from the very first version basically unchanged, since it turns out to be correct as originally written — it only needed to not be manually reset, which the old code's unmount cleanup was doing.

embed.$shareId.tsx (real miss): fixed — it had the exact same inline-getter pattern and wasn't touched by the original PR. Confirmed via grep that all three player routes (r.$recordingId.tsx, share.$shareId.tsx, embed.$shareId.tsx) now use the same useState + onVideoElementChange pattern.

Tests (real gap): added app/hooks/use-view-tracking.test.tsx with 6 tests, run against a real DOM <video> element (happy-dom + react-dom/client + fake timers, matching this repo's existing video-player.test.tsx conventions) — all passing:

• rerenders while playing (watch time keeps accumulating, doesn't reset)
• video replacement / Edit → Done (fresh session, no counter bleed)
• recordingId change on a reused element (final flush attributed to the outgoing recording, not the incoming one)
• async Loom trackOpenWithoutVideo activation (fires exactly once, after it resolves true)
• unmount / final flush (correct recordingId and accumulated watch time)
• React StrictMode's dev mount→cleanup→remount cycle (no duplicate view-start POST)

Full template test suite (695 tests, including the existing video-player.test.tsx) and typecheck both pass after the onVideoElementChange addition.

PR description: rewritten to describe the actual current diff instead of the original memoization-only framing.

Pushed as cb3d008e0. Only the standing human-review requirement is outstanding — happy to walk through any part of the redesign in more detail if useful.

[builder-io-integration]

There was a problem with your request, please try again later. Error id: daafa6471f464cf08a92c27bc6e38f0d

[shomix]

Tested this out locally. things are looking good so am going to merge it