Abstract upload flow #1048
Abstract upload flow #1048
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughDelegates the multi-stage upload workflow to a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant B as UploadCapButton
participant L as legacyUploadCap
participant X as XHR (video/poster)
participant P as sendProgressUpdate
participant R as Router
U->>B: select file / click upload
B->>L: legacyUploadCap(file, ctx)
rect rgb(245,250,255)
note over L: parse, resize/convert, capture thumbnail
L->>X: upload video (XHR)
loop Video progress
X-->>L: progress %
L->>P: POST progress (batched)
end
L->>X: upload thumbnail (XHR)
loop Thumbnail progress
X-->>L: progress %
L->>P: POST progress
end
end
alt success (true)
L-->>B: true
B->>R: refresh()
B->>B: clear input / reset upload state
else failure (false/throw)
L-->>B: false / throw
B->>B: log error / clear upload state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
32-43: Don’t block non‑Pro uploads ≤5 min; gate after parsing duration.Current pre‑click gate forces upgrade for all non‑Pro users, but the server allows ≤300s uploads. Move gating after metadata parsing and only show the Upgrade modal when the duration exceeds 5 minutes.
@@ const handleClick = () => { if (!user) return; - - const isCapPro = userIsPro(user); - - if (!isCapPro) { - setUpgradeModalOpen(true); - return; - } - inputRef.current?.click(); }; @@ const handleChange = async (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (!file || !user) return; - const ok = await legacyUploadCap(file, folderId, setUploadStatus); + const isCapPro = userIsPro(user); + const ok = await legacyUploadCap( + file, + folderId, + setUploadStatus, + isCapPro, + () => setUpgradeModalOpen(true), + ); if (ok) router.refresh(); if (inputRef.current) inputRef.current.value = ""; }; @@ -async function legacyUploadCap( - file: File, - folderId: string | undefined, - setUploadStatus: (state: UploadStatus | undefined) => void, -) { +async function legacyUploadCap( + file: File, + folderId: string | undefined, + setUploadStatus: (state: UploadStatus | undefined) => void, + isCapPro: boolean, + onUpgradeRequired?: () => void, +) { @@ const duration = metadata.durationInSeconds ? Math.round(metadata.durationInSeconds) : undefined; + if (!isCapPro && duration && duration > 300) { + onUpgradeRequired?.(); + toast.error("Upgrade required to upload videos longer than 5 minutes."); + setUploadStatus(undefined); + return false; + } + setUploadStatus({ status: "creating" });Also applies to: 45-52, 84-88, 105-121
🧹 Nitpick comments (6)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (6)
341-341: Remove inline comments to comply with repository rules.Repo guidelines disallow inline/block/doc comments. These can be dropped; names are self‑explanatory.
- // Create progress tracking state outside React @@ - // Clear any existing pending task @@ - // Don't send completion update immediately - let xhr.onload handle it - // to avoid double progress updates @@ - // Schedule delayed update (after 2 seconds) @@ - // Guarantee final 100% progress updateAlso applies to: 356-361, 365-369, 419-421
321-325: Revoke thumbnail Object URL to avoid memory leaks.
thumbnailUrlis created viaURL.createObjectURLand never revoked.@@ - const thumbnailUrl = thumbnailBlob - ? URL.createObjectURL(thumbnailBlob) - : undefined; + const thumbnailUrl = thumbnailBlob + ? URL.createObjectURL(thumbnailBlob) + : undefined; @@ - setUploadStatus(undefined); - return true; + if (thumbnailUrl) URL.revokeObjectURL(thumbnailUrl); + setUploadStatus(undefined); + return true; } catch (err) { console.error("Video upload failed", err); + if (thumbnailUrl) URL.revokeObjectURL(thumbnailUrl); } setUploadStatus(undefined); return false;Also applies to: 491-499
343-349: Remove unusedlastUpdateTimefrom progress tracker.Field is never read; drop it and its assignment.
- const uploadState = { + const uploadState = { videoId: uploadId, uploaded: 0, total: 0, pendingTask: undefined as ReturnType<typeof setTimeout> | undefined, - lastUpdateTime: Date.now(), }; @@ - uploadState.total = total; - uploadState.lastUpdateTime = Date.now(); + uploadState.total = total;Also applies to: 354-355
225-225: Remove no‑oploadstartlisteners.They add noise and no behavior.
- testVideo.addEventListener("loadstart", () => {}); @@ - video.addEventListener("loadstart", () => {});Also applies to: 317-317
49-52: Optional: Prefer EffectRuntime mutations and targeted cache updates overrouter.refresh().Wrap server action calls with
useEffectMutationand update the relevant TanStack Query cache viasetQueryDatato avoid broad invalidations and improve UX.If you want, I can sketch the minimal
useEffectMutationwrapper and the cache touch points for the caps list.
1-82: Naming: consider kebab‑case filename.Repo convention suggests module filenames in kebab‑case. Rename to
upload-cap-button.tsxif consistent with the rest of the app.Please confirm whether caps components are standardizing on kebab‑case filenames; I’ll generate a safe rename patch if so.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
🧬 Code graph analysis (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
UploadStatus(11-33)apps/web/actions/video/upload.ts (1)
createVideoAndGetUploadUrl(149-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (10)
22-24: Remove unused prop type "grey".It's declared but unused; drop it to keep the signature clean.
- grey?: boolean;
49-52: Prefer targeted cache updates over router.refresh().Use useEffectMutation + setQueryData/setQueriesData to update relevant queries instead of a broad refresh.
Which queries should reflect the new upload? If you share their keys, I can sketch the precise cache updates.
490-498: Revoke object URL to prevent memory leak.thumbnailUrl is never revoked; free it on success/failure.
- setUploadStatus(undefined); - return true; + setUploadStatus(undefined); + if (thumbnailUrl) URL.revokeObjectURL(thumbnailUrl); + return true; @@ - } catch (err) { - console.error("Video upload failed", err); - } + } catch (err) { + console.error("Video upload failed", err); + if (thumbnailUrl) URL.revokeObjectURL(thumbnailUrl); + }
224-225: Remove no-op event listeners.These listeners do nothing and can be dropped.
- testVideo.addEventListener("loadstart", () => {}); + // removed no-op listener- video.addEventListener("loadstart", () => {}); + // removed no-op listenerAlso applies to: 316-317
342-355: Drop unused progress tracker field.lastUpdateTime is unused; remove the field and its assignments.
- const uploadState = { + const uploadState = { videoId: uploadId, uploaded: 0, total: 0, pendingTask: undefined as ReturnType<typeof setTimeout> | undefined, - lastUpdateTime: Date.now(), }; @@ - uploadState.lastUpdateTime = Date.now();
401-413: Throttle UI progress updates to reduce re-renders.xhr.upload.onprogress can fire very frequently; consider batching to ~100–200ms or via requestAnimationFrame.
149-166: Throttle conversion progress updates.Similarly, rate-limit setUploadStatus in onProgress to avoid excessive renders.
340-370: Remove newly added inline comments (repo rule: no comments).Delete the added comments to match the codebase guideline.
Also applies to: 419-419
84-88: Consider moving legacyUploadCap to a shared module.Aligns with the PR goal of reuse across Loom exporter and this button.
1-1: File naming: prefer kebab-case for module filenames.Rename to upload-cap-button.tsx per repo convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
🧬 Code graph analysis (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
UploadStatus(11-33)apps/web/actions/video/upload.ts (1)
createVideoAndGetUploadUrl(149-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
56-69: LGTM on the button UX/state.Disabling + spinner tied to isUploading is clear and consistent.
109-120: No action required — createVideoAndGetUploadUrl is a Server Action.apps/web/actions/video/upload.ts has "use server" at the top and exports createVideoAndGetUploadUrl, so importing/calling it from the client component apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx is valid.
This literally just breaks out the old flow into a new dedicated function so I can work on the Effect rewrite side by side. This should not change any logic.
Rewriting to Effect means our Loom exporter can share code with the UploadCapButton and it also will make the whole process more reliable and possibly even faster if using Effects concurrency primitives.
Summary by CodeRabbit
New Features
Improvements
Refactor