Remove progress for legacy clients #1035
Conversation
WalkthroughAdds a desktop-version HTTP header to desktop uploader progress requests, records videoUploads on web-side video creation (with server-side insertion gated by desktop semver), removes a small exported helper, and centralizes upload state into a new union-based UploadStatus used across upload UI components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor DesktopApp
participant DesktopUploader as UploadProgressUpdater
participant Server as /api/desktop/.../video
DesktopApp->>DesktopUploader: send_api_update(videoId, uploaded, total)
DesktopUploader->>Server: POST /progress (JSON body)
Note right of DesktopUploader #DDEBF7: Adds header\nX-Cap-Desktop-Version: env!("CARGO_PKG_VERSION")
Server-->>DesktopUploader: 200 OK
sequenceDiagram
autonumber
participant Client as Web Client
participant Action as actions/video/upload.ts
participant DB as Database
Client->>Action: createVideoAndGetUploadUrl()
Action->>DB: insert into videos(...)
Action->>DB: insert into videoUploads({ videoId: idToUse })
Action-->>Client: presigned URL + ids
sequenceDiagram
autonumber
actor DesktopApp
participant API as GET /api/desktop/.../video/create
participant Semver as isAtLeastSemver
participant DB as Database
DesktopApp->>API: GET /create (Header: X-Cap-Desktop-Version)
API->>Semver: isAtLeastSemver(versionString, 0,3,68)
alt >= 0.3.68
API->>DB: insert videoUploads({ videoId })
else < 0.3.68 or missing
API-->API: skip insert
end
API-->>DesktopApp: response
sequenceDiagram
autonumber
participant Button as UploadCapButton
participant Ctx as UploadingContext
participant UI as UploadPlaceholderCard
participant Router as Router
Button->>Ctx: setUploadStatus({status: "parsing"})
Button->>Ctx: setUploadStatus({status: "creating"})
Button->>Ctx: setUploadStatus({status: "converting", capId, progress})
Button->>Ctx: setUploadStatus({status: "uploadingVideo", capId, progress, thumbnailUrl?})
UI->>Ctx: read uploadStatus -> render progress/label/thumbnail
Button->>Ctx: setUploadStatus(undefined)
Button->>Router: refresh()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
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
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/upload.rs (1)
189-197: Header addition is correct; consider also sending it on video create for earlier capability detectionUsing env!("CARGO_PKG_VERSION") is fine. Optional: also include X-Cap-Desktop-Version on the create endpoint call (create_or_get_video) so the server can pre-create progress rows for capable desktop clients before the first POST /progress arrives.
- let response = app - .authed_api_request(s3_config_url, |client, url| client.get(url)) + let response = app + .authed_api_request(s3_config_url, |client, url| { + client.get(url).header("X-Cap-Desktop-Version", env!("CARGO_PKG_VERSION")) + })apps/web/actions/video/upload.ts (1)
234-236: Create a progress row on web uploads — add conflict guardInsertion is fine. Add onConflictDoNothing to avoid flakiness if this action is retried.
- await db().insert(videoUploads).values({ + await db().insert(videoUploads).values({ videoId: idToUse, - }); + }).onConflictDoNothing();apps/web/app/api/desktop/[...route]/video.ts (1)
128-137: Create route gating aligns with goal; avoid hard failure on bad headerGood: gating progress row creation on desktop version. If the header is malformed, isGreaterThanSemver currently throws and returns 500. Prefer treating invalid/absent headers as “legacy” (no insert) without throwing.
- const xCapVersion = c.req.header("X-Cap-Desktop-Version"); - const clientSupportsUploadProgress = xCapVersion - ? isGreaterThanSemver(xCapVersion, 0, 3, 68) - : false; + const xCapVersion = c.req.header("X-Cap-Desktop-Version"); + const clientSupportsUploadProgress = + !!xCapVersion && isGreaterThanSemver(xCapVersion, 0, 3, 68);…and update isGreaterThanSemver to return false (not throw) on parse failure (see separate comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/upload.rs(1 hunks)apps/web/actions/video/upload.ts(2 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(3 hunks)apps/web/app/api/desktop/[...route]/video.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
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/upload.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/upload.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/upload.rs
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/actions/**/*.ts: All Groq/OpenAI calls must be implemented in Next.js Server Actions under apps/web/actions
Place Server Actions for the web app under apps/web/actions and mark them with 'use server'
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 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/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/UploadCapButton.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/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/UploadCapButton.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/actions/video/upload.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
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/desktop/[...route]/video.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
🧬 Code graph analysis (2)
apps/web/actions/video/upload.ts (2)
packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (1)
videoUploads(644-650)
apps/web/app/api/desktop/[...route]/video.ts (3)
packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (1)
videoUploads(644-650)apps/desktop/scripts/prepare.js (1)
major(30-30)
🪛 GitHub Check: Typecheck
apps/web/app/api/desktop/[...route]/video.ts
[failure] 339-339:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
[failure] 338-338:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
[failure] 337-337:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
⏰ 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 (4)
apps/web/actions/video/upload.ts (1)
10-10: Schema import looks goodImporting videoUploads alongside s3Buckets and videos is appropriate.
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
371-376: Thumbnail progress forced to 100%This matches the simplified UX; no issues.
395-397: Final 100% update and UI completion is goodEnsures UI reaches completion even if the last progress event didn’t fire.
apps/web/app/api/desktop/[...route]/video.ts (1)
321-348: Robustness: strip +build metadata, don't throw on parse failure, and fix equality logicType-check passes (no TS errors). Still recommend making parsing total (strip build metadata), return false on invalid input instead of throwing, and fix the logic that currently treats an exact release as “greater than” itself.
File: apps/web/app/api/desktop/[...route]/video.ts — function isGreaterThanSemver
-function isGreaterThanSemver( - versionString: string, - major: number, - minor: number, - patch: number, -): boolean { - // Parse version string, remove 'v' prefix if present - const match = versionString - .replace(/^v/, "") - .match(/^(\d+)\.(\d+)\.(\d+)(?:-(.+))?/); - - if (!match) { - throw new Error(`Invalid semver version: ${versionString}`); - } - - const [, vMajor, vMinor, vPatch, prerelease] = match; - const parsedMajor = parseInt(vMajor, 10); - const parsedMinor = parseInt(vMinor, 10); - const parsedPatch = parseInt(vPatch, 10); +function isGreaterThanSemver( + versionString: string, + major: number, + minor: number, + patch: number, +): boolean { + // Accept optional 'v' prefix and strip build metadata; allow optional prerelease. + const core = versionString.replace(/^v/, "").split("+")[0]; + const match = core.match(/^(\d+)\.(\d+)\.(\d+)(?:-([\w.-]+))?$/); + if (!match) return false; + const parsedMajor = Number(match[1]); + const parsedMinor = Number(match[2]); + const parsedPatch = Number(match[3]); + const prerelease = match[4]; // Compare major.minor.patch if (parsedMajor !== major) return parsedMajor > major; if (parsedMinor !== minor) return parsedMinor > minor; if (parsedPatch !== patch) return parsedPatch > patch; - // If versions are equal, prerelease versions have lower precedence - return !prerelease; // true if no prerelease (1.0.0 > 1.0.0-alpha), false if prerelease + // Numeric parts are equal: prerelease is lower precedence than a release; equality is not greater. + return false; }Likely an incorrect or invalid review comment.
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
64-76: Add a switch to hide the percent for legacy/no-progress casesShowing “0%” contradicts the PR goal. Expose a boolean to suppress the number while keeping the label.
Apply:
const ProgressCircle = ({ progress, className, progressTextClassName, subTextClassName, - subText, + subText, + showPercent = true, }: { progress: number; className?: string; progressTextClassName?: string; subTextClassName?: string; - subText?: string; + subText?: string; + showPercent?: boolean; }) => { @@ - <p - className={clsx( - "text-sm font-semibold tabular-nums md:leading-tight leading-tight text-white xs:text-sm md:text-lg", - progressTextClassName, - )} - > - {Math.round(progress)}% - </p> + {showPercent && ( + <p + className={clsx( + "text-sm font-semibold tabular-nums md:leading-tight leading-tight text-white xs:text-sm md:text-lg", + progressTextClassName, + )} + > + {Math.round(progress)}% + </p> + )} @@ - {subText || "Uploading..."} + {subText || "Uploading..."}Also applies to: 109-116, 123-124
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (5)
56-63: Hide the “0%” number when progress is unavailableFor legacy clients (no server progress), showing “0%” is misleading. Hide the percent but keep the label.
Apply:
<ProgressCircle - progressTextClassName="md:!text-[11px]" + progressTextClassName="sr-only md:!text-[11px]" subTextClassName="!mt-0 md:!text-[7px] !text-[10px] mb-1" className="md:scale-[1.5] scale-[1.2]" progress={0} + subText="Uploading..." + showPercent={false} />
42-42: Use a translucent overlay so the thumbnail remains visibleOpaque black hides the preview completely.
Apply:
- <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">
126-134: Simplify: else branch is unreachable after the failed guardOnly “uploading” reaches here.
Apply:
- progress={progress.status === "uploading" ? progress.progress : 0} + progress={progress.progress}
46-73: DRY up repeated ProgressCircle markupThree branches duplicate the same wrapper/props.
Extract a tiny helper:
function SmallCircle(props: Omit<React.ComponentProps<typeof ProgressCircle>, "progress"> & { progress: number }) { return ( <div className="relative size-20 md:size-16"> <ProgressCircle progressTextClassName="md:!text-[11px]" subTextClassName="!mt-0 md:!text-[7px] !text-[10px] mb-1" className="md:scale-[1.5] scale-[1.2]" {...props} /> </div> ); }Then replace each branch with as needed.
25-25: Remove non-standard Tailwind classborder-px— not defined in repoNo tailwind.config.js defines a custom borderWidth
px; Tailwind defaults do not includeborder-px. apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (around line 25): removeborder-px(redundant withborder) or replace withborder-[1px].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx(1 hunks)apps/web/app/api/desktop/[...route]/video.ts(3 hunks)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx(2 hunks)packages/utils/src/helpers.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/utils/src/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
- apps/web/app/api/desktop/[...route]/video.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/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.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/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.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/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
🧬 Code graph analysis (1)
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (3)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(21-29)packages/utils/src/helpers.ts (1)
getUploadStatus(77-80)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). (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 (1)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
69-76: Nice: optional subText prop adds needed flexibilityNon‑breaking API, aligns with new callers using custom labels.
Also applies to: 123-124
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
Outdated
Show resolved
Hide resolved
oscartbeaumont
force-pushed
the
remove-progress-for-legacy-clients
branch
from
0171b41 to
370a544
Compare
oscartbeaumont
force-pushed
the
remove-progress-for-legacy-clients
branch
from
370a544 to
b14adac
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/components/VideoThumbnail.tsx (1)
14-15: TypeobjectFitprecisely; dropas any.Avoid
anyper guidelines and let TS validate values.- objectFit?: string; + objectFit?: "fill" | "contain" | "cover" | "none" | "scale-down";- style={{ objectFit: objectFit as any }} + style={{ objectFit }}Also applies to: 123-123
🧹 Nitpick comments (15)
apps/web/components/VideoThumbnail.tsx (4)
73-77: Remove dead UploadingContext wiring.The commented-out hook usage adds noise. Drop it (and the now-unused import at Line 7) to avoid confusion and unnecessary re-renders.
- // const { uploadingCapId } = useUploadingContext(); - - // useEffect(() => { - // imageUrl.refetch(); - // }, [imageUrl.refetch, uploadingCapId]); + // Upload progress no longer alters thumbnail fetching for legacy clients.
7-7: Delete unused import.-import { useUploadingContext } from "@/app/(org)/dashboard/caps/UploadingContext";
79-79: Stabilize the fallback gradient across renders.Randomizing each render can cause flicker. Persist it with a ref.
- const randomGradient = `linear-gradient(to right, ${generateRandomGrayScaleColor()}, ${generateRandomGrayScaleColor()})`; + const gradientRef = useRef<string>(); + if (!gradientRef.current) { + gradientRef.current = `linear-gradient(to right, ${generateRandomGrayScaleColor()}, ${generateRandomGrayScaleColor()})`; + } + const randomGradient = gradientRef.current;
24-34: Clamp seconds to 1–59 to avoid "60 secs".
Math.ceilcan render 60s just below a minute. Clamp instead.- const remainingSeconds = Math.ceil(momentDuration.asSeconds() % 60); // Use ceil to avoid 0 secs + const remainingSeconds = Math.max(1, Math.floor(momentDuration.asSeconds() % 60)); // Avoid 0, never reach 60apps/web/app/(org)/dashboard/caps/uploader.ts (1)
7-13: Remove deadtodomutation andany.This scaffold isn’t used and violates strict TS. Delete it to keep surface area clean.
-const todo = { - mutationKey: ["todo"], - mutationFn: async (data: any) => { - await new Promise((resolve) => setTimeout(resolve, 1000)); - return "bruh"; - }, -} satisfies MutationOptions;apps/web/app/(org)/dashboard/caps/Caps.tsx (4)
270-279: Remove TODO and dead code.The filtered
visibleVideoslogic is fully disabled; keep the code lean.- const visibleVideos = data; // TODO: Remove this - - // const visibleVideos = useMemo( - // () => - // isUploading && uploadingCapId - // ? data.filter((video) => video.id !== uploadingCapId) - // : data, - // [data, isUploading, uploadingCapId], - // ); + const visibleVideos = data;
299-311: Drop commented props/placeholder JSX.These comments are no longer actionable and clutter the file.
- <UploadCapButton - // onStart={(id, thumbnailUrl) => { - // setIsUploading(true); - // setUploadingCapId(id); - // setUploadingThumbnailUrl(thumbnailUrl); - // setUploadProgress(0); - // }} - size="sm" - // onComplete={() => { - // setIsUploading(false); - // setUploadingCapId(null); - // setUploadingThumbnailUrl(undefined); - // setUploadProgress(0); - // }} - /> + <UploadCapButton size="sm" />- {/*{isUploading && ( - <UploadPlaceholderCard key={"upload-placeholder"} /> - )}*/}Also applies to: 333-335
11-11: Remove unuseduseMemoimport.-import { useEffect, useMemo, useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react";
1-1: Filename style.Repo guideline prefers kebab-case file names. Consider renaming
Caps.tsxtocaps.tsxin a separate PR to avoid noisy diffs.apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (3)
432-446: Use functional state update to avoid stalependingTaskcapture.
uploadStatereferenced insideonprogresscan be stale; use the setter form.- xhr.upload.onprogress = (event) => { + xhr.upload.onprogress = (event) => { if (event.lengthComputable) { const percent = (event.loaded / event.total) * 100; setUploadProgress(percent); onProgress?.(uploadId, 100, percent); - setUploadState({ - videoId: uploadId, - uploaded: event.loaded, - total: event.total, - lastUpdateTime: Date.now(), - pendingTask: uploadState.pendingTask, - }); + setUploadState((prev) => ({ + videoId: uploadId, + uploaded: event.loaded, + total: event.total, + lastUpdateTime: Date.now(), + pendingTask: prev.pendingTask, + })); } };
448-456: Send a final progress update on success.Guarantees immediate completion signal instead of waiting for the debounce effect.
xhr.onload = () => { if (xhr.status >= 200 && xhr.status < 300) { setUploadProgress(100); onProgress?.(uploadId, 100, 100); - // sendProgressUpdate(uploadId, 100, 100); // TODO + // Immediate completion update; server will clean up the row. + sendProgressUpdate(uploadId, 100, 100); resolve(); } else {
113-161: Timer management viauseRefinstead of state.Storing
pendingTaskin state causes extra renders and complicates cleanup. Prefer auseRef<ReturnType<typeof setTimeout> | null>and clear/schedule it directly in this effect. I can send a small patch if you want it in this PR.apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (2)
15-15: Delete commented import.Remove the commented
UploadPlaceholderCardimport to keep the module clean.-// import { UploadPlaceholderCard } from "../../../caps/components/UploadPlaceholderCard";
184-186: Remove commented placeholder block.With progress/placeholder removed, delete this block rather than keeping commented code.
- {/*{isUploading && ( - <UploadPlaceholderCard key={"upload-placeholder"} /> - )}*/}apps/web/app/(org)/dashboard/caps/components/index.ts (1)
7-7: Remove commented-out re-export for UploadPlaceholderCard.Delete the commented line in apps/web/app/(org)/dashboard/caps/components/index.ts (line 7). UploadPlaceholderCard is defined at apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx and is imported directly in apps/web/app/(org)/dashboard/caps/Caps.tsx; no code imports it from the components barrel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/(org)/dashboard/caps/Caps.tsx(4 hunks)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx(4 hunks)apps/web/app/(org)/dashboard/caps/components/index.ts(1 hunks)apps/web/app/(org)/dashboard/caps/uploader.ts(1 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx(4 hunks)apps/web/app/api/desktop/[...route]/video.ts(3 hunks)apps/web/components/VideoThumbnail.tsx(1 hunks)packages/utils/src/helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/utils/src/helpers.ts
🧰 Additional context used
📓 Path-based instructions (5)
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/components/index.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/uploader.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/components/VideoThumbnail.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/Caps.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/components/index.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/uploader.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/components/VideoThumbnail.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/Caps.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/components/index.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/(org)/dashboard/caps/uploader.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/components/VideoThumbnail.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/Caps.tsx
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/desktop/[...route]/video.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/components/VideoThumbnail.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/Caps.tsx
🧬 Code graph analysis (3)
apps/web/app/api/desktop/[...route]/video.ts (3)
packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (1)
videoUploads(644-650)apps/desktop/scripts/prepare.js (1)
major(30-30)
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(47-55)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
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 (3)
apps/web/app/api/desktop/[...route]/video.ts (1)
300-306: POST /progress still creates rows for legacy clients; gate the insert.This re-enables progress for legacy clients (no header), contradicting the PR goal. Gate like
/createor require a pre-existing row.- if (result.rowsAffected === 0) - await db().insert(videoUploads).values({ - videoId, - uploaded, - total, - updatedAt, - }); + if (result.rowsAffected === 0) { + const xCapVersion = c.req.header("X-Cap-Desktop-Version"); + const clientSupportsUploadProgress = + !!xCapVersion && isAtLeastSemver(xCapVersion, 0, 3, 68); + if (clientSupportsUploadProgress) { + await db().insert(videoUploads).values({ + videoId, + uploaded, + total, + updatedAt, + }); + } + }Alternative: only update when a row already exists (created by capable desktop or web flow).
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (2)
160-167: LGTM: Simplified visibility logic matches the decision to drop in-flight upload placeholders.
177-177: LGTM: Empty-state condition is correct with placeholder removed.
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
77-77: Use context-derived isUploading/uploadingCapId instead of recomputing.UploadingContext already exposes these; simplify and avoid duplication.
- const { state } = useUploadingContext(); + const { state, isUploading, uploadingCapId } = useUploadingContext(); ... - const isUploading = state !== undefined; - const uploadingCapId = state && "capId" in state ? state.capId : undefined;Also applies to: 263-265, 292-292
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
55-67: Scope beforeunload to active transfer phases only (optional).Avoid prompting on ephemeral states like "parsing"/"creating" to reduce false dialogs.
- if (state?.status) { + if (state?.status === "converting" || + state?.status === "uploadingThumbnail" || + state?.status === "uploadingVideo") {apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (3)
15-21: Rename local UploadState to avoid confusion with context UploadState.Two different UploadState types in nearby modules is error-prone.
-type UploadState = { +type UploadProgressState = { videoId?: string; uploaded: number; total: number; pendingTask?: ReturnType<typeof setTimeout>; lastUpdateTime: number; }; ... -const [uploadState, setUploadState] = useState<UploadState>({ +const [uploadState, setUploadState] = useState<UploadProgressState>({Also applies to: 37-41
293-295: Remove unused thumbnailUrl to avoid leaking an object URL.It's created but never used or revoked.
- const thumbnailUrl = thumbnailBlob - ? URL.createObjectURL(thumbnailBlob) - : undefined;
388-395: Redundant setState(undefined).You clear state both before and in finally; keep only the finally to avoid double renders.
- setState(undefined); ... } finally { - setState(undefined); + setState(undefined);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/(org)/dashboard/caps/Caps.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(10 hunks)apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx(3 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/UploadCapButtonWithFolder.tsx(0 hunks)apps/web/app/(org)/dashboard/folder/[id]/page.tsx(2 hunks)packages/utils/src/helpers.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/utils/src/helpers.ts
- apps/web/app/(org)/dashboard/folder/[id]/components/UploadCapButtonWithFolder.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/Caps.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/page.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.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/Caps.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/page.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.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/Caps.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/page.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/page.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsx
🧬 Code graph analysis (4)
apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(42-49)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
UploadCapButton(23-508)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(42-49)
apps/web/app/(org)/dashboard/folder/[id]/page.tsx (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
UploadCapButton(23-508)
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (2)
packages/utils/src/helpers.ts (2)
getProgressCircleConfig(63-68)calculateStrokeDashoffset(70-75)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (2)
useUploadingContext(42-49)UploadState(15-36)
⏰ 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 (4)
apps/web/app/(org)/dashboard/folder/[id]/page.tsx (1)
15-15: LGTM on centralizing UploadCapButton; confirm barrel export stability.The shared component with folderId looks correct and keeps the page a Server Component with a Client boundary. Please verify the components barrel re-exports UploadCapButton without pulling unused code paths.
Also applies to: 28-28
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (1)
21-22: LGTM; state‑driven placeholder is clean and robust.Early return and friendly status mapping are appropriate. No blockers.
Also applies to: 39-43
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
71-76: LGTM on unified UploadState and derived flags.The union and derived isUploading/uploadingCapId look good.
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
328-336: Final progress send may be stale and conflicts with “disable legacy progress.”This uses potentially stale uploadState and still reports progress. If the PR goal is to remove progress for legacy clients, gate or drop this call. If keeping it for modern clients, compute from known values to avoid 0/0.
- sendProgressUpdate(uploadId, uploadState.total, uploadState.total); + // Intentionally not sending final progress here to align with legacy-client behavior. + // If needed for modern clients, prefer: + // sendProgressUpdate(uploadId, optimizedBlob.size, optimizedBlob.size);
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
Outdated
Show resolved
Hide resolved
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)
405-429: Gate web progress calls or enforce server-side header check to avoid creating video_uploads rowsServer-only initial insert is gated by X-Cap-Desktop-Version > 0.3.68 (apps/web/app/api/desktop/[...route]/video.ts:128–136), but the progress update path will update-or-insert regardless (apps/web/app/api/desktop/[...route]/video.ts:284–304). The web UploadCapButton posts to /api/desktop/video/progress without that header (apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx:405–429), so web uploads will create video_uploads rows. Action: either stop/gate client-side progress calls for web uploads, send an appropriate X-Cap-Desktop-Version from permitted clients, or add a server-side check to ignore requests missing/with-too-old header.
🧹 Nitpick comments (8)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (5)
389-396: Remove duplicate state reset; keep it in finally only.setState(undefined) is called both before router.refresh() and again in finally. One is enough.
- setState(undefined); - router.refresh();
292-299: Revoke thumbnail object URL to avoid a small leak.thumbnailUrl created via URL.createObjectURL is never revoked.
@@ - const thumbnailUrl = thumbnailBlob + const thumbnailUrl = thumbnailBlob ? URL.createObjectURL(thumbnailBlob) : undefined; @@ - router.refresh(); + if (thumbnailUrl) { + URL.revokeObjectURL(thumbnailUrl); + } + router.refresh();Also applies to: 389-392
33-34: Use isUploading from context instead of recomputing.Aligns with single source of truth and removes the TODO.
- const { state, setState } = useUploadingContext(); + const { state, setState, isUploading } = useUploadingContext(); @@ - const isUploading = !!state; // TODO + // using isUploading from UploadingContextAlso applies to: 481-482
15-22: Avoid type-name collision with context UploadState.Local UploadState shadows the context’s UploadState. Rename for clarity.
-type UploadState = { +type UploadBytesState = { videoId?: string; uploaded: number; total: number; pendingTask?: ReturnType<typeof setTimeout>; lastUpdateTime: number; }; @@ - const [uploadState, setUploadState] = useState<UploadState>({ + const [uploadState, setUploadState] = useState<UploadBytesState>({ uploaded: 0, total: 0, lastUpdateTime: Date.now(), });Also applies to: 37-41
1-1: File naming should be kebab-case per guidelines.Consider renaming to apps/web/app/(org)/dashboard/caps/components/upload-cap-button.tsx.
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (3)
55-69: beforeunload guard is fine; consider adding a passive fallback.Some browsers may skip beforeunload; optionally add a pagehide listener to clear state or flush telemetry with sendBeacon if needed.
70-81: Memoize provider value to avoid needless re-renders.Small optimization; keeps value stable when identity doesn’t need to change beyond state updates.
- import { createContext, useContext, useEffect, useState } from "react"; + import { createContext, useContext, useEffect, useMemo, useState } from "react"; @@ - return ( - <UploadingContext.Provider - value={{ - state, - setState, - isUploading: state !== undefined, - uploadingCapId: state && "capId" in state ? state.capId : null, - }} - > + const value = useMemo( + () => ({ + state, + setState, + isUploading: state !== undefined, + uploadingCapId: state && "capId" in state ? state.capId : null, + }), + [state], + ); + + return ( + <UploadingContext.Provider value={value}> {children} </UploadingContext.Provider> );Also applies to: 4-4
1-1: File naming should be kebab-case per guidelines.Consider renaming to apps/web/app/(org)/dashboard/caps/uploading-context.tsx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(10 hunks)apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.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/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.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/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.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/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx
🧬 Code graph analysis (1)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(43-50)
⏰ 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/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
329-336: Stop using stale state in xhr.onload; use a ref updated in onprogress (or drop the call).sendProgressUpdate reads uploadState captured at handler creation, which is often 0/0. Use a ref updated in onprogress and read it in onload, guarding zero totals.
Apply these diffs:
@@ - const [uploadState, setUploadState] = useState<UploadState>({ + const [uploadState, setUploadState] = useState<UploadState>({ uploaded: 0, total: 0, lastUpdateTime: Date.now(), }); + // Track last progress without relying on stale closures + const lastProgressRef = useRef<{ uploaded: number; total: number }>({ + uploaded: 0, + total: 0, + });@@ xhr.upload.onprogress = (event) => { if (event.lengthComputable) { const percent = (event.loaded / event.total) * 100; setState({ status: "uploadingVideo", capId: uploadId, progress: percent, thumbnailUrl: thumbnailUrl, }); setUploadState({ videoId: uploadId, uploaded: event.loaded, total: event.total, lastUpdateTime: Date.now(), pendingTask: uploadState.pendingTask, }); + // keep last known values for onload + lastProgressRef.current = { uploaded: event.loaded, total: event.total }; } }; xhr.onload = () => { if (xhr.status >= 200 && xhr.status < 300) { - sendProgressUpdate(uploadId, uploadState.total, uploadState.total); + const { uploaded, total } = lastProgressRef.current; + if (total > 0) { + // final 100% confirmation using last known totals + sendProgressUpdate(uploadId, total, total); + } resolve(); } else { reject(new Error(`Upload failed with status ${xhr.status}`)); } };
312-317: Correct status for main video upload — good.Switch to status: "uploadingVideo" during main upload is correct and fixes prior UI/state bugs.
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
45-49: Context guard is correct.Throwing when used outside provider prevents subtle runtime bugs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (2)
4-4: Add useCallback to imports for optimization.The
setStatefunction fromuseStatewould benefit fromuseCallbackoptimization in the provider to prevent unnecessary re-renders when passed down through context.-import { createContext, useContext, useEffect, useState } from "react"; +import { createContext, useCallback, useContext, useEffect, useState } from "react";Then wrap
setStatein the provider:export function UploadingProvider({ children }: { children: React.ReactNode }) { const [state, setState] = useState<UploadState>(); + const setStateCallback = useCallback((state: UploadState | undefined) => { + setState(state); + }, []); // ... return ( <UploadingContext.Provider value={{ state, - setState, + setState: setStateCallback, isUploading: state !== undefined, uploadingCapId: state && "capId" in state ? state.capId : null, }} > {children} </UploadingContext.Provider> ); }
7-9: Address the TODO and improve naming.The TODO comment indicates that the naming should be improved. Consider renaming
state/setStatetouploadState/setUploadStatefor clarity.interface UploadingContextType { - // TODO: Rename these - state: UploadState | undefined; - setState: (state: UploadState | undefined) => void; + uploadState: UploadState | undefined; + setUploadState: (state: UploadState | undefined) => void; isUploading: boolean; uploadingCapId: string | null; }Would you like me to help update all the call sites to use the renamed properties?
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
431-479: Address the TODO comment for progress updates.The commented-out
useEffectcontains logic for batched progress updates that might be important for performance. Consider whether this functionality should be restored or permanently removed.Do you want me to help determine if this batched progress update logic should be restored, or would you prefer to remove the commented code entirely?
481-481: Remove TODO and finalize isUploading derivation.The
isUploadingderivation is correct - it should be true when state is defined. Remove the TODO comment.- const isUploading = !!state; // TODO + const isUploading = !!state;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(10 hunks)apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx(3 hunks)
🧰 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/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.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/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.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/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsx
🧬 Code graph analysis (2)
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (1)
packages/utils/src/helpers.ts (2)
getProgressCircleConfig(63-68)calculateStrokeDashoffset(70-75)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(43-50)
🪛 GitHub Check: Typecheck
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
[failure] 27-27:
Property 'thumbnailUrl' does not exist on type 'never'.
[failure] 25-25:
This comparison appears to be unintentional because the types '"parsing" | "creating" | "converting" | "uploadingThumbnail" | "uploadingVideo"' and '"uploading"' have no overlap.
⏰ 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 (16)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (3)
15-37: LGTM - Well-structured state machine.The
UploadStateunion type provides a clear state machine for the upload process with appropriate data for each phase. The progression from parsing → creating → converting → uploadingVideo → uploadingThumbnail is logical.
56-68: LGTM - Good user experience enhancement.The beforeunload handler prevents accidental page closes during uploads. The Chrome-specific
returnValuerequirement is properly handled.
75-76: LGTM - Clean derived state logic.The derived
isUploadinganduploadingCapIdproperties are cleanly computed from the unified state, reducing the complexity of managing multiple separate state variables.apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (5)
7-7: LGTM - Good performance optimization.Moving the circumference calculation to module scope prevents recalculating it on every render.
12-19: LGTM - Clear progress calculation logic.The conditional logic correctly extracts progress from the appropriate state variants while defaulting to 0 for states without progress.
90-104: LGTM - Clear status mapping.The
getFriendlyStatusfunction provides user-friendly labels for each state. The grouping of "uploadingThumbnail" and "uploadingVideo" under "Uploading" makes sense from a UX perspective.
25-25: Duplicate comment: Fix incorrect status comparison.This is the same TypeScript issue flagged in the static analysis - the status "uploading" doesn't exist in the UploadState union type.
10-10: Verify state consistency across components.The early return when
!stateensures the component only renders when actively uploading. Ensure this behavior aligns with expectations in parent components.Also applies to: 21-21
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (8)
33-33: LGTM - Clean integration with context.The component correctly integrates with the new centralized upload state management via
useUploadingContext().
60-62: LGTM - Proper dynamic imports.Using dynamic imports for heavy modules like
@remotion/media-parserand@remotion/webcodecsimproves initial bundle size and loading performance.
64-64: LGTM - Clear state progression.The state transitions through "parsing" → "creating" → "converting" provide clear feedback about the upload progress to users.
Also applies to: 80-80, 96-96
130-135: LGTM - Consistent state updates.The progress updates during conversion correctly maintain the "converting" status while updating progress values.
312-317: LGTM - Correct status for main video upload.The status is correctly set to "uploadingVideo" during the main video upload, and the
thumbnailUrlis properly included when available.
365-369: LGTM - Appropriate thumbnail upload handling.The thumbnail upload correctly uses "uploadingThumbnail" status and calculates progress as 90% + 10% of thumbnail progress, providing smooth visual feedback.
389-389: LGTM - Proper state cleanup.The state is properly cleared both on success and in the finally block, ensuring clean state regardless of upload outcome.
Also applies to: 395-395
15-21: Consider removing unused local UploadState type.The local
UploadStatetype (lines 15-21) appears to be different from the imported one and might be unused now that the component uses the centralized state management.
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
Outdated
Show resolved
Hide resolved
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 (5)
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (4)
126-153: Retries don’t refresh the URL; likely ineffective for expired/signed URLs.reloadVideo() calls video.load() without fetching a fresh URL, so we may repeatedly reload an expired resource. Call fetchNewUrl() before reloading and rely on the src change to trigger load; keep the position restore on canplay.
Example refactor:
const reloadVideo = useCallback(async () => { const video = videoRef.current; if (!video || retryCount.current >= maxRetries) return; const currentPosition = video.currentTime; const wasPlaying = !video.paused; await fetchNewUrl(); // updates resolvedVideoSrc; React re-renders <video src=...> const restorePosition = () => { video.currentTime = currentPosition; if (wasPlaying) { video.play().catch((err) => console.error("Error resuming playback:", err)); } video.removeEventListener("canplay", restorePosition); }; video.addEventListener("canplay", restorePosition); retryCount.current += 1; }, [fetchNewUrl, maxRetries]);
85-123: Adding a cache-busting query param can break pre‑signed S3/R2 URLs.Appending &_t=... will invalidate AWS SigV4 and similar signatures. Prefer HEAD with cache: 'no-store' (and avoid modifying the query), or use RequestCache control and rely on redirects.
Example:
const response = await fetch(videoSrc, { method: "HEAD", cache: "no-store", redirect: "follow" }); const finalUrl = response.redirected ? response.url : videoSrc;If you still need cache-busting, only apply it to hosts you control (never for URLs containing X-Amz-Algorithm/X-Amz-Signature or .r2.cloudflarestorage.com/.s3./amazonaws.com).
305-313: Duplicate 'loadedmetadata' listener registered; one removal → leak + double-calls.The event is added twice (Lines 307 and 311) but removed once in cleanup. Drop the duplicate registration.
Use a single registration:
video.addEventListener("loadedmetadata", handleLoadedMetadataWithTracks);
11-12: Legacy-client gating is NOT implemented — useUploadProgress is gated by localStorage; CapVideoPlayer handles null
- useUploadProgress (apps/web/app/s/[videoId]/_components/ProgressCircle.tsx) uses localStorage key "betaUploadProgress" to enable the hook (enabled = enableBetaUploadProgress ? enabledRaw : false) and does not inspect client version or headers.
- The hook returns null when disabled; CapVideoPlayer (apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx) uses optional chaining (uploadProgress?.status) and treats null as "not uploading" — UI is safe.
- If the PR's objective was to suppress progress for legacy/old desktop clients, that behavior is missing. Implement detection (server-side or via the desktop upload header X-Cap-Desktop-Version — set in apps/desktop/src-tauri/src/upload.rs) and gate the hook accordingly (server feature flag or API-driven enabled flag passed to the client).
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (1)
25-31: Guard against undefined thumbnailUrl to avoidrequest
uploadStatus.thumbnailUrlcan be undefined; rendering<img src={undefined}>can trigger an unintended request. Add a truthy guard.- {uploadStatus.status === "uploadingVideo" ? ( + {uploadStatus.status === "uploadingVideo" && uploadStatus.thumbnailUrl ? ( <img - src={uploadStatus.thumbnailUrl} + src={uploadStatus.thumbnailUrl} alt="Uploading thumbnail" className="object-cover w-full h-full" /> ) : (
🧹 Nitpick comments (12)
apps/web/app/api/desktop/[...route]/video.ts (2)
128-136: Header‑gated create insert looks good; extract a shared min‑version constant and simplify boolean.Avoid magic numbers and reuse across endpoints.
Apply this diff:
- const xCapVersion = c.req.header("X-Cap-Desktop-Version"); - const clientSupportsUploadProgress = xCapVersion - ? isAtLeastSemver(xCapVersion, 0, 3, 68) - : false; + const xCapVersion = c.req.header("X-Cap-Desktop-Version"); + const clientSupportsUploadProgress = + !!xCapVersion && + isAtLeastSemver( + xCapVersion, + MIN_PROGRESS_VERSION.major, + MIN_PROGRESS_VERSION.minor, + MIN_PROGRESS_VERSION.patch, + );Add near the top of the file:
const MIN_PROGRESS_VERSION = { major: 0, minor: 3, patch: 68 } as const;
321-343: Semver helper: tighten parsing and accept common input variants.Trim input, accept uppercase “V”, and fully anchor the regex (support optional build metadata). Behavior is unchanged otherwise.
-function isAtLeastSemver( +function isAtLeastSemver( versionString: string, major: number, minor: number, patch: number, ): boolean { - const match = versionString - .replace(/^v/, "") - .match(/^(\d+)\.(\d+)\.(\d+)(?:-(.+))?/); + const match = versionString + .trim() + .replace(/^v/i, "") + .match(/^(\d+)\.(\d+)\.(\d+)(?:-([0-9A-Za-z.-]+))?(?:\+[0-9A-Za-z.-]+)?$/); if (!match) return false; const [, vMajor, vMinor, vPatch, prerelease] = match; const M = vMajor ? parseInt(vMajor, 10) || 0 : 0; const m = vMinor ? parseInt(vMinor, 10) || 0 : 0; const p = vPatch ? parseInt(vPatch, 10) || 0 : 0; if (M > major) return true; if (M < major) return false; if (m > minor) return true; if (m < minor) return false; if (p > patch) return true; if (p < patch) return false; // Equal triplet: accept only non-prerelease return !prerelease; }apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx (5)
425-430: Don't hide the loading overlay whenever any uploadProgress exists. Use status flags.If uploadProgress is non-null but not "uploading" (e.g., queued/processing), the spinner overlay hides prematurely. Gate by isUploading/isUploadFailed instead of truthiness.
- videoLoaded || !!uploadProgress + videoLoaded || isUploading || isUploadFailed ? "opacity-0 pointer-events-none" : "opacity-100",
341-341: Also null the timeout ref after clearing to avoid stale handles.Ensures we don’t double-clear and keeps the ref consistent with other paths.
- if (retryTimeout.current) clearTimeout(retryTimeout.current); + if (retryTimeout.current) { + clearTimeout(retryTimeout.current); + retryTimeout.current = null; + }
66-66: Timer type should be browser-compatible.NodeJS.Timeout is incorrect in browsers; use ReturnType.
const retryTimeout = useRef<ReturnType<typeof setTimeout> | null>(null);
489-503: Clickable div for Play lacks semantics; use a button with aria-label.Improves keyboard access and a11y without UI change.
Example:
<button type="button" aria-label="Play" onClick={() => videoRef.current?.play()} className="flex absolute inset-0 z-10 justify-center items-center m-auto bg-blue-500 rounded-full transition-colors transform cursor-pointer hover:bg-blue-600 focus:outline-none focus:ring-2 focus:ring-blue-400 size-12 xs:size-20 md:size-32" > <FontAwesomeIcon icon={faPlay} className="text-white size-4 xs:size-8 md:size-12" /> </button>
1-1: Rename file to kebab-case to match repo guidelines.CapVideoPlayer.tsx → cap-video-player.tsx. Update imports accordingly.
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
25-30: Feature flag implementation looks good, but consider removing commented debug code.The beta flag implementation effectively gates the upload progress feature behind localStorage. However, the commented debug line (Line 26) should be removed before merging to production.
-// TODO: Remove this once we are more confident in the feature -// localStorage.setItem("betaUploadProgress", "true"); +// TODO: Remove this once we are more confident in the featureapps/web/components/VideoThumbnail.tsx (1)
73-76: Use undefined over null for optional capId; tighten types; avoid refetch in deps
- Prefer undefined for “missing” values in TS and avoid nullable unions here.
- Remove
imageUrl.refetchfrom the effect deps to prevent unnecessary re-renders; it’s stable but not needed.- Don’t cast
objectFitto any; type the prop and style correctly.- const uploadingCapId = - uploadStatus && "capId" in uploadStatus ? uploadStatus.capId : null; + const uploadingCapId = + uploadStatus && "capId" in uploadStatus ? uploadStatus.capId : undefined;Outside this hunk:
- In props:
objectFit?: React.CSSProperties["objectFit"]- Where used:
style={{ objectFit }}- Effect deps:
useEffect(() => { imageUrl.refetch(); }, [uploadingCapId]);apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (2)
51-64: Only install beforeunload handler while uploadingAvoid a global listener when idle; install/uninstall only while a status is active.
-useEffect(() => { - const handleBeforeUnload = (e: BeforeUnloadEvent) => { - if (state?.status) { - e.preventDefault(); - e.returnValue = ""; - return ""; - } - }; - window.addEventListener("beforeunload", handleBeforeUnload); - return () => window.removeEventListener("beforeunload", handleBeforeUnload); -}, [state]); +useEffect(() => { + if (!state?.status) return; + const handleBeforeUnload = (e: BeforeUnloadEvent) => { + e.preventDefault(); + e.returnValue = ""; + return ""; + }; + window.addEventListener("beforeunload", handleBeforeUnload); + return () => window.removeEventListener("beforeunload", handleBeforeUnload); +}, [state?.status]);
19-33: Prefer shared ID type for capIdUse the shared VideoId type for stricter consistency across app/web-domain.
Example (outside this hunk):
import type { Video } from "@cap/web-domain";- Replace
capId: stringwithcapId: Video.VideoIdin all union members.apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
290-296: Revoke object URL for thumbnail to prevent memory leak
thumbnailUrl(fromcreateObjectURL) is never revoked. Track it and revoke in finally.Example change (outside exact hunk lines):
// before try { let thumbnailUrlRef: string | undefined; try { // ... const thumbnailBlob = await captureThumbnail(); const thumbnailUrl = thumbnailBlob ? URL.createObjectURL(thumbnailBlob) : undefined; thumbnailUrlRef = thumbnailUrl; setUploadStatus({ status: "uploadingVideo", capId: uploadId, progress: 0, thumbnailUrl }); // ... } finally { setUploadStatus(undefined); if (thumbnailUrlRef) URL.revokeObjectURL(thumbnailUrlRef); if (inputRef.current) inputRef.current.value = ""; }Also applies to: 451-452
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/app/(org)/dashboard/caps/Caps.tsx(3 hunks)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(10 hunks)apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx(3 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx(1 hunks)apps/web/app/(org)/dashboard/folder/[id]/page.tsx(2 hunks)apps/web/app/api/desktop/[...route]/video.ts(3 hunks)apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx(2 hunks)apps/web/app/s/[videoId]/_components/ProgressCircle.tsx(1 hunks)apps/web/components/VideoThumbnail.tsx(1 hunks)apps/web/components/pages/_components/ComparePlans.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/components/pages/_components/ComparePlans.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(org)/dashboard/folder/[id]/page.tsx
🧰 Additional context used
📓 Path-based instructions (5)
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/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/api/desktop/[...route]/video.tsapps/web/components/VideoThumbnail.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.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/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/api/desktop/[...route]/video.tsapps/web/components/VideoThumbnail.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.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/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/app/api/desktop/[...route]/video.tsapps/web/components/VideoThumbnail.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
React/Solid components should be named using PascalCase
Files:
apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsxapps/web/components/VideoThumbnail.tsxapps/web/app/s/[videoId]/_components/ProgressCircle.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx
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/desktop/[...route]/video.ts
🧬 Code graph analysis (7)
apps/web/app/api/desktop/[...route]/video.ts (2)
packages/database/index.ts (1)
db(30-35)packages/database/schema.ts (1)
videoUploads(644-650)
apps/web/components/VideoThumbnail.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(39-46)
apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (1)
packages/web-domain/src/Video.ts (3)
Video(13-35)VideoId(9-9)VideoId(10-10)
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (2)
apps/web/app/(org)/dashboard/Contexts.tsx (1)
useDashboardContext(42-42)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(39-46)
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(39-46)
apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(39-46)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
UploadCapButton(15-510)
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (2)
packages/utils/src/helpers.ts (2)
getProgressCircleConfig(63-68)calculateStrokeDashoffset(70-75)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (2)
useUploadingContext(39-46)UploadStatus(11-33)
⏰ 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 (10)
apps/web/app/api/desktop/[...route]/video.ts (1)
300-306: Progress POST still creates rows for legacy clients; gate the insert and make it race‑safe.As implemented, legacy clients (no/old header) can still create
video_uploadsvia POST, which defeats “remove progress for legacy clients.” Also, the insert can 500 under concurrent updates (duplicate PK). Gate by version and use on-duplicate update.Apply this diff:
- if (result.rowsAffected === 0) - await db().insert(videoUploads).values({ - videoId, - uploaded, - total, - updatedAt, - }); + if (result.rowsAffected === 0) { + const xCapVersion = c.req.header("X-Cap-Desktop-Version"); + const clientSupportsUploadProgress = + !!xCapVersion && + isAtLeastSemver( + xCapVersion, + MIN_PROGRESS_VERSION.major, + MIN_PROGRESS_VERSION.minor, + MIN_PROGRESS_VERSION.patch, + ); + if (clientSupportsUploadProgress) { + await db() + .insert(videoUploads) + .values({ videoId, uploaded, total, updatedAt }) + .onDuplicateKeyUpdate({ + set: { uploaded, total, updatedAt }, + }); + } + }Alternative (stricter): require a pre‑existing row to avoid any creation in POST:
- if (result.rowsAffected === 0) { - ... - } + if (result.rowsAffected === 0) { + // No creation in POST; legacy/unsupported clients will be ignored. + // Intentionally do nothing. + }apps/web/app/s/[videoId]/_components/ProgressCircle.tsx (2)
42-43: Verify refetchInterval respects the beta flag correctly.The refetchInterval callback includes an
enabledcheck, which should now reference the computedenabledvariable (Line 33) rather than potentially using the raw parameter.The refetchInterval logic appears correct, but please confirm that the
enabledvariable referenced in the callback (Line 43) is the computed one from Line 33, not the raw parameter. The current implementation should work correctly since theenabledvariable from Line 33 is in scope.
32-33: Parameter rename is safe — callers verified.Renaming
enabled→enabledRawis fine; all callers pass positional boolean expressions (no named-argument usage). Verified call sites:
- apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx — useUploadProgress(videoId, hasActiveUpload || false)
- apps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx — useUploadProgress(videoId, hasActiveUpload || false)
- apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx — useUploadProgress(cap.id, cap.hasActiveUpload || false)
- Definition: apps/web/app/s/[videoId]/_components/ProgressCircle.tsx — export function useUploadProgress(videoId: Video.VideoId, enabledRaw: boolean)
apps/web/app/(org)/dashboard/caps/Caps.tsx (2)
261-264: Correct union narrowing for capIdUsing
"capId" in uploadStatusis the right way to access the discriminated member. Looks good.
291-291: Upload button integration looks goodSwitch to
<UploadCapButton size="sm" />aligns with the centralized upload state; no leaking callbacks.apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (1)
12-18: Progress calc and friendly-status mapping look solidThe stroke offset gating and status label mapping are consistent with the UploadStatus union.
Also applies to: 40-42, 90-104
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (3)
49-67: Status state machine transitions look correctTransitions through parsing → creating → converting with progress are coherent and match the UploadStatus union.
Also applies to: 81-119
372-379: Final progress uses potentially stale/zero totals
progressTracker.getTotal() || 1can report 1/1 or 0/0 if no progress events fired. Use the actual blob size for an accurate terminal update.- // Guarantee final 100% progress update - const total = progressTracker.getTotal() || 1; - sendProgressUpdate(uploadId, total, total); + // Guarantee final 100% progress update using actual size + const totalBytes = optimizedBlob.size || progressTracker.getTotal() || 1; + sendProgressUpdate(uploadId, totalBytes, totalBytes);
456-479: Progress endpoint is gated by X-Cap-Desktop-Version — confirm header intentBackend reads X-Cap-Desktop-Version and enables progress only for semver >= 0.3.68 (apps/web/app/api/desktop/[...route]/video.ts:128–131). Desktop uploader sends that header (apps/desktop/src-tauri/src/upload.rs:188–193). The web call in UploadCapButton.tsx currently omits the version header (apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx:456–479) — so those requests will be treated as legacy and ignored. Add the X-Cap-Desktop-Version header with the client semver if you intend to enable progress here; otherwise keep omission to deliberately disable progress.
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (1)
30-36: Verify UploadingProvider wrappingLGTM on deriving isUploading/uploadingCapId from context — confirm this component is always rendered under UploadingProvider to avoid a runtime throw from useUploadingContext().
File: apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (lines 30–36)
rg returned "unrecognized file type: tsx" — unable to verify. From repo root re-run:
rg -n -C2 'useUploadingContext' || true rg -n -C2 'UploadingProvider' || true grep -RIn --line-number 'useUploadingContext' . || trueIf still no hits, inspect top-level provider composition (app/layout.tsx or pages/_app.tsx) to ensure UploadingProvider wraps this route.
It's cringe that they show as 0% for the whole upload process so let's just skip any progress reporting for uploads from old clients to maintain the old behavior.
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores