Improve uploading context #1051
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces local upload state with a TanStack store and a new useUploadingStatus hook, refactors components to read from the store, updates upload flow to use QueryClient for thumbnail refetching, removes userId from VideoThumbnail in favor of imageUrlQuery(videoId), and adjusts the thumbnail API to require only videoId. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UploadCapButton
participant Store as uploadingStore
participant Server as Backend
participant Q as QueryClient
participant API as /api/thumbnail
User->>UI: Select file & confirm
UI->>Store: setUploadStatus({ capId, progress })
Note over Store: useUploadingStatus() → [isUploading, uploadingCapId]
UI->>Server: upload video + screenshot
alt screenshot available after upload
UI->>Q: refetchQueries(imageUrlQuery(capId))
Q->>API: GET /api/thumbnail?videoId=capId
API-->>Q: 200 { screen: url } or 404
Q-->>UI: cache updated
end
UI->>Store: setUploadStatus(undefined)
sequenceDiagram
autonumber
participant Component as Caps/FolderVideosSection
participant Hook as useUploadingStatus
participant List as visibleVideos
Component->>Hook: read [isUploading, uploadingCapId]
Hook-->>Component: tuple
Component->>List: filter videos (exclude uploadingCapId when isUploading)
Component-->>Component: render filtered list
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 ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/api/thumbnail/route.ts (2)
15-25: Fix error message to match new API shape (no userId).Update message to reference only videoId.
Apply this diff:
- if (!videoId) + if (!videoId) return new Response( JSON.stringify({ error: true, - message: "userId or videoId not supplied", + message: "videoId not supplied", }), { status: 400, headers: getHeaders(origin), }, );
27-35: Auth/authorization missing — endpoint can leak private thumbnails.This route signs and returns a thumbnail URL without verifying ownership or public visibility. Anyone with a videoId can retrieve a presigned URL.
Immediate stop‑gap (until full HttpApi migration):
- If you can’t check session here, at least gate on public videos:
const [query] = await db() @@ if (!query) return new Response( JSON.stringify({ error: true, message: "Video not found" }), { status: 404, headers: getHeaders(origin), }, ); + + // Temporary guard: only allow public videos + if (!query.video.public) { + return new Response( + JSON.stringify({ error: true, message: "Unauthorized" }), + { status: 401, headers: getHeaders(origin) }, + ); + }Preferred fix (align with repo guidelines):
- Rebuild this route with @effect/platform HttpApi, using provideOptionalAuth:
- Allow when video is public.
- If not public, require auth and owner match (or org permission).
- Map domain errors with HttpApiError.* and export apiToHandler(ApiLive).
I can draft the HttpApi route and the Layer wiring if you want to take this in this PR.
🧹 Nitpick comments (9)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (3)
50-62: Reduce unnecessary re-renders by selecting once and deriving tuple outside.Select uploadStatus once to avoid selector returning a new tuple object on each store update.
Apply this diff:
-export function useUploadingStatus() { - const { uploadingStore } = useUploadingContext(); - return useStore( - uploadingStore, - (s) => - [ - s.uploadStatus !== undefined, - s.uploadStatus && "capId" in s.uploadStatus - ? s.uploadStatus.capId - : null, - ] as const, - ); -} +export function useUploadingStatus() { + const { uploadingStore } = useUploadingContext(); + const uploadStatus = useStore(uploadingStore, (s) => s.uploadStatus); + return [ + uploadStatus !== undefined, + uploadStatus && "capId" in uploadStatus ? uploadStatus.capId : null, + ] as const; +}
69-86: Stabilize context value to prevent provider-wide re-renders.Memoize the value and callback so consumers don’t re-render due to changing function/object identities.
Apply this diff:
- return ( - <UploadingContext.Provider - value={{ - uploadingStore, - setUploadStatus: (status: UploadStatus | undefined) => { - uploadingStore.setState((state) => ({ - ...state, - uploadStatus: status, - })); - }, - }} - > + const setUploadStatus = React.useCallback( + (status: UploadStatus | undefined) => { + uploadingStore.setState((state) => ({ + ...state, + uploadStatus: status, + })); + }, + [uploadingStore], + ); + + const value = React.useMemo( + () => ({ uploadingStore, setUploadStatus }), + [uploadingStore, setUploadStatus], + ); + + return ( + <UploadingContext.Provider value={value}> {children} <ForbidLeaveWhenUploading /> </UploadingContext.Provider> );
88-88: Remove inline comment per repo guidelines.Project rule forbids inline/block comments in TS/TSX.
Apply this diff:
-// Separated to prevent rerendering whole treeapps/web/app/(org)/dashboard/caps/Caps.tsx (1)
27-27: Remove unused import.useUploadingContext isn’t used here.
Apply this diff:
-import { useUploadingContext, useUploadingStatus } from "./UploadingContext"; +import { useUploadingStatus } from "./UploadingContext";apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (2)
5-5: Remove unused import.useStore isn’t used.
Apply this diff:
-import { useStore } from "@tanstack/react-store";
17-20: Trim imports to only what’s used.useUploadingContext is unused.
Apply this diff:
-import { - useUploadingContext, - useUploadingStatus, -} from "../../../caps/UploadingContext"; +import { useUploadingStatus } from "../../../caps/UploadingContext";apps/web/components/VideoThumbnail.tsx (1)
44-55: Clarify error message.Message should reflect thumbnail fetch, not “pre-signed URLs.”
Apply this diff:
- } else throw new Error("Failed to fetch pre-signed URLs"); + } else throw new Error("Failed to fetch thumbnail URL");apps/web/app/api/thumbnail/route.ts (1)
54-57: Type safety nit (optional).contents.find((item) => ...) relies on any-ish S3 types. If available, narrow item to the SDK’s listed object type to avoid accidental undefined access.
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (1)
19-19: Decouple query helper from UI component moduleImporting imageUrlQuery from a component couples data/query concerns to UI. Consider moving the query helper (and its queryKey) into a shared queries module (e.g., "@/queries/video") to avoid circular deps and improve reuse.
Apply this minimal import change if you extract the helper:
-import { imageUrlQuery } from "@/components/VideoThumbnail"; +import { imageUrlQuery } from "@/queries/video";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
apps/web/app/(org)/dashboard/caps/Caps.tsx(2 hunks)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx(4 hunks)apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx(0 hunks)apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx(6 hunks)apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx(1 hunks)apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx(3 hunks)apps/web/app/(org)/dashboard/spaces/[spaceId]/components/VideoCard.tsx(0 hunks)apps/web/app/api/thumbnail/route.ts(3 hunks)apps/web/components/VideoThumbnail.tsx(2 hunks)apps/web/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- apps/web/app/(org)/dashboard/spaces/[spaceId]/components/VideoCard.tsx
- apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx
🧰 Additional context used
📓 Path-based instructions (7)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/components/VideoThumbnail.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/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,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/components/VideoThumbnail.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/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; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/components/VideoThumbnail.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/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,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/components/VideoThumbnail.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/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/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/components/VideoThumbnail.tsxapps/web/app/api/thumbnail/route.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/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/app/api/**/route.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/route.ts: Place API routes only under apps/web/app/api and implement each route in a route.ts file
Construct API routes with @effect/platform HttpApi/HttpApiBuilder and export only the handler from apiToHandler(ApiLive)
Map domain errors to transport errors with HttpApiError.* and keep error translation exhaustive
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guest routes; avoid duplicate session lookups
Provide dependencies via Layer.provide in API routes instead of manual provideService calls
Files:
apps/web/app/api/thumbnail/route.ts
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/api/thumbnail/route.tsapps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsxapps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsxapps/web/app/(org)/dashboard/caps/UploadingContext.tsx
🧠 Learnings (2)
📚 Learning: 2025-09-22T14:17:47.380Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.380Z
Learning: Applies to apps/web/**/*.{ts,tsx} : Use TanStack Query v5 for all client-side server state and data fetching in the web app
Applied to files:
apps/web/package.json
📚 Learning: 2025-09-22T14:17:47.380Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.380Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : In the desktop app, use tanstack/solid-query for server state management
Applied to files:
apps/web/package.json
🧬 Code graph analysis (4)
apps/web/app/api/thumbnail/route.ts (3)
packages/database/index.ts (1)
db(30-35)apps/web/utils/helpers.ts (1)
getHeaders(18-26)apps/web/utils/s3.ts (1)
createBucketProvider(374-397)
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingStatus(50-62)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingStatus(50-62)
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (2)
packages/utils/src/helpers.ts (1)
getProgressCircleConfig(63-68)apps/web/app/(org)/dashboard/caps/UploadingContext.tsx (1)
useUploadingContext(41-48)
⏰ 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 (9)
apps/web/package.json (1)
65-65: Dependency addition looks good.Adding @tanstack/store alongside @tanstack/react-store is appropriate for the new Store usage.
apps/web/app/(org)/dashboard/caps/components/UploadPlaceholderCard.tsx (1)
11-12: LGTM — switched to store selector cleanly.Reading uploadStatus via useStore(uploadingStore, ...) is correct and keeps this component independent from context value shape.
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
260-267: LGTM — new hook usage and filtering logic are sound.Tuple from useUploadingStatus and filtering out the uploading cap id are correct.
apps/web/app/(org)/dashboard/folder/[id]/components/FolderVideosSection.tsx (1)
161-168: LGTM — central hook adoption and visibility filtering are correct.Good alignment with the new UploadingContext API.
apps/web/components/VideoThumbnail.tsx (1)
56-66: userId prop removed — no call sites pass itChecked apps/web/components/VideoThumbnail.tsx and inspected usages in apps/web/app/(org)/dashboard/spaces/[spaceId]/components/VideoCard.tsx and apps/web/app/(org)/dashboard/caps/components/CapCard/CapCard.tsx — none pass userId.
apps/web/app/(org)/dashboard/caps/components/UploadCapButton.tsx (4)
7-9: LGTM: QueryClient + TanStack store import is appropriateType-only import for QueryClient and using useQueryClient locally is correct.
31-36: LGTM: Store‑derived isUploading and QueryClient acquisitionReading isUploading via a selector on uploadingStore minimizes re-renders and cleanly disables/spinners the button. useQueryClient usage is scoped correctly.
54-60: LGTM: Injecting QueryClient into the upload flowPassing queryClient into legacyUploadCap makes the side effect explicit and testable.
92-97: LGTM: Signature updated with typed QueryClientType-only import and explicit parameter typing are correct.
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 (2)
apps/web/app/api/thumbnail/route.ts (2)
15-25: Fix misleading error message and set JSON content typeThe guard only checks for videoId; message mentions userId. Also add Content-Type for JSON responses.
Apply this diff:
- message: "userId or videoId not supplied", + message: "videoId not supplied", ... - headers: getHeaders(origin), + headers: { ...getHeaders(origin), "Content-Type": "application/json" },
27-35: Require auth & visibility check before returning signed thumbnail URLs
- apps/web/app/api/thumbnail/route.ts currently returns a signed URL to anyone with a videoId — add HttpAuthMiddleware (or HttpApiBuilder + provideOptionalAuth for public cases) and enforce authorization (owner/org membership or video visibility) before generating the URL.
- Validate videoId as a UUID at the schema layer.
Confirm whether thumbnails are intended to be public; if not, I will sketch the HttpApi + auth + visibility-check implementation.
🧹 Nitpick comments (3)
apps/web/app/api/thumbnail/route.ts (3)
45-49: Wrap provider creation in try/catch to return consistent 500 JSONcreateBucketProvider can throw; move it into the try so errors are surfaced via the structured 500 response.
Apply this diff:
- const prefix = `${query.video.ownerId}/${query.video.id}/`; - const bucketProvider = await createBucketProvider(query.bucket); - - try { + try { + const prefix = `${query.video.ownerId}/${query.video.id}/`; + const bucketProvider = await createBucketProvider(query.bucket);
54-56: Tighten types for contents items; consider avoiding list if naming is stableRemove implicit any by annotating the item shape. Optionally, skip ListObjects and directly sign the known key to reduce latency/cost.
Apply this diff:
- const thumbnailKey = contents.find((item) => + const thumbnailKey = (contents as Array<{ Key?: string }>).find((item) => item.Key?.endsWith("screen-capture.jpg"), )?.Key;If the thumbnail is always at a deterministic path, prefer:
const key = `${prefix}screen-capture.jpg`; const thumbnailUrl = await bucketProvider.getSignedObjectUrl(key);and 404 on NotFound.
58-68: Set JSON content type on error responsesEnsure clients parse JSON reliably.
Apply this diff:
- headers: getHeaders(origin), + headers: { ...getHeaders(origin), "Content-Type": "application/json" },Consider doing the same for the 200 and 500 responses for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/thumbnail/route.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/web/app/api/**/route.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/route.ts: Place API routes only under apps/web/app/api and implement each route in a route.ts file
Construct API routes with @effect/platform HttpApi/HttpApiBuilder and export only the handler from apiToHandler(ApiLive)
Map domain errors to transport errors with HttpApiError.* and keep error translation exhaustive
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guest routes; avoid duplicate session lookups
Provide dependencies via Layer.provide in API routes instead of manual provideService calls
Files:
apps/web/app/api/thumbnail/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/app/api/thumbnail/route.ts
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/api/thumbnail/route.ts
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/app/api/thumbnail/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/app/api/thumbnail/route.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/api/thumbnail/route.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/api/thumbnail/route.ts
🧬 Code graph analysis (1)
apps/web/app/api/thumbnail/route.ts (3)
packages/database/index.ts (1)
db(30-35)apps/web/utils/helpers.ts (1)
getHeaders(18-26)apps/web/utils/s3.ts (1)
createBucketProvider(374-397)
⏰ 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/api/thumbnail/route.ts (1)
36-43: Good change: 404 for missing videoReturning 404 is correct and addresses the previous review.
Improve React render performance on the dashboard and setup things better to potentially allow multiple uploads at once via the dashboard.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores