Uploader Bugs (round 2) #1264
Uploader Bugs (round 2) #1264
Conversation
|
Warning Rate limit exceeded@oscartbeaumont has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughSplit Videos.getById into viewer and owner-specific methods ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Videos
participant Policy
rect rgb(220,240,220)
Note over Client,API: Viewer request flow
Client->>API: GET /embed or /s or /playlist
API->>Videos: getByIdForViewing(videoId)
Videos->>Policy: (optional) viewer checks
Policy-->>Videos: allow / deny
Videos-->>API: video data | NotFound
API-->>Client: metadata / response
end
sequenceDiagram
participant Client
participant API
participant CurrentUser
participant Videos
participant Policy
participant S3
rect rgb(240,230,230)
Note over Client,API: Owner multipart upload flow
Client->>API: multipart initiate / complete
API->>CurrentUser: resolve user
CurrentUser-->>API: user
API->>Videos: getByIdForOwner(Video.VideoId.make(id)) [with CurrentUser provided]
Videos->>Policy: owner check
Policy-->>Videos: allow / deny
Videos-->>API: video or NotFound
API->>S3: listObjects / listParts
alt listedObjects.Contents present
API->>S3: deleteObjects (guarded when objects.length>0)
S3-->>API: delete response
end
API-->>Client: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/app/api/desktop/[...route]/video.ts (2)
294-300: Fix S3 DeleteObjects guard (should allow 1+ keys) and filter undefined Keys.Using
> 1skips valid single-object deletions and can leak data. Also, map may include{ Key: undefined }which S3 rejects. Compute keys first, then conditionally delete.Apply:
- if (listedObjects.Contents && listedObjects.Contents.length > 1) - yield* bucket.deleteObjects( - listedObjects.Contents.map((content: any) => ({ - Key: content.Key, - })), - ); + const keys = + (listedObjects.Contents ?? []) + .flatMap((content: any) => (content?.Key ? [{ Key: content.Key }] : [])); + if (keys.length > 0) { + yield* bucket.deleteObjects(keys); + }
279-284: Authorization bug: deletion of videoUploads without owner check enables cross-tenant deletion.
DELETE /deleteremoves anyvideoUploadsbyvideoIdbefore verifying ownership. A malicious user can delete others’ upload progress by guessing IDs.Apply (and return 403 early when not owner):
- await db().delete(videoUploads).where(eq(videoUploads.videoId, videoId)); - - await db() - .delete(videos) - .where(and(eq(videos.id, videoId), eq(videos.ownerId, user.id))); + // Enforce ownership before any side effects + if (result.video.ownerId !== user.id) { + return c.json({ error: true, message: "Forbidden" }, { status: 403 }); + } + + await db() + .delete(videoUploads) + .where(eq(videoUploads.videoId, videoId)); + + await db() + .delete(videos) + .where(eq(videos.id, videoId));Optional hardening: use the actual owner for S3 prefix.
- const listedObjects = yield* bucket.listObjects({ - prefix: `${user.id}/${videoId}/`, - }); + const listedObjects = yield* bucket.listObjects({ + prefix: `${result.video.ownerId}/${videoId}/`, + });apps/web/actions/organization/delete-space.ts (1)
80-90: Allow single-object deletes and filter Keys before calling DeleteObjects.
> 1skips valid deletes when only one object exists; mapping may include undefined Keys. Build the keys array, guard onkeys.length > 0, and log the actual count.Apply:
- if (listedObjects.Contents && listedObjects.Contents.length > 1) { - yield* bucket.deleteObjects( - listedObjects.Contents.map((content) => ({ - Key: content.Key, - })), - ); - - console.log( - `Deleted ${listedObjects.Contents.length} objects for space ${spaceId}`, - ); - } + const keys = + (listedObjects.Contents ?? []) + .flatMap((content) => (content?.Key ? [{ Key: content.Key }] : [])); + if (keys.length > 0) { + yield* bucket.deleteObjects(keys); + console.log(`Deleted ${keys.length} objects for space ${spaceId}`); + }packages/web-backend/src/Videos/index.ts (1)
72-78: DeleteObjects should run with 1+ keys; filter Keys first.Same guard bug:
> 1skips single-object deletes. Also ensure Keys are defined.Apply:
- if (listedObjects.Contents && listedObjects.Contents.length > 1) { - yield* bucket.deleteObjects( - listedObjects.Contents.map((content) => ({ - Key: content.Key, - })), - ); - } + const keys = + (listedObjects.Contents ?? []) + .flatMap((c) => (c?.Key ? [{ Key: c.Key }] : [])); + if (keys.length > 0) { + yield* bucket.deleteObjects(keys); + }
🧹 Nitpick comments (2)
apps/web/app/api/upload/[...route]/multipart.ts (2)
78-80: Avoid double-injecting CurrentUser; rely on provideOptionalAuth.
provideOptionalAuthalready providesCurrentUserfrom the session; manually providingCurrentUserfromc.get("user")can drift if they ever diverge. Simplify by removing the extra provide.[Based on learnings]
Apply:- ).pipe( - provideOptionalAuth, - Effect.tapError(Effect.logError), - Effect.catchAll((e) => { /* ... */ }), - Effect.provideService(CurrentUser, user), - runPromise, - ); + ).pipe( + provideOptionalAuth, + Effect.tapError(Effect.logError), + Effect.catchAll((e) => { /* ... */ }), + runPromise, + );
474-478: Same here: remove manual CurrentUser injection.Redundant with
provideOptionalAuth; keep a single source of truth for auth context.[Based on learnings]
Apply:- ).pipe( - provideOptionalAuth, - Effect.provideService(CurrentUser, user), - runPromise, - ); + ).pipe(provideOptionalAuth, runPromise);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/actions/organization/delete-space.ts(1 hunks)apps/web/app/api/desktop/[...route]/video.ts(2 hunks)apps/web/app/api/playlist/route.ts(1 hunks)apps/web/app/api/upload/[...route]/multipart.ts(6 hunks)apps/web/app/embed/[videoId]/page.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(1 hunks)packages/web-backend/src/Videos/index.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/api/playlist/route.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/page.tsxpackages/web-backend/src/Videos/index.tsapps/web/app/embed/[videoId]/page.tsxapps/web/actions/organization/delete-space.tsapps/web/app/api/upload/[...route]/multipart.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/playlist/route.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/page.tsxpackages/web-backend/src/Videos/index.tsapps/web/app/embed/[videoId]/page.tsxapps/web/actions/organization/delete-space.tsapps/web/app/api/upload/[...route]/multipart.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/playlist/route.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/actions/organization/delete-space.tsapps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/playlist/route.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/api/upload/[...route]/multipart.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 fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/playlist/route.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/actions/organization/delete-space.tsapps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/playlist/route.tsapps/web/app/api/desktop/[...route]/video.tsapps/web/app/s/[videoId]/page.tsxapps/web/app/embed/[videoId]/page.tsxapps/web/app/api/upload/[...route]/multipart.ts
apps/web/actions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
All Groq/OpenAI calls must execute in Next.js Server Actions under apps/web/actions; do not invoke AI providers elsewhere
Files:
apps/web/actions/organization/delete-space.ts
🧠 Learnings (2)
📚 Learning: 2025-10-14T10:15:44.019Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.019Z
Learning: Applies to apps/web/app/**/*.{ts,tsx} : Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/web/app/api/* : On the server, always run effects through `EffectRuntime.runPromise` after `provideOptionalAuth` to ensure cookies and per-request context are attached.
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
🧬 Code graph analysis (4)
apps/web/app/s/[videoId]/page.tsx (1)
packages/web-backend/src/Videos/index.ts (1)
Videos(12-243)
packages/web-backend/src/Videos/index.ts (2)
packages/web-domain/src/Video.ts (3)
Video(16-59)VideoId(12-12)VideoId(13-13)packages/web-domain/src/Policy.ts (2)
Policy(8-12)policy(29-41)
apps/web/app/embed/[videoId]/page.tsx (1)
packages/web-backend/src/Videos/index.ts (1)
Videos(12-243)
apps/web/app/api/upload/[...route]/multipart.ts (3)
packages/web-backend/src/Videos/index.ts (1)
Videos(12-243)packages/web-domain/src/Authentication.ts (1)
CurrentUser(8-15)packages/web-backend/src/Auth.ts (1)
provideOptionalAuth(91-113)
⏰ 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). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
apps/web/app/api/desktop/[...route]/video.ts (1)
23-23: Import cleanup LGTM.The streamlined desktop utils import looks correct.
apps/web/app/api/upload/[...route]/multipart.ts (2)
49-53: Owner-scoped video resolution is correct.Deriving
videoIdfrom body/fileKey and enforcingvideos.getByIdForOwnermatches the authorization objective.If there are any remaining
getByIdowner flows, consider migrating them similarly.Also applies to: 58-60
235-239: Completing upload with owner check LGTM.The switch to Effect pipeline and
getByIdForOwnerbefore mutating DB/S3 is correct and consistent.Also applies to: 252-254
packages/web-backend/src/Videos/index.ts (2)
19-26: Viewer/Owner split is solid.Clearer API surface and scoped policies improve safety. Span names also make tracing clearer.
Also applies to: 27-34, 41-47
215-221: Switch to getByIdForViewer for analytics LGTM.Matches viewer context and avoids over-permissive access.
apps/web/app/embed/[videoId]/page.tsx (1)
32-32: Verification complete: No insecuregetByIdcalls found in viewer-facing pages.All occurrences use properly scoped methods (
getByIdForOwnerfor owner context,getByIdForViewerfor public context). The code at line 32 is correct, and the broader codebase follows the established policy model consistently.apps/web/app/s/[videoId]/page.tsx (1)
137-137: LGTM! Appropriate authorization for viewer context.The change from
getByIdtogetByIdForViewercorrectly applies viewer-specific access control for this public share page metadata generation. The existing error handling forPolicyDeniedandVerifyVideoPasswordErrorremains appropriate.apps/web/app/api/playlist/route.ts (1)
50-58: LGTM! Correct authorization for public content delivery.The switch to
getByIdForViewerproperly enforces viewer-level access control for this public API endpoint. The error handling correctly mapsPolicyDeniedtoUnauthorizedandVerifyVideoPasswordErrortoForbidden, maintaining appropriate HTTP semantics for content access.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/web-backend/src/S3Buckets/S3BucketAccess.ts (1)
259-259: Good addition of URL expiration for security.Adding
expiresIn: 3600ensures that presigned upload URLs expire after 1 hour, which is a security best practice. This is consistent withgetSignedObjectUrl(line 48) and provides sufficient time for uploading individual parts in a multipart upload.Optional enhancement: Consider making the expiration configurable by accepting an optional
signingArgsparameter (similar togetPresignedPutUrlat line 188), which would provide flexibility for different upload scenarios:getPresignedUploadPartUrl: ( key: string, uploadId: string, partNumber: number, args?: Omit< S3.UploadPartCommandInput, "Key" | "Bucket" | "PartNumber" | "UploadId" >, + signingArgs?: RequestPresigningArguments, ) => wrapS3Promise( provider.getPublic.pipe( Effect.map((client) => S3Presigner.getSignedUrl( client, new S3.UploadPartCommand({ ...args, Bucket: provider.bucket, Key: key, UploadId: uploadId, PartNumber: partNumber, }), - { expiresIn: 3600 }, + { expiresIn: 3600, ...signingArgs }, ), ), ), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web-backend/src/S3Buckets/S3BucketAccess.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
packages/web-backend/src/S3Buckets/S3BucketAccess.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:
packages/web-backend/src/S3Buckets/S3BucketAccess.ts
⏰ 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)
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)
packages/web-backend/src/Videos/index.ts (1)
72-78: Empty array check won't prevent deleting 0 objects.The condition
if (listedObjects.Contents)will be truthy for an empty array[], so the code will still attempt to calldeleteObjectswith an empty array. This doesn't fully address the PR objective of "preventing attempts to delete 0 objects from S3."Apply this diff to properly check for non-empty arrays:
-if (listedObjects.Contents) { +if (listedObjects.Contents?.length) { yield* bucket.deleteObjects( listedObjects.Contents.map((content) => ({ Key: content.Key, })), ); }
🧹 Nitpick comments (2)
packages/web-backend/src/Videos/index.ts (1)
19-25: Update the Effect span name to match the renamed function.The Effect span on line 24 still references
"Videos.getById", but the function has been renamed togetByIdForViewing.Apply this diff:
const getByIdForViewing = (id: Video.VideoId) => repo .getById(id) .pipe( Policy.withPublicPolicy(policy.canView(id)), - Effect.withSpan("Videos.getById"), + Effect.withSpan("Videos.getByIdForViewing"), );apps/web/app/api/upload/[...route]/multipart.ts (1)
67-79: Verify the CurrentUser provision pattern.Both
provideOptionalAuth(line 68) and manualEffect.provideService(CurrentUser, user)(line 78) are used. Looking at theprovideOptionalAuthimplementation, it already providesCurrentUserby reading from cookies/session viagetCurrentUser. The manual provision with the Hono middleware user appears to override this.This pattern seems redundant. Please confirm whether:
- Both are needed due to differences between Hono auth middleware and Effect auth
- Only one should be used
- The order matters for your use case
The same pattern appears in the
/completehandler (lines 475-477).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/actions/organization/delete-space.ts(1 hunks)apps/web/app/api/desktop/[...route]/video.ts(2 hunks)apps/web/app/api/playlist/route.ts(1 hunks)apps/web/app/api/upload/[...route]/multipart.ts(7 hunks)apps/web/app/embed/[videoId]/page.tsx(1 hunks)apps/web/app/s/[videoId]/page.tsx(1 hunks)packages/web-backend/src/S3Buckets/S3BucketAccess.ts(2 hunks)packages/web-backend/src/Videos/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/app/api/desktop/[...route]/video.ts
- apps/web/app/embed/[videoId]/page.tsx
- packages/web-backend/src/S3Buckets/S3BucketAccess.ts
- apps/web/actions/organization/delete-space.ts
- apps/web/app/s/[videoId]/page.tsx
- apps/web/app/api/playlist/route.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
packages/web-backend/src/Videos/index.tsapps/web/app/api/upload/[...route]/multipart.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:
packages/web-backend/src/Videos/index.tsapps/web/app/api/upload/[...route]/multipart.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/upload/[...route]/multipart.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/upload/[...route]/multipart.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 fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/upload/[...route]/multipart.ts
🧠 Learnings (2)
📚 Learning: 2025-10-14T10:15:44.019Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.019Z
Learning: Applies to apps/web/app/**/*.{ts,tsx} : Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/web/app/api/* : On the server, always run effects through `EffectRuntime.runPromise` after `provideOptionalAuth` to ensure cookies and per-request context are attached.
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
🧬 Code graph analysis (2)
packages/web-backend/src/Videos/index.ts (2)
packages/web-domain/src/Video.ts (3)
Video(16-59)VideoId(12-12)VideoId(13-13)packages/web-domain/src/Policy.ts (2)
Policy(8-12)policy(29-41)
apps/web/app/api/upload/[...route]/multipart.ts (5)
packages/web-backend/src/Videos/index.ts (1)
Videos(12-243)packages/web-backend/src/Database.ts (1)
Database(5-15)packages/web-domain/src/Authentication.ts (1)
CurrentUser(8-15)packages/web-backend/src/Auth.ts (1)
provideOptionalAuth(91-113)apps/web/lib/server.ts (1)
runPromise(123-135)
⏰ 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 (5)
packages/web-backend/src/Videos/index.ts (3)
27-33: LGTM!The new
getByIdForOwnermethod correctly enforces owner-only access usingpolicy.isOwner, which addresses the authorization fix mentioned in the PR objectives.
41-46: LGTM!The public API clearly distinguishes between viewing and owner-scoped access patterns with appropriate documentation.
215-221: LGTM!Correctly uses
getByIdForViewingsince analytics should be accessible to anyone with viewing permissions.apps/web/app/api/upload/[...route]/multipart.ts (2)
50-58: LGTM!Correctly constructs the branded
VideoIdtype and usesgetByIdForOwnerto ensure only video owners can initiate uploads.
235-259: LGTM!The handler structure properly returns an Effect pipeline, correctly uses
getByIdForOwnerto validate video ownership before completing the upload, and properly constructs the brandedVideoIdtype.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/app/api/upload/[...route]/multipart.ts (2)
484-485: Provide optional auth before runPromise (per server runtime pattern)For consistency with initiate and our server guidance, include provideOptionalAuth before runPromise to attach per-request context.
- }).pipe(Effect.provideService(CurrentUser, user), runPromise); + }).pipe(provideOptionalAuth, Effect.provideService(CurrentUser, user), runPromise);Based on learnings
99-121: Replace console. with Effect logger for structured, redacted logs*Use Effect.log / Effect.logError to keep logs structured, trace-aware, and redactable; avoid leaking user/video identifiers.
Example:
- console.log(`Creating multipart upload in bucket: ${bucket.bucketName}, content-type: ${finalContentType}, key: ${fileKey}`); + yield* Effect.log(`Creating multipart upload`, { bucket: bucket.bucketName, contentType: finalContentType, key: fileKey });Also prefer not to log raw identifiers in production; if necessary, redact or hash them.
Also applies to: 277-283, 303-315, 328-347, 359-372, 448-451, 470-482
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/api/upload/[...route]/multipart.ts(6 hunks)apps/web/lib/server.ts(2 hunks)packages/web-backend/src/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
packages/web-backend/src/index.tsapps/web/lib/server.tsapps/web/app/api/upload/[...route]/multipart.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:
packages/web-backend/src/index.tsapps/web/lib/server.tsapps/web/app/api/upload/[...route]/multipart.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/lib/server.tsapps/web/app/api/upload/[...route]/multipart.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 fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/lib/server.tsapps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/upload/[...route]/multipart.ts
🧠 Learnings (2)
📚 Learning: 2025-10-14T10:15:44.019Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.019Z
Learning: Applies to apps/web/app/**/*.{ts,tsx} : Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/web/app/api/* : On the server, always run effects through `EffectRuntime.runPromise` after `provideOptionalAuth` to ensure cookies and per-request context are attached.
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
🧬 Code graph analysis (2)
apps/web/lib/server.ts (2)
packages/web-backend/src/index.ts (1)
VideosRepo(14-14)packages/web-backend/src/Videos/VideosRepo.ts (1)
VideosRepo(15-106)
apps/web/app/api/upload/[...route]/multipart.ts (5)
packages/web-domain/src/Authentication.ts (1)
CurrentUser(8-15)packages/web-backend/src/Videos/VideosRepo.ts (1)
VideosRepo(15-106)packages/web-backend/src/Database.ts (1)
Database(5-15)packages/database/schema.ts (1)
videos(285-340)apps/web/lib/server.ts (1)
runPromise(124-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
packages/web-backend/src/index.ts (1)
14-14: LGTM: Publicly exporting VideosRepoMakes the repo available to web layer; consistent with new owner/viewer flows.
apps/web/lib/server.ts (1)
17-17: LGTM: Add VideosRepo to global DependenciesCorrectly exposes VideosRepo.Default for server effects; complements new routes.
Also applies to: 103-104
apps/web/app/api/upload/[...route]/multipart.ts (1)
262-264: LGTM: Owner-scoped fetch via Videos.getByIdForOwnerCorrectly narrows access to video owners; aligns with PR’s auth fixes.
There was a problem hiding this comment.
Actionable comments posted: 1
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/api/upload/[...route]/multipart.ts (1)
75-84: Add PolicyDeniedError handling to return 403.The policy check on line 63 can fail with
PolicyDeniedErrorwhen the user is not the owner. This should be caught and mapped to a 403 Forbidden response instead of falling through to the generic 500 error.Apply this diff:
Effect.catchAll((e) => { if (e._tag === "VideoNotFoundError") return Effect.succeed<Response>(c.text("Video not found", 404)); + if (e._tag === "PolicyDeniedError") + return Effect.succeed<Response>(c.text("Forbidden", 403)); + // if (e._tag === "DatabaseError") // return Effect.succeed<Response>(c.text("Video not found", 404)); return Effect.succeed<Response>( c.json({ error: "Error initiating multipart upload" }, 500), ); }),
♻️ Duplicate comments (1)
apps/web/app/api/upload/[...route]/multipart.ts (1)
52-54: Return 400 on missing videoId instead of throwing.Throwing here escapes your Effect error mapping and yields a 500. Respond early with a 400.
Apply this diff:
- if (!videoIdRaw) throw new Error("Video ID is required"); + if (!videoIdRaw) return c.text("Video ID is required", 400);Note: The same issue exists in the
/completehandler at line 258.
🧹 Nitpick comments (1)
apps/web/app/api/upload/[...route]/multipart.ts (1)
79-80: Remove or document commented DatabaseError handling.This commented-out error handling maps
DatabaseErrorto "Video not found", which seems semantically incorrect. If this is unused, remove it. If it's a placeholder for future work, add a TODO comment explaining the intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/upload/[...route]/multipart.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/api/upload/[...route]/multipart.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/upload/[...route]/multipart.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/upload/[...route]/multipart.ts
apps/web/app/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/app/api/**/*.{ts,tsx}: Prefer Server Actions for API surface; when routes are necessary, implement under app/api and export only the handler from apiToHandler(ApiLive)
Construct API routes with @effect/platform HttpApi/HttpApiBuilder, declare contracts with Schema, and only export the handler
Use HttpAuthMiddleware for required auth and provideOptionalAuth for guests; avoid duplicating session lookups
Map domain errors to transport with HttpApiError.* and keep translation exhaustive (catchTags/tapErrorCause)
Inside HttpApiBuilder.group, acquire services with Effect.gen and provide dependencies via Layer.provide instead of manual provideService
Files:
apps/web/app/api/upload/[...route]/multipart.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 fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/api/upload/[...route]/multipart.ts
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/api/upload/[...route]/multipart.ts
🧠 Learnings (2)
📚 Learning: 2025-10-14T10:15:44.019Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T10:15:44.019Z
Learning: Applies to apps/web/app/**/*.{ts,tsx} : Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/web/app/api/* : On the server, always run effects through `EffectRuntime.runPromise` after `provideOptionalAuth` to ensure cookies and per-request context are attached.
Applied to files:
apps/web/app/api/upload/[...route]/multipart.ts
🧬 Code graph analysis (1)
apps/web/app/api/upload/[...route]/multipart.ts (6)
packages/web-backend/src/Videos/VideosRepo.ts (1)
VideosRepo(15-106)packages/web-domain/src/Policy.ts (2)
policy(29-41)Policy(8-12)packages/web-backend/src/Videos/VideosPolicy.ts (1)
VideosPolicy(9-96)packages/web-backend/src/Database.ts (1)
Database(5-15)packages/web-domain/src/Authentication.ts (1)
CurrentUser(8-15)apps/web/lib/server.ts (1)
runPromise(124-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/web/app/api/upload/[...route]/multipart.ts (2)
243-245: Effective migration to Effect-based handler.The restructure removes
asyncand returns an Effect directly, then properly injectsCurrentUsercontext viaprovideService. This aligns with the Effect-based architecture and enables proper context propagation.Also applies to: 482-482
260-262: Owner-scoped retrieval correctly applied.Using
videos.getByIdForOwnerdirectly encapsulates the authorization check, which is the key fix mentioned in the PR objectives. This is cleaner than the manual policy check pattern.
Another round of going through PostHog and fixing issues.
Changes:
Summary by CodeRabbit
New Features
Bug Fixes
Chores