Skip to content

fix: delay ObjectURL revocation to prevent failed downloads#622

Open
hobostay wants to merge 1 commit into
heygen-com:mainfrom
hobostay:fix/objecturl-revoked-too-early
Open

fix: delay ObjectURL revocation to prevent failed downloads#622
hobostay wants to merge 1 commit into
heygen-com:mainfrom
hobostay:fix/objecturl-revoked-too-early

Conversation

@hobostay
Copy link
Copy Markdown
Contributor

@hobostay hobostay commented May 5, 2026

Summary

  • Delay URL.revokeObjectURL from 0ms to 1000ms in frame capture download
  • setTimeout(..., 0) revokes the blob URL before the browser initiates the download, causing empty/failed downloads

Details

Affected file: packages/studio/src/App.tsx (line 875)

When capturing a frame, the code creates a blob URL, triggers a download via link.click(), then immediately schedules revokeObjectURL with setTimeout(..., 0). The 0ms timeout fires on the next microtask, before the browser has started reading from the blob, resulting in a failed or empty download.

Test plan

  • Capture a frame in the studio and verify the download completes successfully
  • Verify the blob URL is eventually cleaned up (no memory leak)

🤖 Generated with Claude Code

…oads

setTimeout(..., 0) revokes the blob URL on the next microtask,
before the browser has initiated the download triggered by
link.click(). This causes empty or failed downloads for frame
captures. Use a 1-second delay to ensure the download starts.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@miguel-heygen miguel-heygen self-requested a review May 5, 2026 01:30
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real race — setTimeout(..., 0) queues the revoke as a macrotask that can fire before the browser kicks off the navigation triggered by link.click(), especially on slower devices. 1000ms is a safe upper bound for download initiation. LGTM.

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive review at 24a45cb0@jrusso1020 already verified and approved.

Audited

  • packages/studio/src/App.tsx:875 (the setTimeout(..., 0)setTimeout(..., 1000))
  • CI: all required green ✓

Strength

James covered the race. setTimeout(..., 0) queues revokeObjectURL as a microtask that can fire before the synchronous-looking link.click() actually kicks off the browser's navigation/download machinery — especially on slow devices. 1000ms is a conservative upper bound.

Important — supersedes the ObjectURL portion of #795

#795 (Jefsky) ships the same fix (in useFrameCapture.ts rather than App.tsx — both are valid call sites of the same pattern), but bundles it with three unrelated changes and a broken LayersPanel.tsx new-file addition. Recommend landing this single-purpose PR and closing the ObjectURL portion of #795. I've flagged the same in #795's review.

Also note: this PR fixes the call site in App.tsx:875, but #795 fixes a SIMILAR pattern in packages/studio/src/hooks/useFrameCapture.ts:1. Worth a Rule 2 audit — grep "URL.revokeObjectURL" packages/studio/src/ and confirm both sites get the 1000ms treatment if the contract is "wait 1000ms before revoking a blob URL." Otherwise users hit the same race on frame capture but not on App.tsx, or vice versa.

Nit

  • Worth a comment in the code explaining why 1000ms specifically — the next maintainer will see setTimeout(..., 1000) and wonder if it can be tightened. A one-line // Give the browser time to initiate the download before revoking. 0ms races with link.click(). would save a future archaeology session.

Verdict

Verdict: COMMENT
Reasoning: Fix is correct (James verified). Audit sibling site useFrameCapture.ts per Rule 2 — #795 fixes that one, this PR fixes the App.tsx one; both call sites need the same treatment to fully close the contract. External-contributor PR — Vance check before merging.

— Vai

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants