Upload Progress in UI #901
Conversation
WalkthroughReplace event-based upload progress with Tauri Channel streaming on desktop; add server-side upload progress tracking (new video_uploads table and POST /progress); surface polling-based upload progress in web UI players and dashboard; plus small clone/conditional refactors in rendering and camera code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Desktop UI
participant Tauri as Tauri (invoke)
participant Uploader as Desktop uploader (Rust)
participant API as Web API /api/desktop/video/progress
User->>UI: Start export & upload
UI->>UI: create Channel<UploadProgress>(onProgress)
UI->>Tauri: invoke upload_exported_video(path, mode, channel)
Tauri->>Uploader: upload_video(..., Some(channel.clone()))
loop chunked read & upload
Uploader->>UI: channel.send({progress})
Uploader->>API: POST /progress {videoId, uploaded, total, updatedAt}
end
Uploader-->>Tauri: Result<UploadedVideo>
Tauri-->>UI: UploadResult
UI->>UI: Update UI (progress complete)
sequenceDiagram
autonumber
actor Viewer
participant Page as Web Player Page
participant RPC as GetUploadProgress RPC
participant DB as video_uploads
Viewer->>Page: open video page
loop adaptive poll (1s..30s)
Page->>RPC: GetUploadProgress(videoId)
RPC->>DB: SELECT uploaded,total,updatedAt
DB-->>RPC: row or null
alt uploading
RPC-->>Page: Some{uploaded,total,updatedAt}
Page->>Page: show ProgressCircle, hide controls
else none or stale
RPC-->>Page: None
Page->>Page: normal UI / show failure
end
end
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: 1
🧹 Nitpick comments (4)
apps/desktop/src-tauri/src/lib.rs (2)
1037-1040: Consider deriving Clone for better performance and simpler logic.The
UploadProgressstruct could benefit from derivingCloneto avoid potential issues with move semantics when passing to channels.-#[derive(Serialize, Type, Debug, Clone)] +#[derive(Serialize, Type, Debug, Clone)] pub struct UploadProgress { progress: f64, }
1078-1078: Consider error handling for channel send operations.While
.ok()is used to ignore send errors (which is reasonable when the receiver might disconnect), consider logging these failures for debugging purposes in production environments.-channel.send(UploadProgress { progress: 0.0 }).ok(); +if let Err(e) = channel.send(UploadProgress { progress: 0.0 }) { + trace!("Failed to send upload progress: {}", e); +}Also applies to: 1117-1117, 1122-1122
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsx (1)
250-263: Consider extracting the channel callback for better testability.The progress calculation and channel callback could be extracted into a separate function for improved testability and reusability.
+const createProgressChannel = (setProgress: (value: number) => void) => { + return new Channel<UploadProgress>((progress) => + setProgress(Math.round(progress.progress * 100)) + ); +}; const [progress, setProgress] = createSignal(0); const reupload = createMutation(() => ({ mutationFn: async () => { setProgress(0); return await commands.uploadExportedVideo( props.recording.path, "Reupload", - new Channel<UploadProgress>((progress) => - setProgress(Math.round(progress.progress * 100)), - ), + createProgressChannel(setProgress), ); }, onSettled: () => setProgress(0), }));apps/desktop/src/routes/editor/ShareButton.tsx (1)
56-65: Consider handling potential Channel errors.While the channel callback is straightforward, consider adding error boundaries or try-catch blocks around the state updates to prevent potential crashes.
const uploadChannel = new Channel<UploadProgress>((progress) => { console.log("Upload progress:", progress); - setUploadState( - produce((state) => { - if (state.type !== "uploading") return; - state.progress = Math.round(progress.progress * 100); - }), - ); + try { + setUploadState( + produce((state) => { + if (state.type !== "uploading") return; + state.progress = Math.round(progress.progress * 100); + }), + ); + } catch (error) { + console.error("Error updating upload progress:", error); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/lib.rs(6 hunks)apps/desktop/src-tauri/src/recording.rs(1 hunks)apps/desktop/src/routes/(window-chrome)/settings/recordings.tsx(4 hunks)apps/desktop/src/routes/editor/ShareButton.tsx(2 hunks)apps/desktop/src/utils/tauri.ts(3 hunks)apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx(2 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(2 hunks)crates/recording/src/feeds/camera.rs(1 hunks)crates/rendering/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/recording/src/feeds/camera.rs
- apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx
- apps/web/app/s/[videoId]/_components/ShareVideo.tsx
- crates/rendering/src/lib.rs
- apps/desktop/src-tauri/src/recording.rs
- apps/desktop/src/utils/tauri.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsxapps/desktop/src/routes/editor/ShareButton.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsxapps/desktop/src/routes/editor/ShareButton.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
**/*.{ts,tsx}: Use Biome to format/lint TypeScript with a 2-space indent
TypeScript file names should be kebab-case (e.g., user-menu.tsx)
Files:
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsxapps/desktop/src/routes/editor/ShareButton.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsxapps/desktop/src/routes/editor/ShareButton.tsx
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/lib.rs
{crates,apps/cli,apps/desktop/src-tauri}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{crates,apps/cli,apps/desktop/src-tauri}/**/*.rs: Rust code must be formatted with rustfmt and adhere to workspace clippy lints
Rust module files should use snake_case names
Files:
apps/desktop/src-tauri/src/lib.rs
{crates,apps/cli,apps/desktop/src-tauri}/**/{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rust tests should live in src (inline/unit) or tests (integration) directories
Files:
apps/desktop/src-tauri/src/lib.rs
🧠 Learnings (2)
📚 Learning: 2025-08-25T10:58:06.142Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.142Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use tanstack/solid-query for server state in the desktop app
Applied to files:
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsx
📚 Learning: 2025-08-25T10:58:06.142Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.142Z
Learning: Applies to apps/desktop/src-tauri/**/*.rs : Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Applied to files:
apps/desktop/src-tauri/src/lib.rs
🧬 Code graph analysis (3)
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
commands(7-269)UploadProgress(453-453)packages/ui-solid/src/ProgressCircle.tsx (1)
ProgressCircle(55-112)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src/utils/tauri.ts (1)
UploadProgress(453-453)
apps/desktop/src/routes/editor/ShareButton.tsx (4)
apps/desktop/src/utils/tauri.ts (3)
UploadProgress(453-453)exportVideo(62-64)commands(7-269)apps/desktop/src/utils/export.ts (1)
exportVideo(4-13)apps/desktop/src/routes/editor/Header.tsx (1)
RESOLUTION_OPTIONS(34-38)apps/desktop/src/routes/editor/context.ts (1)
meta(308-310)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
apps/desktop/src-tauri/src/lib.rs (3)
74-74: LGTM! Correct import for Channel-based progress.The addition of
ipc::Channelimport aligns with the transition from event-based to channel-based progress reporting, following the Tauri best practices.
1052-1056: LGTM! Clean channel integration for progress reporting.The function signature properly integrates the
Channel<UploadProgress>parameter, enabling real-time progress streaming to the frontend.
2314-2320: LGTM! Cleaner conditional logic.The refactored conditional using
if *focused && let Ok(window_id) = window_id && window_id.activates_dock()is more idiomatic and improves readability.apps/desktop/src/routes/(window-chrome)/settings/recordings.tsx (2)
1-1: LGTM! Proper imports for channel-based progress.The imports are correctly updated to include
ProgressCircle,Channel, andUploadProgresstype for the new progress reporting mechanism.Also applies to: 9-9, 25-30
273-278: LGTM! Clean progress UI implementation.The conditional rendering properly shows either the
ProgressCirclewith actual progress or the rotation icon, providing clear visual feedback to users.apps/desktop/src/routes/editor/ShareButton.tsx (3)
4-4: LGTM! Proper Channel import for progress tracking.The imports correctly include
Channelfrom Tauri API and theUploadProgresstype.Also applies to: 11-11
73-93: LGTM! Clean export video integration with progress callback.The export video call properly integrates with the progress callback for rendering state updates.
112-117: LGTM! Proper error handling for upload results.The error handling correctly checks for different failure states and provides appropriate user feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/web/app/api/upload/[...route]/multipart.ts (1)
279-296: Don’t gate progress cleanup on rowsAffected; always derive videoId from fileKey and verify ownership explicitly.
- Using
body.videoIdrisks mismatching the DB row you update/delete vs. the S3fileKey. Always trustvideoIdFromFileKeyor reject mismatches.- Deleting
video_uploadsonly whenrowsAffected > 0is brittle: if no metadata fields change (or values are identical), your update can be a no-op and the progress row won’t be cleared, leaving the UI stuck.- Fix: derive
videoIdfromfileKey, optionally reject mismatch ifbody.videoIdis present, explicitly check ownership via a SELECT, then delete progress unconditionally for owned rows. Optionally coerce numeric fields to numbers before update.Apply this diff within the current block:
- const videoId = "videoId" in body ? body.videoId : videoIdFromFileKey; - if (videoId) { - const result = await db() - .update(videos) - .set({ - duration: updateIfDefined(body.durationInSecs, videos.duration), - width: updateIfDefined(body.width, videos.width), - height: updateIfDefined(body.height, videos.height), - fps: updateIfDefined(body.fps, videos.fps), - }) - .where(and(eq(videos.id, videoId), eq(videos.ownerId, user.id))); - - // This proves authentication - if (result.rowsAffected > 0) - await db() - .delete(videoUploads) - .where(eq(videoUploads.videoId, videoId)); - } + // Source of truth: derive from fileKey to prevent mismatch with body.videoId + const videoId = videoIdFromFileKey; + if ("videoId" in body && body.videoId !== videoIdFromFileKey) { + console.warn( + `videoId mismatch: body.videoId=${body.videoId} fileKey=${fileKey}`, + ); + return c.json({ error: "videoId mismatch" }, 400); + } + if (videoId) { + // Prove ownership explicitly + const owned = await db() + .select({ id: videos.id }) + .from(videos) + .where(and(eq(videos.id, videoId), eq(videos.ownerId, user.id))) + .limit(1); + if (!owned.length) { + console.warn( + `Refusing to update/delete progress for unowned videoId=${videoId}`, + ); + } else { + // Optional: coerce numeric fields if present + const duration = + body.durationInSecs !== undefined + ? Number(body.durationInSecs) + : undefined; + const width = + body.width !== undefined ? Number(body.width) : undefined; + const height = + body.height !== undefined ? Number(body.height) : undefined; + const fps = body.fps !== undefined ? Number(body.fps) : undefined; + + // Update only when any metadata is provided + if ( + duration !== undefined || + width !== undefined || + height !== undefined || + fps !== undefined + ) { + await db() + .update(videos) + .set({ + duration: updateIfDefined(duration, videos.duration), + width: updateIfDefined(width, videos.width), + height: updateIfDefined(height, videos.height), + fps: updateIfDefined(fps, videos.fps), + }) + .where(eq(videos.id, videoId)); + } + // Always clear progress for owned video after complete + await db() + .delete(videoUploads) + .where(eq(videoUploads.videoId, videoId)); + } + }apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
461-484: Make overlay non-blocking to preserve navigationThe overlay's absolute positioning with
z-50blocks the underlying Link from functioning. Addpointer-events-noneto allow click-through while keeping the visual overlay.- <div className="flex absolute inset-0 z-50 justify-center items-center bg-black rounded-t-xl"> + <div className="flex absolute inset-0 z-50 justify-center items-center bg-black/70 rounded-t-xl pointer-events-none">Consider also making the overlay slightly translucent (
bg-black/70) so users can still see the thumbnail beneath.apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
337-344: Use locally captured values for final progress updateThe
xhr.onloadhandler reads fromuploadStatewhich may be stale due to React state batching. Small uploads might not triggeronprogress, leaving the state uninitialized.Store the required values (videoId, file size) before the upload starts and use them in
onload:+ const finalVideoId = uploadId; + const finalTotal = optimizedBlob.size; await new Promise<void>((resolve, reject) => { const xhr = new XMLHttpRequest(); // ... existing code ... xhr.onload = () => { if (xhr.status >= 200 && xhr.status < 300) { - sendProgressUpdate(uploadId, uploadState.total, uploadState.total); + sendProgressUpdate(finalVideoId, finalTotal, finalTotal); resolve(); } else { reject(new Error(`Upload failed with status ${xhr.status}`)); } };apps/web/app/api/desktop/[...route]/video.ts (1)
294-301: Add check for existing row before insert to prevent race conditionThe current pattern could fail with a primary key constraint violation if another process inserts a row between the UPDATE and INSERT operations.
Check for existence before inserting:
if (result.rowsAffected === 0) { - const result2 = await db().insert(videoUploads).values({ - videoId, - uploaded, - total, - updatedAt, - }); + // Check if row exists (might have newer updatedAt) + const [existing] = await db() + .select({ videoId: videoUploads.videoId }) + .from(videoUploads) + .where(eq(videoUploads.videoId, videoId)); + + if (!existing) { + // Only insert if row doesn't exist + await db().insert(videoUploads).values({ + videoId, + uploaded, + total, + updatedAt, + }); + } + // If row exists with newer updatedAt, silently ignore }Alternatively, use an upsert pattern with
ON DUPLICATE KEY UPDATEif your database supports it.
🧹 Nitpick comments (2)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (1)
167-167: Coerce hasActiveUpload to boolean to avoid unintended pollingThe optional
hasActiveUpload?: booleancan beundefined, which will pass through touseUploadProgressand may trigger unintended behavior. Explicitly convert to boolean to ensure predictable polling.- const uploadProgress = useUploadProgress(cap.id, cap.hasActiveUpload); + const uploadProgress = useUploadProgress(cap.id, Boolean(cap.hasActiveUpload));apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
451-498: Potential memory leak from effect dependenciesThe effect cleanup function references
uploadState.pendingTaskfrom the closure, but this value might be stale when cleanup runs. This could lead to timers not being properly cleared.Store the timer reference in a ref to ensure proper cleanup:
+ const pendingTaskRef = useRef<ReturnType<typeof setTimeout>>(); useEffect(() => { if (!uploadState.videoId || uploadState.uploaded === 0 || !isUploading) return; - // Clear any existing pending task - if (uploadState.pendingTask) clearTimeout(uploadState.pendingTask); + // Clear any existing pending task + if (pendingTaskRef.current) { + clearTimeout(pendingTaskRef.current); + pendingTaskRef.current = undefined; + } const shouldSendImmediately = uploadState.uploaded >= uploadState.total; if (shouldSendImmediately) { // Send completion update immediately and clear state sendProgressUpdate( uploadState.videoId, uploadState.uploaded, uploadState.total, ); - setUploadState((prev) => ({ - ...prev, - pendingTask: undefined, - })); } else { // Schedule delayed update (after 2 seconds) const newPendingTask = setTimeout(() => { if (uploadState.videoId) { sendProgressUpdate( uploadState.videoId, uploadState.uploaded, uploadState.total, ); } }, 2000); - setUploadState((prev) => ({ - ...prev, - pendingTask: newPendingTask, - })); + pendingTaskRef.current = newPendingTask; } return () => { - if (uploadState.pendingTask) clearTimeout(uploadState.pendingTask); + if (pendingTaskRef.current) { + clearTimeout(pendingTaskRef.current); + pendingTaskRef.current = undefined; + } }; }, [ uploadState.videoId, uploadState.uploaded, uploadState.total, isUploading, ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(4 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(4 hunks)apps/web/app/api/desktop/[...route]/video.ts(5 hunks)apps/web/app/api/upload/[...route]/multipart.ts(2 hunks)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx(1 hunks)packages/web-domain/src/Video.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/s/[videoId]/_components/ProgressCircle.tsx
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When HTTP routes are necessary, implement them under app/api/*, configure CORS correctly, and set precise revalidation
Files:
apps/web/app/api/upload/[...route]/multipart.tsapps/web/app/api/desktop/[...route]/video.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/api/upload/[...route]/multipart.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/api/upload/[...route]/multipart.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
**/*.{ts,tsx}: Use Biome to format/lint TypeScript with a 2-space indent
TypeScript file names should be kebab-case (e.g., user-menu.tsx)
Files:
apps/web/app/api/upload/[...route]/multipart.tspackages/web-domain/src/Video.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
🧬 Code graph analysis (4)
apps/web/app/api/upload/[...route]/multipart.ts (2)
packages/database/index.ts (2)
db(30-35)updateIfDefined(38-39)packages/database/schema.ts (2)
videos(238-284)videoUploads(644-650)
packages/web-domain/src/Video.ts (3)
packages/web-domain/src/Folder.ts (1)
NotFoundError(13-16)packages/web-domain/src/Errors.ts (1)
InternalError(3-6)packages/web-domain/src/Policy.ts (1)
PolicyDeniedError(18-21)
apps/web/app/api/desktop/[...route]/video.ts (4)
packages/utils/src/constants/plans.ts (1)
userIsPro(21-45)packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (2)
videoUploads(644-650)videos(238-284)packages/web-domain/src/Video.ts (1)
Video(13-35)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx (2)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
useUploadProgress(25-65)apps/web/components/VideoThumbnail.tsx (1)
VideoThumbnail(46-136)
⏰ 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)
🔇 Additional comments (3)
apps/web/app/api/upload/[...route]/multipart.ts (1)
2-3: LGTM on schema imports.Importing
videoUploadshere is appropriate for cleanup after completion.packages/web-domain/src/Video.ts (1)
37-44: LGTM! Well-structured upload progress type with proper constraintsThe
UploadProgressclass properly constrainsuploadedandtotalas non-negative integers, matching the database schema and preventing invalid states.apps/web/app/api/desktop/[...route]/video.ts (1)
269-278: LGTM! Ownership verification properly implementedThe ownership check correctly destructures the first element from the array result and validates both existence and ownership before proceeding.
There was a problem hiding this comment.
Actionable comments posted: 4
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/s/[videoId]/page.tsx (1)
103-113: Declare hasActiveUpload?: boolean on VideoWithOrganization and the nestedvideotypeThe fetched data includes hasActiveUpload (see apps/web/app/s/[videoId]/page.tsx:297 and the object at ~line 664) but the type at ~lines 103–113 omits it — add
hasActiveUpload?: booleanto VideoWithOrganization and to the nestedvideotype so consumers (ShareVideo, HLSVideoPlayer, CapVideoPlayer, etc.) do not get TS errors.
♻️ Duplicate comments (1)
apps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx (1)
280-286: Loading overlay condition inverted; spinner won’t show during normal load.- className={clsx( - "flex absolute inset-0 z-10 justify-center items-center bg-black transition-opacity duration-300", - isUploading || videoLoaded || !isUploadFailed - ? "opacity-0 pointer-events-none" - : "opacity-100", - )} + className={clsx( + "flex absolute inset-0 z-10 justify-center items-center bg-black transition-opacity duration-300", + !isUploading && !videoLoaded && !isUploadFailed + ? "opacity-100" + : "opacity-0 pointer-events-none", + )}
🧹 Nitpick comments (1)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
307-313: Duplicate ‘loadedmetadata’ listener registration.handleLoadedMetadataWithTracks is added twice; causes duplicate cue handling and inconsistent state.
- video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); - video.addEventListener("load", handleLoad); - video.addEventListener("play", handlePlay); - video.addEventListener("error", handleError as EventListener); - video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); + video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks); + video.addEventListener("load", handleLoad); + video.addEventListener("play", handlePlay); + video.addEventListener("error", handleError as EventListener);Ensure cleanup removes it once (already handled).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/desktop/src-tauri/src/upload.rs(9 hunks)apps/web/actions/videos/get-user-videos.ts(2 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(4 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(2 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsx(1 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/SharedCapCard.tsx(1 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx(3 hunks)apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx(2 hunks)apps/web/app/embed/[videoId]/page.tsx(3 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(4 hunks)apps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx(3 hunks)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx(1 hunks)apps/web/app/s/[videoId]/_components/ShareVideo.tsx(2 hunks)apps/web/app/s/[videoId]/page.tsx(4 hunks)apps/web/lib/folder.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/desktop/src-tauri/src/recording.rs
- apps/web/app/s/[videoId]/_components/ShareVideo.tsx
- apps/web/app/s/[videoId]/_components/ProgressCircle.tsx
- apps/web/app/embed/[videoId]/_components/EmbedVideo.tsx
- apps/web/app/(org)/dashboard/caps/page.tsx
- apps/web/actions/videos/get-user-videos.ts
- apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx
- apps/desktop/src-tauri/src/upload.rs
- apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/SharedCapCard.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/lib/folder.tsapps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/SharedCapCard.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/lib/folder.tsapps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
**/*.{ts,tsx}: Use Biome to format/lint TypeScript with a 2-space indent
TypeScript file names should be kebab-case (e.g., user-menu.tsx)
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/SharedCapCard.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/lib/folder.tsapps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/SharedCapCard.tsxapps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/app/s/[videoId]/page.tsxapps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx
🧬 Code graph analysis (5)
apps/web/app/embed/[videoId]/page.tsx (1)
packages/database/schema.ts (3)
videos(238-284)sharedVideos(286-306)videoUploads(644-650)
apps/web/app/s/[videoId]/page.tsx (1)
packages/database/schema.ts (4)
videos(238-284)sharedVideos(286-306)users(46-94)videoUploads(644-650)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (3)
packages/web-domain/src/Video.ts (3)
Video(13-35)VideoId(9-9)VideoId(10-10)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
useUploadProgress(25-62)apps/web/app/s/[videoId]/_components/video/media-player.tsx (32)
MediaPlayerVideo(3114-3114)MediaPlayerVideo(3138-3138)MediaPlayerLoading(3118-3118)MediaPlayerLoading(3142-3142)MediaPlayerError(3119-3119)MediaPlayerError(3144-3144)MediaPlayerVolumeIndicator(3120-3120)MediaPlayerVolumeIndicator(3143-3143)MediaPlayerControls(3116-3116)MediaPlayerControls(3140-3140)MediaPlayerControlsOverlay(3117-3117)MediaPlayerControlsOverlay(3141-3141)MediaPlayerSeek(3124-3124)MediaPlayerSeek(3148-3148)MediaPlayerPlay(3121-3121)MediaPlayerPlay(3145-3145)MediaPlayerSeekBackward(3122-3122)MediaPlayerSeekBackward(3146-3146)MediaPlayerSeekForward(3123-3123)MediaPlayerSeekForward(3147-3147)MediaPlayerVolume(3125-3125)MediaPlayerVolume(3149-3149)MediaPlayerTime(3126-3126)MediaPlayerTime(3150-3150)MediaPlayerCaptions(3131-3131)MediaPlayerCaptions(3155-3155)MediaPlayerSettings(3133-3133)MediaPlayerSettings(3157-3157)MediaPlayerPiP(3130-3130)MediaPlayerPiP(3154-3154)MediaPlayerFullscreen(3129-3129)MediaPlayerFullscreen(3153-3153)
apps/web/lib/folder.ts (1)
packages/database/schema.ts (2)
videos(238-284)videoUploads(644-650)
apps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx (2)
packages/web-domain/src/Video.ts (3)
Video(13-35)VideoId(9-9)VideoId(10-10)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
useUploadProgress(25-62)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
apps/web/app/(org)/dashboard/spaces/[spaceId]/SharedCaps.tsx (1)
35-36: Type shape extension LGTM.SharedVideoData now includes hasActiveUpload; no logic change here. Ensure the server data guarantees this field to avoid undefined where not intended.
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (1)
389-399: Progress gating depends on server flag; verify upstream fixes.useUploadProgress(videoId, hasActiveUpload || false) will be disabled if hasActiveUpload is inverted upstream. After fixing queries, this component is correct.
apps/web/app/(org)/dashboard/spaces/[spaceId]/components/SharedCapCard.tsx (1)
17-18: Approve — downstream typing includes hasActiveUpload.CapCard's cap type declares
hasActiveUpload: boolean | undefinedin apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx, so passing it through from SharedCapCard is safe.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/app/embed/[videoId]/page.tsx (1)
150-150: Fix inverted hasPassword predicate (use IS NOT NULL).This currently returns true when no password exists.
Apply:
- hasPassword: sql`${videos.password} IS NULL`.mapWith(Boolean), + hasPassword: sql`${videos.password} IS NOT NULL`.mapWith(Boolean),
🧹 Nitpick comments (2)
apps/web/app/embed/[videoId]/page.tsx (2)
160-161: Add a LIMIT to avoid extra rows from sharedVideos left join.
shared_videoscan be one-to-many; destructuring[video]will take the first arbitrary row. Add a limit to reduce scan and ensure deterministic single row..leftJoin(videoUploads, eq(videos.id, videoUploads.videoId)) .where(eq(videos.id, videoId)), + .limit(1),
203-204: Narrow the type of hasActiveUpload to boolean.SQL
IS NOT NULLmaps to a definite boolean; noundefined.- hasActiveUpload: boolean | undefined; + hasActiveUpload: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/actions/videos/get-user-videos.ts(2 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(2 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx(3 hunks)apps/web/app/embed/[videoId]/page.tsx(3 hunks)apps/web/app/s/[videoId]/page.tsx(4 hunks)apps/web/lib/folder.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/actions/videos/get-user-videos.ts
- apps/web/app/(org)/dashboard/spaces/[spaceId]/page.tsx
- apps/web/app/s/[videoId]/page.tsx
- apps/web/app/(org)/dashboard/caps/page.tsx
- apps/web/lib/folder.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/embed/[videoId]/page.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/embed/[videoId]/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
**/*.{ts,tsx}: Use Biome to format/lint TypeScript with a 2-space indent
TypeScript file names should be kebab-case (e.g., user-menu.tsx)
Files:
apps/web/app/embed/[videoId]/page.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/embed/[videoId]/page.tsx
🧬 Code graph analysis (1)
apps/web/app/embed/[videoId]/page.tsx (1)
packages/database/schema.ts (3)
videos(238-284)sharedVideos(286-306)videoUploads(644-650)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Analyze (rust)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (2)
apps/web/app/embed/[videoId]/page.tsx (2)
9-10: Import of videoUploads looks correct.Matches schema and enables the join below.
154-156: hasActiveUpload mapping is correct.Using
IS NOT NULLwith.mapWith(Boolean)is the right approach here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/app/(org)/dashboard/caps/page.tsx (1)
176-179: Aggregate hasActiveUpload to be GROUP BY–safe (or use EXISTS).Using
${videoUploads.videoId} IS NOT NULLin a grouped query is non-aggregated and can error under only_full_group_by or yield nondeterministic values due to row multiplication from other LEFT JOINs. Replace with an aggregate or a scalar EXISTS subquery.Minimal fix (aggregate; keeps the LEFT JOIN):
- hasActiveUpload: sql`${videoUploads.videoId} IS NOT NULL`.mapWith( - Boolean, - ), + hasActiveUpload: sql<number>`IF(COUNT(DISTINCT ${videoUploads.videoId}) > 0, 1, 0)`.mapWith(Boolean),Alternative (preferred; removes need to join video_uploads and avoids row blow-up):
- .leftJoin(videoUploads, eq(videos.id, videoUploads.videoId)) + // no join needed for hasActiveUpload when using EXISTS- hasActiveUpload: sql`${videoUploads.videoId} IS NOT NULL`.mapWith( - Boolean, - ), + hasActiveUpload: sql<boolean>` + EXISTS( + SELECT 1 + FROM video_uploads vu + WHERE vu.video_id = ${videos.id} + ) + `.mapWith(Boolean),Also applies to: 186-195
🧹 Nitpick comments (2)
apps/web/app/embed/[videoId]/page.tsx (2)
203-204: Tighten type: make hasActiveUpload non-optional.This field is always selected; removing undefined simplifies consumers.
- hasActiveUpload: boolean | undefined; + hasActiveUpload: boolean;
150-150: Optional cleanup: drop unused hasPassword from this select if it’s not consumed.If downstream doesn’t use it here, omit to save bytes and reduce coupling.
- hasPassword: sql`${videos.password} IS NOT NULL`.mapWith(Boolean),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(5 hunks)apps/web/app/(org)/dashboard/caps/page.tsx(2 hunks)apps/web/app/embed/[videoId]/page.tsx(3 hunks)apps/web/app/s/[videoId]/page.tsx(4 hunks)apps/web/lib/folder.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/lib/folder.ts
- apps/web/app/s/[videoId]/page.tsx
- apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for client-side server state and data fetching in the web app
Mutations should call Server Actions and perform precise cache updates with setQueryData/setQueriesData, avoiding broad invalidations
Prefer Server Components for initial data and pass initialData to client components for React Query hydration
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/app/embed/[videoId]/page.tsx
{apps/web,packages/ui}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps/web,packages/ui}/**/*.{ts,tsx}: Use Tailwind CSS exclusively for styling in the web app and shared React UI components
Component naming: React components in PascalCase; hooks in camelCase starting with 'use'
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/app/embed/[videoId]/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
**/*.{ts,tsx}: Use Biome to format/lint TypeScript with a 2-space indent
TypeScript file names should be kebab-case (e.g., user-menu.tsx)
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/app/embed/[videoId]/page.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/page.tsxapps/web/app/embed/[videoId]/page.tsx
🧬 Code graph analysis (2)
apps/web/app/(org)/dashboard/caps/page.tsx (1)
packages/database/schema.ts (2)
videos(238-284)videoUploads(644-650)
apps/web/app/embed/[videoId]/page.tsx (1)
packages/database/schema.ts (3)
videos(238-284)sharedVideos(286-306)videoUploads(644-650)
⏰ 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 (1)
apps/web/app/embed/[videoId]/page.tsx (1)
150-156: LGTM on password/upload flags.Predicates now correctly use IS NOT NULL, and the LEFT JOIN on video_uploads matches. No issues.
Also applies to: 160-161
| const [progress, setProgress] = createSignal(0); | ||
| const reupload = createMutation(() => ({ | ||
| mutationFn: () => { | ||
| return commands.uploadExportedVideo( | ||
| mutationFn: async () => { | ||
| setProgress(0); | ||
| return await commands.uploadExportedVideo( | ||
| props.recording.path, | ||
| "Reupload", | ||
| new Channel<UploadProgress>((progress) => | ||
| setProgress(Math.round(progress.progress * 100)), | ||
| ), | ||
| ); | ||
| }, | ||
| onSettled: () => setProgress(0), | ||
| })); |
There was a problem hiding this comment.
nit but this could be refactored to use an event bus and the signal moved into the progress component, that gets recreated and initialized at 0 when reupload.isPending
Backdrop (the progress circle can be hard to read). Is this a problem in practice?"error"state with some UINOTE: This PR requires database migrations: https://app.planetscale.com/cap/cap-production/deploy-requests/36
Summary by CodeRabbit
New Features
Chores