fix: don't restart the player when the 720p transform finalizes mid-review - #66
Conversation
|
@maria-rcks is attempting to deploy a commit to the Ping Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughVideo playback state is keyed by ChangesVideo playback flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VideoPage
participant getPlaybackSession
participant selectDashboardPlaybackPreferenceAfterOriginalLoad
participant VideoPlayer
VideoPage->>getPlaybackSession: load session for resolvedVideoId
getPlaybackSession-->>VideoPage: return video-scoped session
VideoPage->>selectDashboardPlaybackPreferenceAfterOriginalLoad: evaluate loaded original URL
selectDashboardPlaybackPreferenceAfterOriginalLoad-->>VideoPage: return playback preference
VideoPage->>VideoPlayer: render active playback URL and poster
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 624991a. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/routes/dashboard/-video.tsx (1)
557-566: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid unnecessary re-renders by returning the existing state object.
When
nextPreferencematches thecurrentPreference, returning a newly constructed object{ videoId: resolvedVideoId, source: nextPreference }will cause React to perform an unnecessary re-render because object references won't match. You can return the existingpreferenceobject when the resolved source hasn't changed.💡 Proposed refactor
setSourcePreference((preference) => { const currentPreference = preference?.videoId === resolvedVideoId ? preference.source : null; const nextPreference = selectDashboardPlaybackPreferenceAfterOriginalLoad({ currentPreference, startedWhileProcessing, originalUrl: result.url, }); + if (currentPreference === nextPreference) return preference; return nextPreference ? { videoId: resolvedVideoId, source: nextPreference } : preference; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/routes/dashboard/-video.tsx` around lines 557 - 566, Update the setSourcePreference callback around selectDashboardPlaybackPreferenceAfterOriginalLoad so it returns the existing preference object when nextPreference equals currentPreference; only construct a new { videoId, source } object when the resolved source actually changes, while preserving the existing fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/routes/dashboard/-video.tsx`:
- Around line 1173-1181: Update both affected Button elements in
app/routes/dashboard/-video.tsx at lines 1173-1181 and 1155-1162: remove
variant="outline" so they use the solid variant, and add font-bold rounded-none
to their classes. For the banner at lines 1173-1181, replace its background and
text classes with bg-[`#f0f0e8`] and text-[`#1a1a1a`].
---
Nitpick comments:
In `@app/routes/dashboard/-video.tsx`:
- Around line 557-566: Update the setSourcePreference callback around
selectDashboardPlaybackPreferenceAfterOriginalLoad so it returns the existing
preference object when nextPreference equals currentPreference; only construct a
new { videoId, source } object when the resolved source actually changes, while
preserving the existing fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 987a1f08-2f84-41b9-b37e-f34bfc24089a
📒 Files selected for processing (3)
app/routes/dashboard/-video.tsxsrc/lib/dashboardPlaybackSource.test.tssrc/lib/dashboardPlaybackSource.ts
| <div | ||
| className="flex flex-shrink-0 items-center justify-between gap-3 bg-[#2a2114] px-4 py-2 text-sm text-[#fff1d5]" | ||
| role="status" | ||
| > | ||
| <span>{playbackLoadError} Original playback is continuing.</span> | ||
| <Button variant="outline" size="sm" onClick={handleRetryPlaybackSession}> | ||
| Retry 720p | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align new UI elements with the coding guidelines.
Both newly added UI sections violate the required visual guidelines for banners and buttons. As per coding guidelines, backgrounds must use warm cream (#f0f0e8), primary text must use near-black (#1a1a1a), buttons must have solid backgrounds with bold text, and primary UI elements must not have rounded corners.
app/routes/dashboard/-video.tsx#L1173-L1181: Update the banner to usebg-[#f0f0e8]andtext-[#1a1a1a]. Removevariant="outline"from the button (falling back to a solid variant) and addfont-bold rounded-noneto its classes.app/routes/dashboard/-video.tsx#L1155-L1162: Removevariant="outline"from the button and appendfont-bold rounded-noneto its classes to adhere to the solid background and sharp edge constraints.
📍 Affects 1 file
app/routes/dashboard/-video.tsx#L1173-L1181(this comment)app/routes/dashboard/-video.tsx#L1155-L1162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/dashboard/-video.tsx` around lines 1173 - 1181, Update both
affected Button elements in app/routes/dashboard/-video.tsx at lines 1173-1181
and 1155-1162: remove variant="outline" so they use the solid variant, and add
font-bold rounded-none to their classes. For the banner at lines 1173-1181,
replace its background and text classes with bg-[`#f0f0e8`] and text-[`#1a1a1a`].
Source: Coding guidelines
While a video is still processing, the dashboard player falls back to the original upload. When Mux finalized the 720p asset mid-review, the reactive status flip made the playback selector demand the Mux session URL before it had been fetched, so the active URL passed through null, unmounting the player and restarting playback from zero. Now, when the original URL is acquired during processing, the session commits a video-scoped 'original' preference, so the player stays on the original source across the processing->ready transition until the reviewer explicitly switches quality or original playback fails. Fresh loads of already-ready videos still wait for the 720p session as before. Playback session and original URL state are scoped by video id so they can't leak across version navigation, and a failed background 720p session fetch now surfaces a Retry action without interrupting original playback.
624991a to
89f09fa
Compare
1 participant
What happened
A reviewer watching a freshly uploaded video (playing the Original source while Mux was still encoding) got kicked off the player the moment the 720p transform finalized. The page appeared to restart: the player unmounted, showed "Loading 720p stream...", then came back at 0:00 on the new source.
Why
While a video is
processing, the dashboard plays the original upload but the source preference silently defaults tomux720. When Convex reactively flipsvideo.statustoready,selectDashboardPlaybackUrlimmediately stops returning the (still perfectly good) original URL and returns the Mux session URL instead — which is stillnullbecausegetPlaybackSessionhasn't resolved yet. So the active URL goesoriginal → null → mux.VideoPlayeronly renders when the URL is truthy, so the null unmounts it and the remount starts from zero.The fix
originalsource preference (reusing the existingsourcePreferencestate — no new state machine). The playback URL then never passes through null: the reviewer stays on the original source across theprocessing → readyflip until they explicitly switch quality or original playback fails.The preference transition lives in a small pure helper (
selectDashboardPlaybackPreferenceAfterOriginalLoad) next to the URL selector so the regression is pinned by unit tests.Testing
bun test src/lib/dashboardPlaybackSource.test.ts— 8 pass, 0 fail, including the regression sequence: processing-on-original → ready-with-no-session → ready-with-session never returns null and never auto-switches.tsc --noEmit,eslint,prettier --checkon changed files — clean.Note
Medium Risk
Touches core dashboard video playback and source-selection timing; behavior is well covered by unit tests but end-to-end encoding transitions weren't verified in-browser.
Overview
Fixes reviewers getting kicked out of the player when a video flips from processing to ready while they're watching the Original upload.
The active URL was briefly going original → null → 720p because the default preference stayed on
mux720once Mux was ready, but the 720p session wasn't loaded yet—soVideoPlayerunmounted and restarted at 0:00. When the original URL is fetched during a processing session, the page now commits a video-scopedoriginalpreference viaselectDashboardPlaybackPreferenceAfterOriginalLoad, withsawProcessingVideoIdRefso a status flip mid-request doesn't drop that lock.Playback session and original URL state are keyed by
resolvedVideoIdso version switches don't reuse stale URLs/posters. If 720p session load fails while Original keeps playing, a non-blocking Retry 720p banner is shown. Unit tests cover the processing → ready sequence and preference rules.Reviewed by Cursor Bugbot for commit 89f09fa. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix player restart when 720p transform finalizes mid-review
videoIdinVideoPageto prevent cross-video state leakage when a transform completes during review.selectDashboardPlaybackPreferenceAfterOriginalLoadindashboardPlaybackSource.ts: locks playback to'original'only when the page started while the video was processing and an Original URL exists, leaving any existing preference unchanged.'processing'status viasawProcessingVideoIdRefto inform the preference decision without re-triggering on unrelated state changes.Macroscope summarized 89f09fa.
Summary by CodeRabbit
Bug Fixes
Tests