feat(web): fall back to raw upload when MP4 playback fails#1702
feat(web): fall back to raw upload when MP4 playback fails#1702richiemcilroy merged 4 commits intomainfrom
Conversation
Paragon SummaryThis pull request review analyzed 4 files and found no issues. The review examined code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. Paragon did not detect any problems in the current diff. Proceed with merge after your normal checks. This PR improves shared-link video playback by preferring the processed MP4, then automatically falling back to the raw upload if the MP4 is unavailable or fails, so playback is more reliable when the optimized asset is missing. It also makes source resolution more robust and adds unit coverage for the fallback flow. Key changes:
Confidence score: 5/5
4 files reviewed, 0 comments |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
richiemcilroy
force-pushed
the
feat/web-raw-playback-fallback
branch
2 times, most recently
from
9e8e48a to
ccd5ef7
Compare
richiemcilroy
force-pushed
the
feat/web-raw-playback-fallback
branch
from
ccd5ef7 to
235b392
Compare
|
@greptileai please review the PR :) |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| videoSrc, | ||
| rawFallbackSrc, | ||
| enableCrossOrigin, | ||
| preferredSource, |
There was a problem hiding this comment.
When upload processing completes, consider resetting preferredSource back to "mp4" (and hasTriedRawFallback to false) before invalidating, so a raw fallback switches back to the optimized MP4 once it becomes available.
| }; | ||
| }; | ||
|
|
||
| if (preferredSource === "raw") { |
There was a problem hiding this comment.
preferredSource === "raw" currently short-circuits and returns null if the raw probe fails, even if MP4 would resolve. If preferredSource is meant as a preference (not exclusive), consider trying raw first then falling back to the MP4 probe.
| @@ -443,12 +477,19 @@ export function CapVideoPlayer({ | |||
| ) { | |||
| setHasError(false); | |||
| void queryClient.invalidateQueries({ | |||
There was a problem hiding this comment.
When processing finishes (or upload completes), this can stay stuck preferring raw if we previously fell back. You probably want to reset to MP4 + invalidate both cache entries so we re-probe the processed URL.
| void queryClient.invalidateQueries({ | |
| setHasError(false); | |
| setPreferredSource("mp4"); | |
| setHasTriedRawFallback(false); | |
| void queryClient.invalidateQueries({ | |
| queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin], | |
| }); |
| }; | ||
| }; | ||
|
|
||
| if (preferredSource === "raw") { |
There was a problem hiding this comment.
If preferredSource is "raw" but the raw preview probe returns null (404/unplayable), we currently bail out without ever trying the MP4 probe. That can turn a transient raw-preview issue into a hard playback failure even when MP4 is available.
| if (preferredSource === "raw") { | |
| if (preferredSource === "raw") { | |
| const raw = await resolveRaw(); | |
| if (raw) { | |
| return raw; | |
| } | |
| } |
| const resolveRawPreviewKey = (video: Video.Video) => | ||
| Effect.gen(function* () { | ||
| const db = yield* Database; | ||
| const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId); |
There was a problem hiding this comment.
getBucketAccess happens before we know whether we even need S3 (when rawFileKey is already in DB). Small perf/readability win to defer it until after the DB check.
| const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId); | |
| const db = yield* Database; | |
| const [uploadRecord] = yield* db.use((db) => | |
| db | |
| .select({ rawFileKey: Db.videoUploads.rawFileKey }) | |
| .from(Db.videoUploads) | |
| .where(eq(Db.videoUploads.videoId, video.id)), | |
| ); | |
| if (uploadRecord?.rawFileKey) { | |
| return uploadRecord.rawFileKey; | |
| } | |
| if (video.source.type !== "webMP4") { | |
| return yield* Effect.fail(new HttpApiError.NotFound()); | |
| } | |
| const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId); |
Also worth double-checking: headObject(...).pipe(Effect.option) will treat any S3 failure (timeouts/credentials/etc) as “missing” and return a 404 here. If the intent is only to ignore “not found”, it may be better to only swallow that specific error and let other S3 errors bubble to the existing 500 mapping.
| void queryClient.invalidateQueries({ | ||
| queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin], | ||
| queryKey: [ | ||
| "resolvedSrc", | ||
| videoSrc, | ||
| rawFallbackSrc, | ||
| enableCrossOrigin, | ||
| preferredSource, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Once preferredSource flips to "raw", this invalidation only targets that cache entry. If the intent is to retry MP4 after processing completes, consider resetting the preference here and invalidating by the base key prefix so both variants get cleared.
| void queryClient.invalidateQueries({ | |
| queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin], | |
| queryKey: [ | |
| "resolvedSrc", | |
| videoSrc, | |
| rawFallbackSrc, | |
| enableCrossOrigin, | |
| preferredSource, | |
| ], | |
| }); | |
| setPreferredSource("mp4"); | |
| setHasTriedRawFallback(false); | |
| void queryClient.invalidateQueries({ | |
| queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin], | |
| }); |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const resolveRawPreviewKey = (video: Video.Video) => | ||
| Effect.gen(function* () { | ||
| const db = yield* Database; | ||
| const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId); | ||
| const [uploadRecord] = yield* db.use((db) => | ||
| db | ||
| .select({ rawFileKey: Db.videoUploads.rawFileKey }) | ||
| .from(Db.videoUploads) | ||
| .where(eq(Db.videoUploads.videoId, video.id)), | ||
| ); | ||
|
|
||
| if (uploadRecord?.rawFileKey) { | ||
| return uploadRecord.rawFileKey; | ||
| } | ||
|
|
||
| if (video.source.type !== "webMP4") { | ||
| return yield* Effect.fail(new HttpApiError.NotFound()); | ||
| } | ||
|
|
||
| const candidateKeys = [ | ||
| `${video.ownerId}/${video.id}/raw-upload.mp4`, | ||
| `${video.ownerId}/${video.id}/raw-upload.webm`, | ||
| ]; | ||
| const headResults = yield* Effect.all( | ||
| candidateKeys.map((key) => s3.headObject(key).pipe(Effect.option)), | ||
| { concurrency: "unbounded" }, | ||
| ); | ||
| for (let i = 0; i < candidateKeys.length; i++) { | ||
| const rawHead = headResults[i]; | ||
| if (Option.isSome(rawHead) && (rawHead.value.ContentLength ?? 0) > 0) { | ||
| return candidateKeys[i]; | ||
| } | ||
| } | ||
|
|
||
| return yield* Effect.fail(new HttpApiError.NotFound()); | ||
| }); |
There was a problem hiding this comment.
getBucketAccess call on the happy path
resolveRawPreviewKey unconditionally acquires an S3 client via S3Buckets.getBucketAccess(video.bucketId) at the very start of its generator, but when uploadRecord.rawFileKey is already set (the common case) that client is never used — the function returns the DB key immediately. Meanwhile getPlaylistResponse (the only caller) already holds its own s3 from its own getBucketAccess call at line 128.
The S3 client inside resolveRawPreviewKey is only needed for the headObject probe in the candidate-key fallback branch (the webMP4 path). Moving yield* S3Buckets.getBucketAccess(video.bucketId) to just before the candidateKeys block — after the early returns for the DB hit and the non-webMP4 guard — would skip the bucket lookup entirely for the common path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/playlist/route.ts
Line: 86-121
Comment:
**Redundant `getBucketAccess` call on the happy path**
`resolveRawPreviewKey` unconditionally acquires an S3 client via `S3Buckets.getBucketAccess(video.bucketId)` at the very start of its generator, but when `uploadRecord.rawFileKey` is already set (the common case) that client is never used — the function returns the DB key immediately. Meanwhile `getPlaylistResponse` (the only caller) already holds its own `s3` from its own `getBucketAccess` call at line 128.
The S3 client inside `resolveRawPreviewKey` is only needed for the `headObject` probe in the candidate-key fallback branch (the `webMP4` path). Moving `yield* S3Buckets.getBucketAccess(video.bucketId)` to just before the `candidateKeys` block — after the early returns for the DB hit and the non-`webMP4` guard — would skip the bucket lookup entirely for the common path.
How can I resolve this? If you propose a fix, please make it concise.
| Effect.gen(function* () { | ||
| const db = yield* Database; | ||
| const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId); | ||
| const [uploadRecord] = yield* db.use((db) => |
There was a problem hiding this comment.
Minor perf: resolveRawPreviewKey always calls getBucketAccess even when rawFileKey is present (common path). Moving it into the webMP4 probe branch avoids the extra bucket/client lookup.
| const [uploadRecord] = yield* db.use((db) => | |
| const resolveRawPreviewKey = (video: Video.Video) => | |
| Effect.gen(function* () { | |
| const db = yield* Database; | |
| const [uploadRecord] = yield* db.use((db) => | |
| db | |
| .select({ rawFileKey: Db.videoUploads.rawFileKey }) | |
| .from(Db.videoUploads) | |
| .where(eq(Db.videoUploads.videoId, video.id)), | |
| ); | |
| if (uploadRecord?.rawFileKey) { | |
| return uploadRecord.rawFileKey; | |
| } | |
| if (video.source.type !== "webMP4") { | |
| return yield* Effect.fail(new HttpApiError.NotFound()); | |
| } | |
| const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId); | |
| const candidateKeys = [ | |
| `${video.ownerId}/${video.id}/raw-upload.mp4`, | |
| `${video.ownerId}/${video.id}/raw-upload.webm`, | |
| ]; | |
| const headResults = yield* Effect.all( | |
| candidateKeys.map((key) => s3.headObject(key).pipe(Effect.option)), | |
| { concurrency: "unbounded" }, | |
| ); | |
| for (let i = 0; i < candidateKeys.length; i++) { | |
| const rawHead = headResults[i]; | |
| if (Option.isSome(rawHead) && (rawHead.value.ContentLength ?? 0) > 0) { | |
| return candidateKeys[i]; | |
| } | |
| } | |
| return yield* Effect.fail(new HttpApiError.NotFound()); | |
| }); |
| "postcss": "^8.4.31", | ||
| "react": "^19.1.1", | ||
| "react-dom": "^19.1.1", | ||
| "react": "19.2.4", |
There was a problem hiding this comment.
Quick sanity check: this PR pins react/react-dom + @types/* in packages/ui (and changes ranges from ^ to exact). If that’s not intentional scope-wise, might be worth reverting to ranges or making the versioning strategy consistent across the workspace so UI doesn’t drift from apps.
1 participant
Improves shared link playback when the processed MP4 is unavailable: playlist API resolves raw preview keys more reliably, the player prefers MP4 then retries with raw once on error. Adds unit coverage for source resolution and fallback behaviour
Greptile Summary
This PR improves shared-link playback reliability by adding a single-shot MP4→raw fallback at the player level and making the
raw-previewplaylist endpoint more resilient when therawFileKeyDB column is unpopulated.Key changes:
playback-source.ts: Adds apreferredSourceoption toresolvePlaybackSource(default"mp4"); when set to"raw"the MP4 probe is skipped entirely. Extracts the raw-resolution logic into a shared innerresolveRawclosure. ExportsshouldFallbackToRawPlaybackSourceas a pure predicate.CapVideoPlayer.tsx: AddspreferredSource/hasTriedRawFallbackstate. On a video error event, if the current source is MP4 and a raw fallback URL exists and hasn't been tried yet, the player switchespreferredSourceto"raw"— changing the React Query key and triggering a fresh resolution. Both states reset correctly whenvideoSrc/rawFallbackSrcchange.playlist/route.ts: ExtractsresolveRawPreviewKey, which first checksvideoUploads.rawFileKeyin the DB, then forwebMP4sources probes two candidate S3 paths in parallel.shouldFallbackToRawPlaybackSource.Confidence Score: 5/5
Safe to merge — fallback logic is correct, the single-shot guard prevents retry loops, and all edge cases are handled.
No P0 or P1 issues found. The one finding (redundant
getBucketAccesscall in the happy path) is a minor P2 style/performance observation that does not affect correctness. The React Query key design, state-reset wiring, Effect-based route refactor, and new unit tests all look solid.apps/web/app/api/playlist/route.ts — redundant S3 bucket access acquisition in
resolveRawPreviewKey.Important Files Changed
resolveRawPreviewKeyhelper that probes candidate S3 keys whenrawFileKeyis absent from the DB; minor redundancy:getBucketAccessis called twice for everyraw-previewrequest (once ingetPlaylistResponse, once inside the new helper).preferredSourceparameter andshouldFallbackToRawPlaybackSourcehelper; refactors raw-resolution into a shared innerresolveRawclosure — logic is correct and well-encapsulated.preferredSource/hasTriedRawFallbackstate and a single-shot fallback in the video error handler; React Query key correctly includespreferredSourceso the re-fetch is isolated; state resets onvideoSrc/rawFallbackSrcchanges are properly wired.shouldFallbackToRawPlaybackSource; coverage is adequate for the new logic surface.Sequence Diagram
sequenceDiagram participant Player as CapVideoPlayer participant Query as React Query participant Resolver as resolvePlaybackSource participant Route as playlist API participant Store as S3 Storage Player->>Query: queryKey includes preferredSource=mp4 Query->>Resolver: preferredSource=mp4 Resolver->>Route: probe videoType=mp4 Route-->>Resolver: 206 signed URL Resolver-->>Query: type=mp4 source resolved Query-->>Player: resolvedSrc.data set Player->>Player: video element loads MP4 Player->>Player: video error event fires Player->>Player: shouldFallbackToRaw returns true Player->>Player: setPreferredSource=raw, setHasTriedRawFallback=true Player->>Query: queryKey now includes preferredSource=raw Query->>Resolver: preferredSource=raw Resolver->>Route: probe videoType=raw-preview Route->>Store: DB lookup for rawFileKey alt rawFileKey in DB Store-->>Route: key found else webMP4 source type Route->>Store: headObject candidate paths Store-->>Route: matching key found end Route-->>Resolver: 206 signed URL Resolver-->>Query: type=raw source resolved Query-->>Player: resolvedSrc.data set Player->>Player: video element loads raw filePrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(web): fall back to raw upload when ..." | Re-trigger Greptile