fix: add videos from caps #1201
Conversation
WalkthroughAdds conditional insertion logic in apps/web/actions/folders/add-videos.ts to route new video records to either sharedVideos or spaceVideos. Uses nanoId for IDs. The path is chosen based on isAllSpacesEntry and presence of folder.spaceId. Subsequent updates to the respective tables remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as add-videos action
participant DB as Database
rect rgba(230,240,255,0.4)
note over A: Determine destination table
U->>A: addVideos(folderId, videoId[], isAllSpacesEntry)
A->>A: Check isAllSpacesEntry && folder.spaceId
alt isAllSpacesEntry && spaceId
A->>DB: INSERT into sharedVideos (id=nanoId, videoId, folderId, organizationId, sharedByUserId)
DB-->>A: Insert OK
A->>DB: Follow-up updates on sharedVideos
else
A->>DB: INSERT into spaceVideos (id=nanoId, videoId, folderId, spaceId, addedById)
DB-->>A: Insert OK
A->>DB: Follow-up updates on spaceVideos
end
A-->>U: Result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/actions/folders/add-videos.ts (1)
54-107: Wrap related database operations in a transaction.The insert (lines 54-85) and update (lines 87-107) are tightly coupled but not atomic. If one operation fails, data may be left inconsistent (e.g., new associations inserted without updating existing ones, or vice versa). Drizzle supports transactions; wrap both operations for atomicity.
Example refactor:
await db().transaction(async (tx) => { // Insert new associations if (isAllSpacesEntry && folder.spaceId) { await tx.insert(sharedVideos).values(/* ... */); } else { await tx.insert(spaceVideos).values(/* ... */); } // Update existing associations if (isAllSpacesEntry && folder.spaceId) { await tx.update(sharedVideos).set({ folderId }).where(/* ... */); } else { await tx.update(spaceVideos).set({ folderId }).where(/* ... */); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/actions/folders/add-videos.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/actions/folders/add-videos.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/actions/folders/add-videos.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/actions/folders/add-videos.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/folders/add-videos.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/actions/folders/add-videos.ts
🧬 Code graph analysis (1)
apps/web/actions/folders/add-videos.ts (3)
packages/database/index.ts (1)
db(18-25)packages/database/schema.ts (2)
sharedVideos(334-362)spaceVideos(631-651)packages/database/helpers.ts (1)
nanoId(6-9)
⏰ 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/actions/folders/add-videos.ts (1)
5-5: LGTM!The import is correctly added and properly used for generating unique IDs in the insertion logic.
| //if we're adding videos to a folder from Caps page, then insert the videos into the folder | ||
| if (isAllSpacesEntry && folder.spaceId) { | ||
| await db() | ||
| .insert(sharedVideos) | ||
| .values( | ||
| validVideoIds.map((videoId) => { | ||
| const id = nanoId(); | ||
| return { | ||
| id, | ||
| videoId, | ||
| folderId, | ||
| organizationId: user.activeOrganizationId, | ||
| sharedByUserId: user.id, | ||
| }; | ||
| }), | ||
| ); | ||
| } else { | ||
| await db() | ||
| .insert(spaceVideos) | ||
| .values( | ||
| validVideoIds.map((videoId) => { | ||
| const id = nanoId(); | ||
| return { | ||
| id, | ||
| videoId, | ||
| folderId, | ||
| spaceId, | ||
| addedById: user.id, | ||
| }; | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Duplicate records: Insert followed by update on same videos.
The new insertion logic creates records for validVideoIds, but lines 87-107 immediately update existing records using those same validVideoIds. This will produce duplicate associations if any video already has a folder assignment:
- Insert (lines 54-85): Creates new
videoId → folderIdrecords - Update (lines 87-107): Modifies existing
videoId → folderIdrecords
Neither sharedVideos nor spaceVideos schema enforces a unique constraint on (videoId, folderId) or (videoId, organizationId/spaceId), so duplicates are allowed. Consider using an upsert pattern (e.g., .onConflictDoUpdate() if a unique constraint exists, or inserting only videos that have no prior folder association).
Run this script to check for unique constraints and potential duplicate risks:
🏁 Script executed:
#!/bin/bash
# Verify schema constraints and search for existing duplicate-prevention logic
# Check schema definitions for unique constraints
rg -nP -A5 'export const (sharedVideos|spaceVideos) = mysqlTable' packages/database/schema.ts
# Search for upsert or duplicate-handling patterns in similar code
rg -nP --type=ts -C3 '\.(insert|upsert)\((sharedVideos|spaceVideos)\)' apps/web/actions/
# Look for existing duplicate checks
rg -nP --type=ts -C3 'onConflict|onDuplicate|IGNORE|REPLACE' apps/web/actions/Length of output: 3821
🏁 Script executed:
#!/bin/bash
# Show validVideoIds usage
rg -n -C5 'validVideoIds' apps/web/actions/folders/add-videos.ts
# Show any update queries
rg -n -C5 '\.update' apps/web/actions/folders/add-videos.tsLength of output: 2416
Prevent duplicate folder associations in folders/add-videos.ts
The code always inserts new sharedVideos/spaceVideos records for validVideoIds before updating, but without a unique constraint on (videoId, folderId) this creates duplicate entries. Replace the insert-then-update pattern with an upsert (.onConflictDoUpdate()) or guard the insert to only add videos not already assigned to the folder.
|
|
||
| //if video already exists in the space, then move it | ||
| //if we're adding videos to a folder from Caps page, then insert the videos into the folder | ||
| if (isAllSpacesEntry && folder.spaceId) { |
There was a problem hiding this comment.
Condition mismatch between insert and update blocks.
The insert block (line 55) uses isAllSpacesEntry && folder.spaceId, while the update block (line 87) only checks isAllSpacesEntry. This creates inconsistent routing:
- Scenario:
isAllSpacesEntry = true,folder.spaceId = null- Insert →
spaceVideos(else branch) - Update →
sharedVideos - Result: Insert and update target different tables
- Insert →
Ensure both blocks use the same condition to maintain consistency. If folder.spaceId is required for sharedVideos, include it in the update condition as well.
Apply this diff to align the conditions:
- if (isAllSpacesEntry) {
+ if (isAllSpacesEntry && folder.spaceId) {
await db()
.update(sharedVideos)Also applies to: 87-87
🤖 Prompt for AI Agents
In apps/web/actions/folders/add-videos.ts around lines 55 and 87, the insert
uses "isAllSpacesEntry && folder.spaceId" while the update only checks
"isAllSpacesEntry", causing inserts and updates to route to different tables
when folder.spaceId is null; update the condition at line 87 to match the insert
(change it to "isAllSpacesEntry && folder.spaceId") so both insert and update
branches use the same routing logic, or alternatively unify both conditions to a
single shared boolean variable and use that in both places if preferred.
Summary by CodeRabbit
New Features
Bug Fixes