fix(studio): release retained preview resources - #2924
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Well-targeted PR — each of the three fixes maps cleanly onto a retainer trace in #2910 and the code changes match the PR-body summary. The .bind(win.gsap) / .bind(tl) → direct-owner refactor is exactly what the "7 nested bound_this" chain called for; the off-DOM Image() probe drops the hidden <img> React Fiber that was the root of the pending-activity retainer; the single releaseRuntimePreview closes the multi-exit-path bug where stop() was doing the restore inline and preview-failure and unmount were skipping it. Tests scaffolded solidly around unmount + timeout paths.
A few things to surface.
Concerns
releaseRuntimePreviewcoverage — only one of its three call sites is under test. The PR body highlights that it's called on stop, preview failure, and unmount, and centralisation is the fix. Only the unmount path is asserted (useGestureRecording.test.tsx). The stop() path (:428) has no test verifying element styles and CSS-var offset are restored end-to-end, and the preview-failure catch (:399-401) has no test at all. Because all three call sites now funnel through one function, a regression that breaks one breaks all three — but a regression that makes stop() skip the call (e.g. an early-return added ahead of it) would only fail a test that exercises stop(). One test per exit path (or a table-driven "exits from state X" harness) would lock the guarantee the PR-body-title makes.- Retainer-diff verification checkbox is unchecked in the PR body. The success criterion for #2910 is a heap-snapshot delta showing the two named chains (hidden-img via pending activities, and gsap.globalTimeline via 7 bound_this) no longer appearing as retainers. Unit tests can only show the code paths run — not that Blink's ThreadState still holds a reference. Worth attaching before/after retainer traces or a heap-snapshot object count delta before merge; the whole investigation has been evidence-first and this is the last piece.
Questions
- Preview-failure UX after
releaseRuntimePreview. Instop(), the runtime teardown happens as part of the user's explicit exit. In the preview-failure catch at:399, the change now restores visibility + translate + CSS-var offset + clears GSAP inline props while recording continues. The comment says "Preview failed — disable it for the rest of the gesture (recording continues)" — after the change, the element visually snaps back to its pre-recording position, but the user's pointer stream is still being captured against the old baseline. Intentional (user sees "preview broke, my recording continues"), or should the failure path leave the element in situ and only null the runtime so the failed preview state persists visibly until stop()? Not a bug — just want to confirm this is the deliberate UX call.
Nits
studioHelpers.ts—finalizecloses overtimeoutbefore itsconstdeclaration. In both the image path (:351-370) and the video path (:378-398),finalizereferenceswindow.clearTimeout(timeout)beforeconst timeout = window.setTimeout(...). Legal via closure (arrow-fn body doesn't evaluate until call, by which pointtimeoutis assigned), but it's a temporal-dead-zone reference at the pointfinalizeis written. Trivial reorder — declarelet timeout: number;first, assign it below — reads more obviously to future editors.img.src = ""per HTML spec resolves to the document base URL and technically fires a fetch for that URL, though every modern browser aborts the pending decode as the observable effect. If oxlint or a future codemod flags this,img.removeAttribute("src")has the same cancel semantics without the base-URL round trip. Not blocking — the pattern is common and works.
What I didn't verify
- Whether the
useMountEffectstill at the top ofCompositionThumbnail.tsxand the newuseEffect([url])compose cleanly whenpreviewUrl/seekTime/duration/selectorprop combinations change rapidly during scrubbing. The new effect keyed on the derivedurlshould recompute and abort correctly on each change, but the reset viasetLoaded(false)at effect start will flash the thumbnail area to empty between URL swaps. If scrubbing is prone to sub-100ms URL changes, worth eyeballing whether the flicker is user-visible. - No heap-snapshot repro from this side — trusting the PR-body claim that the traces map to these three surfaces.
Otherwise clean. LGTM from my side, leaving as a comment for stamps to route to <@james-russo-og>.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 92b385d.
Three targeted fixes, each mapped to a retainer chain in #2910. All three look correct.
Retainer 1 — hidden <img> in pending-activities queue (CompositionThumbnail.tsx)
- Moving the probe from a rendered
<img class="hidden">to an off-DOMnew Image()insideuseEffectremoves the React Fiber that Blink was holding via the pending-activity bucket. ✓ - Cleanup does the right dance:
cancelled = trueguard-first, then null both handler props, thenprobe.src = ""to abort the in-flight decode. Even if the browser dispatchedonloadto the task queue before cleanup ran, thecancelledclosure guard prevents a stalesetLoaded. ✓ - The visible tile
<img>s below{loaded && ...}only mount after the probe resolves, so they don't sit unresolved in the pending-activity queue during a slow-network URL — that was the specific mechanism the retainer trace named. ✓ useEffectdisable comment is appropriate — this is external-system sync (image probe), the exact case the rule reserves for effect use.
Retainer 2 — GSAP references held via bound_this chain (useGestureRecording.ts)
- Two independent changes here doing complementary work:
- Replacing
tl.seek.bind(tl)/win.gsap.set.bind(win.gsap)with directtimeline: tl/gsap: win.gsapobject references removes Studio's boundary-layer contribution to thebound_thischain the trace showed rooted atindex-CISVI7UC.js:284. The GSAP-internal nested binds aren't Studio-fixable, but Studio no longer adds the final link that anchored them to aV8EventListener. - Extracting
releaseRuntimePreview()and wiring it into the unmount effect is the actual leak-fix — pre-PR the unmount effect only nulledisRecordingRefand ranr.cleanup;r.runtimewas never nulled on unmount-during-recording, sotl+win.gsap+ iframe window survived component teardown viarefs.current.runtime. Nowr.runtime = nullruns on every exit path.
- Replacing
- Small semantic drift I traced and cleared: the old inline path in
stopRecordingre-readr.runtime?.elementfor the offset-restoration block. New code captureselementonce. No sync path between the blocks could nullr.runtime, so behaviorally identical. - Behavior change on the pointermove
try/catchfallback — the failed-preview path now restores visibility/translate/offset/clearProps in addition to nulling runtime, instead of leaving the element visually stuck at preview state. PR body calls this out ("release … on stop, preview failure, and unmount"), so intentional. Correct.
Retainer 3 (dropped-asset probes, adjacent — studioHelpers.ts)
- Old code left the underlying image/video request pending when the 3s timeout fired:
setTimeout → resolve(null)but noimg.src = "". Newfinalize()closure withsettledguard nulls handlers and emptiessrc(video does the extra.load()aftersrc = "", which is the correct HTMLMediaElement abort dance). ✓ settledguard is redundant on the happy path (handlers self-remove on fire in the old{once: true}pattern; now-nulled on the new pattern) but cheap and defends the "timeout races with actual load" corner. Fine.
Tests
useGestureRecording.test.tsxcovers the unmount-during-recording path end to end (iframe + fake gsap + fake timeline + fakecancelAnimationFrame+ saved-style-restoration assertions). Would catch regression on the unmount wiring.CompositionThumbnail.test.tscovers both the successful probe → tile render and the unmount → probe abort paths.studioHelpers.test.tscovers image + video timeout → resolves null +src = ""+load()for the video path.- One small coverage gap: no explicit test for the URL-change re-run of the
CompositionThumbnaileffect (dep array[url]should abort the previous probe before starting the new one). Not blocking — the abort logic is exactly the same as the unmount path, which is tested.
Nit only: studioHelpers.test.ts line 428 mocks probe.addEventListener: vi.fn() on the image probe, but production code no longer touches addEventListener (moved to .onload / .onerror properties). Dead mock property, harmless.
CI green across the required matrix (CLI smoke, Studio load smoke, Test, Typecheck, Windows tests, preview parity, Fallow audit).
Approving.
— Via
What
Why
The retainer traces in #2910 map to two independent Studio lifetimes: a hidden image left in the browser pending-activity queue, and document listeners retaining recording state that held bound GSAP and timeline methods. The neighboring dropped-asset dimension probes also resolved their timeout without aborting the underlying media request.
How
Media probes now own explicit cancellation and handler teardown. Gesture recording uses a single releaseRuntimePreview path that restores temporary styles, clears preview properties, releases runtime references, and is called by every exit path.
Test plan
Closes #2910