Skip to content

fix(mobile): video previews survive re-signed URLs and a late-attached player item - #32196

Open
bartlomiejbloniarz wants to merge 1 commit into
pingdotgg:mainfrom
bartlomiejbloniarz:fix/video-preview-loader
Open

bartlomiejbloniarz wants to merge 1 commit into
pingdotgg:mainfrom
bartlomiejbloniarz:fix/video-preview-loader

Conversation

@bartlomiejbloniarz

@bartlomiejbloniarz bartlomiejbloniarz commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Problem

On mobile, video previews in PR descriptions often never appear. The video plays, but its first-frame preview stays black until the app restarts. Two bugs in the preview loader cause it:

  • Each preview request was tied to the video's signed URL. A re-signed URL, which comes with reconnects and playback, cancelled the extraction in progress and queued it again from the start. With frequent re-signing a preview never finished, while every attempt still made range requests.
  • On iOS, expo-video's replaceAsync resolves before the item is attached to the player. A frame requested before then comes back empty, without an error.

Change

  • An extraction that has started now finishes and caches its frame, so a request with a re-signed URL for the same video reuses it. Queued extractions are still skipped when their screen is left. Only one extraction runs at a time, so finishing one costs at most the existing 15-second cap.
  • The loader waits for expo-video's sourceChange event before requesting the frame. Both platforms emit it once the item is attached.

This adds no retries and lowers the number of requests.

Related PRs

This follows #25988, which kept before/after video tables intact in PR descriptions. #32195 fixes a separate iOS bug where a preview that does load is drawn cropped; the two PRs are independent. A fix for videos keeping their own aspect ratio will follow once these merge.

Scope and approval

A focused fix of an obvious bug: previews for attached videos fail to load. It changes only the mobile thumbnail loader.

Verification

  • vp test run src/lib/videoThumbnails.test.ts src/lib/filePreview.test.ts src/components in apps/mobile: passes. The new tests cover reusing a started extraction for a re-signed URL, waiting for the item to attach, and a clean timeout that does not block the next request. All three fail on current main.
  • vp run typecheck in apps/mobile, vp lint and vp fmt --check on the changed files: pass.
  • With signed URLs forced to refresh every second, the previous loader started 178 extractions in 30 seconds and showed none of three previews. With this change it started 9 and showed all three.
  • iOS simulator, cold start on current main and with this change: below. Android emulator, checked on an earlier main: previews already loaded there, and still do.

iOS — cold start, opening a PR with three attached videos (the two in the table are still drawn cropped; #32195 fixes that)

BeforeAfter
loader-ios-before.mp4
loader-ios-after.mp4

Claude Opus 5.5 in Claude Code, running in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 6, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 6ad7cd6

Macroscope's review found this PR approvable — Video previews now wait for player attachment and preserve in-flight thumbnail extraction across URL refreshes or navigation. The change is limited to transient preview handling, with bounded work and existing cleanup retained.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Thumbnail extraction now waits for the player source to become available and times out after 15 seconds if it does not. An extraction can complete after its caller aborts. Tests cover reuse, attachment, timeout, and cleanup.

Changes: Video thumbnail extraction

Layer / File(s) Summary
Extraction lifecycle and validation
apps/mobile/src/lib/videoThumbnails.ts, apps/mobile/src/lib/videoThumbnails.test.ts, apps/mobile/src/components/VideoThumbnailImage.tsx
Extraction waits for a non-null player source, times out after 15 seconds, and removes its source-change subscription during cleanup. An abort before extraction prevents it from starting; an abort during extraction does not discard a produced thumbnail. Tests cover extraction reuse, delayed attachment, timeout, and cleanup. The component comment now describes this behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 6ad7c

Video previews can reuse a completed frame after a URL refresh. No concrete issue remains that should delay merging after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title describes both main changes to mobile video previews.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description includes the problem, change, scope justification, verification results, before-and-after evidence, and agent details.
Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between b90e554 and 6ad7cd6.

Files selected for processing: 3

  • apps/mobile/src/components/VideoThumbnailImage.tsx
  • apps/mobile/src/lib/videoThumbnails.test.ts
  • apps/mobile/src/lib/videoThumbnails.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

This branch has not been deployed

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant