Upload progress improvements #1077
Conversation
WalkthroughDesktop: refactors recording error handling, overhauls upload progress clamping/guards/logging, and changes auth header name. Web: adds optional supportsUploadProgress affecting DB insert, refactors CapCard rendering/guards, enhances desktop progress API validation/logging, and removes turbopack from dev script. Backend: deletes related videoUploads on video delete and removes an unused import. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Desktop as Desktop App
participant WebAPI as Web API (/api/desktop)
participant DB as DB (videos, videoUploads, videoUploadProgress)
rect rgba(230,245,255,0.6)
note over Desktop,WebAPI: Create video and get upload URL
Desktop->>WebAPI: POST /create (metadata)
WebAPI->>DB: Insert video
alt supportsUploadProgress = true
WebAPI->>DB: Insert videoUploads row
else
note right of WebAPI: Skip videoUploads insert
end
WebAPI-->>Desktop: { uploadUrl, videoId }
end
rect rgba(240,255,240,0.6)
note over Desktop,WebAPI: Upload with clamped progress
loop For each chunk
Desktop->>Desktop: Compute progress (clamp, guard total=0)
Desktop->>WebAPI: POST /progress { uploaded, total } with X-Cap-Desktop-Version
WebAPI->>WebAPI: Validate inputs, log
WebAPI->>DB: Upsert/Update progress
alt uploaded >= total and total > 0
WebAPI->>DB: Cleanup progress row
WebAPI-->>Desktop: Completed
else
WebAPI-->>Desktop: Progress updated
end
end
end
sequenceDiagram
autonumber
participant Service as Videos Service
participant DB as DB (videos, videoUploads)
note over Service,DB: Delete video and related uploads
Service->>DB: Execute Promise.all([delete videos, delete videoUploads])
DB-->>Service: Deletions resolved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (2)
141-151: Use useEffectMutation instead of useMutationClient mutations in apps/web must use useEffectMutation from @/lib/EffectRuntime; avoid direct useMutation. Replace deleteMutation to comply.
[Based on coding guidelines]
- const deleteMutation = useMutation({ - mutationFn: async () => { - await onDelete?.(); - }, - onError: (error) => { - console.error("Error deleting cap:", error); - }, - onSettled: () => { - setConfirmOpen(false); - }, - }); + const deleteMutation = useEffectMutation({ + mutationFn: () => + Effect.tryPromise(async () => { + await onDelete?.(); + }), + onError: (error) => { + console.error("Error deleting cap:", error); + }, + onSettled: () => { + setConfirmOpen(false); + }, + });
1-10: Add “use client” directive
Insert"use client";as the very first line in apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx to enable React hooks under Next.js App Router.
🧹 Nitpick comments (3)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (2)
538-547: Remove newly added inline commentInline comments in TS/TSX are disallowed in this codebase. Remove the commented-out class fragment.
[Based on coding guidelines]
- // uploadProgress && "opacity-30",
153-159: Avoid broad router.refresh in duplicate successPrefer targeted cache updates (setQueryData/setQueriesData) for affected queries instead of router.refresh().
[Based on coding guidelines]
apps/desktop/src-tauri/src/recording.rs (1)
269-276: Avoid unnecessary String cloneYou can move link directly into VideoUploadInfo; no need to clone.
- let link = app.make_app_url(format!("/s/{}", s3_config.id())).await; - info!("Pre-created shareable link: {}", link); + let link = app.make_app_url(format!("/s/{}", s3_config.id())).await; + info!("Pre-created shareable link: {}", link); Some(VideoUploadInfo { id: s3_config.id().to_string(), - link: link.clone(), + link, config: s3_config, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/recording.rs(2 hunks)apps/desktop/src-tauri/src/upload.rs(8 hunks)apps/desktop/src-tauri/src/web_api.rs(1 hunks)apps/web/actions/video/upload.ts(3 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(5 hunks)apps/web/app/api/desktop/[...route]/video.ts(3 hunks)apps/web/package.json(1 hunks)packages/web-backend/src/Videos/VideosRepo.ts(1 hunks)packages/web-backend/src/Videos/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/web-backend/src/Videos/index.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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:
packages/web-backend/src/Videos/VideosRepo.tsapps/desktop/src-tauri/src/web_api.rsapps/web/actions/video/upload.tsapps/desktop/src-tauri/src/upload.rsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsxapps/desktop/src-tauri/src/recording.rs
**/*.{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:
packages/web-backend/src/Videos/VideosRepo.tsapps/web/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.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:
packages/web-backend/src/Videos/VideosRepo.tsapps/web/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/web_api.rsapps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/recording.rs
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must be implemented in Next.js Server Actions under apps/web/actions; do not place AI calls elsewhere
Files:
apps/web/actions/video/upload.ts
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/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.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/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.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/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
🧬 Code graph analysis (6)
packages/web-backend/src/Videos/VideosRepo.ts (1)
packages/database/index.ts (1)
db(29-34)
apps/web/actions/video/upload.ts (3)
packages/utils/src/constants/plans.ts (1)
userIsPro(21-45)packages/database/index.ts (1)
db(29-34)packages/database/schema.ts (1)
videoUploads(648-654)
apps/desktop/src-tauri/src/upload.rs (2)
packages/web-domain/src/Video.ts (1)
UploadProgress(61-68)apps/desktop/src/utils/tauri.ts (1)
UploadProgress(453-453)
apps/web/app/api/desktop/[...route]/video.ts (2)
packages/database/index.ts (1)
db(29-34)packages/database/schema.ts (1)
videoUploads(648-654)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (2)
packages/ui-solid/src/ProgressCircle.tsx (1)
ProgressCircle(55-112)apps/web/components/VideoThumbnail.tsx (1)
VideoThumbnail(56-125)
apps/desktop/src-tauri/src/recording.rs (1)
apps/desktop/src-tauri/src/upload.rs (2)
create_or_get_video(443-505)id(77-79)
⏰ 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). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (16)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (6)
295-303: Z-index fix improves layeringAdding z-10 on the card container is correct and resolves overlay stacking issues.
310-319: Dropdown overlay stacking LGTMUsing z-20 for the action stack aligns the layering with other overlays.
361-400: Disable Download while uploadingGuarding the Download action when a cap has an active upload is correct and prevents broken flows.
414-423: Disable Duplicate while uploadingDisabling Duplicate during active uploads avoids duplicate data inconsistencies.
503-505: Prevent navigation while deletingPreventing navigation on Link when isDeleting is set avoids mid-flight route changes. Good.
508-551: Upload-progress overlay rendering is solidDedicated overlay with failure/active cases is clear and avoids thumbnail flicker. No correctness issues spotted.
apps/desktop/src-tauri/src/recording.rs (1)
254-268: Error propagation on video pre-create is correctmap_err with error! then early return simplifies control flow and avoids silent failures.
apps/desktop/src-tauri/src/upload.rs (8)
138-156: Progress clamping and zero-total guardsGood defensive checks; prevents divide-by-zero and over-reporting.
157-190: Debounce/skip redundant updates and only send when validAborting pending tasks and avoiding no-op updates reduces noisy writes. Immediate-send condition is correct.
203-216: Schedule delayed updates only with valid totalsAvoids server writes for unknown totals; appropriate.
220-255: Better observability on progress API errorsStatus + body logging is helpful for diagnosing server-side failures.
304-319: Guarded channel progress with zero-total handlingPrevents NaN progress values on the UI channel when size is unknown.
816-825: Zero-size file guard in realtime uploadCorrectly waits when recording hasn’t written data; errors if recording ended with empty file.
924-928: Early return on zero-size during chunk uploadPrevents invalid S3 part uploads and surfaces a clear error.
1141-1153: Post-chunk progress update and logging LGTMUsing current file_size for progress is acceptable; logs are useful during multipart.
apps/desktop/src-tauri/src/web_api.rs (1)
18-30: Header rename consistency confirmedBoth the desktop client (
web_api.rs) and server routes (video.ts) now exclusively use"X-Cap-Desktop-Version"with no remaining"X-Desktop-Version"references.
| if (result.rowsAffected === 0) { | ||
| console.log("No existing progress record, inserting new one:", { | ||
| videoId: videoIdRaw, | ||
| uploaded, | ||
| total, | ||
| }); | ||
| await db().insert(videoUploads).values({ | ||
| videoId, | ||
| uploaded, | ||
| total, | ||
| updatedAt, | ||
| }); | ||
| } else { | ||
| console.log("Updated existing progress record:", { | ||
| videoId: videoIdRaw, | ||
| uploaded, | ||
| total, | ||
| rowsAffected: result.rowsAffected, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix stale progress update handling when no rows are updated.
rowsAffected === 0 can mean “incoming update is stale” because of the lte(videoUploads.updatedAt, updatedAt) guard—not just “row missing”. In that case this new insert path will hit the table’s PK constraint and throw, breaking progress reporting. Guard the insert with an existence check (or similar strategy) so stale updates are ignored instead of crashing.
- if (result.rowsAffected === 0) {
- console.log("No existing progress record, inserting new one:", {
- videoId: videoIdRaw,
- uploaded,
- total,
- });
- await db().insert(videoUploads).values({
- videoId,
- uploaded,
- total,
- updatedAt,
- });
- } else {
+ if (result.rowsAffected === 0) {
+ const [existingProgress] = await db()
+ .select({ updatedAt: videoUploads.updatedAt })
+ .from(videoUploads)
+ .where(eq(videoUploads.videoId, videoId))
+ .limit(1);
+
+ if (existingProgress) {
+ console.log("Ignoring stale progress update:", {
+ videoId: videoIdRaw,
+ uploaded,
+ total,
+ existingUpdatedAt: existingProgress.updatedAt,
+ });
+ } else {
+ console.log("No existing progress record, inserting new one:", {
+ videoId: videoIdRaw,
+ uploaded,
+ total,
+ });
+ await db().insert(videoUploads).values({
+ videoId,
+ uploaded,
+ total,
+ updatedAt,
+ });
+ }
+ } else {
console.log("Updated existing progress record:", {
videoId: videoIdRaw,
uploaded,🤖 Prompt for AI Agents
In apps/web/app/api/desktop/[...route]/video.ts around lines 333 to 352, the
code treats result.rowsAffected === 0 as “no existing record” but that can also
mean the incoming update was stale due to the lte(updatedAt) guard; attempting
to unconditionally insert then hits the PK constraint and throws. Change the
branch so that before inserting you verify existence (e.g., SELECT count/exists
WHERE videoId = ?), and only INSERT when the row truly does not exist; otherwise
treat rowsAffected === 0 as a stale/no-op and skip the insert. Optionally
replace the two-step check+insert with an upsert/INSERT ... ON CONFLICT DO
NOTHING to avoid the race and PK error.
|
I have merged this into #1092 as it benifits from it's other work. |
This PR aims to address the issues that plagued the original rollout and get this feature live finally.
Changes:
UploadCapButton. This can be a separate PR and derisks the rollout.Instant mode progress is stuck at 0/0- Was issue with local development env S3 ports not longer matching.Sudio mode progress working???Future:
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation