security(video): move temp files out of public directory and require Supabase signed URLs#828
Open
Dasmat13 wants to merge 5 commits into
Open
security(video): move temp files out of public directory and require Supabase signed URLs#828Dasmat13 wants to merge 5 commits into
Dasmat13 wants to merge 5 commits into
Conversation
…Supabase signed URLs - Write audio and rendered videos to OS tmp dir instead of ./public/ - Require Supabase Storage and return signed URLs instead of public paths - Update cleanup job to remove temp assets from /tmp only - Remove public/videos fallback path from video pipeline
|
@Dasmat13 is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
gssoc'26
|
@coderabbitai review |
Contributor
WalkthroughVideo generation removes the baseUrl contract and public directory writes, moving temp audio/render output to OS temp storage with signed Supabase URLs surfaced via a new getVideoSignedUrl helper. The moderation action route adopts Zod validation with new tests; the doubts route gains explicit type annotations. ChangesVideo pipeline private storage migration
Moderation action request validation and tests
TypeScript-only fixes in doubts route
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VideoGenerateRoute
participant Inngest
participant RunVideoPipeline
participant TempDir
participant Storage
participant VideoStatusRoute
VideoGenerateRoute->>Inngest: emit video/generate.requested without baseUrl
Inngest->>RunVideoPipeline: runVideoPipeline({ content, imageUrl })
RunVideoPipeline->>TempDir: write temp audio and render output in os.tmpdir()
RunVideoPipeline->>Storage: uploadVideo(outputLocation, objectName)
Storage-->>RunVideoPipeline: uploaded objectName / videoUrl
RunVideoPipeline->>TempDir: delete local rendered file and temp audio
VideoStatusRoute->>Storage: getVideoSignedUrl(objectName)
Storage-->>VideoStatusRoute: signedUrl
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
codeant-ai
Bot
added
the
size:M
github-actions
Bot
removed
the
size:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const deletedFiles = await step.run("delete-old-files", async () => { | ||
| const tempDir = path.resolve("./public/temp-assets"); | ||
| const videosDir = path.resolve("./public/videos"); | ||
| const tempDir = path.join(os.tmpdir(), "doubtdesk-audio-"); |
There was a problem hiding this comment.
Suggestion: The cleanup target path is incorrect for how temp audio directories are created. mkdtempSync(path.join(os.tmpdir(), "doubtdesk-audio-")) creates directories like /tmp/doubtdesk-audio-xxxxxx, but this code checks only /tmp/doubtdesk-audio-, which usually does not exist, so old temp assets are never deleted. [logic error]
Severity Level: Major ⚠️
⚠️ Hourly temp cleanup never touches actual audio temp directories.
⚠️ Orphaned audio files persist after failed video renders.
⚠️ Disk usage grows over time from leaked audio files.Steps of Reproduction ✅
1. Start the Inngest worker so scheduled functions in `src/inngest/functions.ts` are
active; note the hourly cron for `cleanupTempAssets` at lines 38-40.
2. Trigger a video generation job by emitting the `video/generate.requested` event, which
invokes `generateVideo` at `src/inngest/functions.ts:73-129` and then `runVideoPipeline`
at `src/lib/video/pipeline.ts:95-251`.
3. Within `runVideoPipeline`, observe that temporary audio directory creation uses
`fs.mkdtempSync(path.join(os.tmpdir(), "doubtdesk-audio-"))` at
`src/lib/video/pipeline.ts:180`, producing a directory like `/tmp/doubtdesk-audio-abcdef`
(with a random suffix) that contains audio files.
4. Allow the hourly `cleanupTempAssets` job to run; its implementation at
`src/inngest/functions.ts:41-57` checks only the fixed path `path.join(os.tmpdir(),
"doubtdesk-audio-")`, which does not exist, so `fs.existsSync(tempDir)` is false and no
files under `/tmp/doubtdesk-audio-*` are ever scanned or deleted, leaving any orphaned
audio files from failed pipeline runs on disk.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/inngest/functions.ts
**Line:** 42:42
**Comment:**
*Logic Error: The cleanup target path is incorrect for how temp audio directories are created. `mkdtempSync(path.join(os.tmpdir(), "doubtdesk-audio-"))` creates directories like `/tmp/doubtdesk-audio-xxxxxx`, but this code checks only `/tmp/doubtdesk-audio-`, which usually does not exist, so old temp assets are never deleted.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| await onProgress({ progress: 65, step: "Generating audio…" }); | ||
| const tempDir = path.resolve("./public/temp-assets"); | ||
| if (!fs.existsSync(tempDir)) fs.mkdirSync(tempDir, { recursive: true }); | ||
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "doubtdesk-audio-")); |
There was a problem hiding this comment.
Suggestion: Each run creates a unique temp directory with mkdtempSync, but only individual audio files are deleted and the directory itself is never removed, causing unbounded empty-directory buildup in /tmp. Remove the temp directory explicitly after file cleanup (and in failure paths). [missing cleanup]
Severity Level: Major ⚠️
⚠️ Every job leaves orphaned audio temp directories in /tmp.
⚠️ Growing directory clutter complicates debugging filesystem state.
⚠️ Slightly increased inode usage on long-lived workers.Steps of Reproduction ✅
1. Trigger a video generation job via POST /api/video/generate
(src/app/api/video/generate/route.ts:22-84), which creates a video_jobs row and emits the
"video/generate.requested" event consumed by Inngest.
2. The generateVideo function in src/inngest/functions.ts:33-58 invokes runVideoPipeline,
and during step 4 (TTS), runVideoPipeline creates a unique tempDir using
fs.mkdtempSync(path.join(os.tmpdir(), "doubtdesk-audio-")) at
src/lib/video/pipeline.ts:180.
3. For each generated scene, runVideoPipeline writes audio-*.mp3 files inside tempDir
(lines 183-201) and, after rendering, deletes each audio file in the "Clean up temporary
audio files" block at src/lib/video/pipeline.ts:230-241; this cleanup only unlinks files
and never removes the tempDir directory itself.
4. After the pipeline completes, inspecting os.tmpdir() on the worker shows that the
doubtdesk-audio-* directory created at line 180 still exists but is now empty; repeating
successful jobs accumulates more orphaned temp directories under /tmp because
runVideoPipeline never deletes its tempDir.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/video/pipeline.ts
**Line:** 180:180
**Comment:**
*Missing Cleanup: Each run creates a unique temp directory with `mkdtempSync`, but only individual audio files are deleted and the directory itself is never removed, causing unbounded empty-directory buildup in `/tmp`. Remove the temp directory explicitly after file cleanup (and in failure paths).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+247
to
+248
| const videoUrl = await uploadVideo(outputLocation, objectName); | ||
| await fs.promises.unlink(outputLocation).catch(() => {}); |
There was a problem hiding this comment.
Suggestion: The local rendered MP4 is only deleted after uploadVideo succeeds; if upload/signing throws, cleanup is skipped and large files accumulate in /tmp. Move output-file deletion into a finally path so it runs on both success and failure. [resource leak]
Severity Level: Major ⚠️
❌ Failed uploads leak large MP4 files in /tmp.
⚠️ Serverless workers risk disk exhaustion under repeated failures.
⚠️ Manual cleanup required to reclaim worker storage space.Steps of Reproduction ✅
1. Start a video generation job by calling POST /api/video/generate (request handler at
src/app/api/video/generate/route.ts:22-84), which inserts a row into videoJobsTable and
emits the "video/generate.requested" event with jobId, content, and imageUrl.
2. The Inngest generateVideo function (src/inngest/functions.ts:33-58) handles this event
and calls runVideoPipeline({ content, imageUrl }, onProgress) defined in
src/lib/video/pipeline.ts:95-250, running OCR, script generation, TTS, and Remotion
render.
3. Inside runVideoPipeline, Remotion renders the MP4 to outputLocation under os.tmpdir()
at src/lib/video/pipeline.ts:209, then awaits uploadVideo(outputLocation, objectName) at
line 247; if Supabase is misconfigured or unavailable (e.g., NEXT_PUBLIC_SUPABASE_URL
missing or bucket error), uploadVideo throws inside src/lib/video/storage.ts:23-38 before
returning.
4. Because the exception occurs before fs.promises.unlink(outputLocation).catch(() => {})
at src/lib/video/pipeline.ts:248, the large MP4 file remains in /tmp on the serverless
worker while generateVideo marks the job as failed at src/inngest/functions.ts:75-80;
repeated upload failures leave multiple leaked video files on disk.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/video/pipeline.ts
**Line:** 247:248
**Comment:**
*Resource Leak: The local rendered MP4 is only deleted after `uploadVideo` succeeds; if upload/signing throws, cleanup is skipped and large files accumulate in `/tmp`. Move output-file deletion into a `finally` path so it runs on both success and failure.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const VIDEO_BUCKET = process.env.SUPABASE_VIDEO_BUCKET || "videos"; | ||
|
|
||
| // How long generated video signed URLs remain valid (1 hour). | ||
| const SIGNED_URL_EXPIRY_SECONDS = 60 * 60; |
There was a problem hiding this comment.
Suggestion: The function now returns a 1-hour signed URL and that URL is persisted in video_jobs, so later status reads will serve an expired link and completed videos become unusable after expiry. Store a stable object key in the job record and generate a fresh signed URL when clients request status/result instead of persisting a short-lived signed URL. [api mismatch]
Severity Level: Critical 🚨
❌ Completed videos become inaccessible after signed URL expiry.
❌ Status endpoint serves expired links, confusing client behavior.
⚠️ Users lose ability to revisit generated explanations later.Steps of Reproduction ✅
1. Start a video generation by calling POST /api/video/generate
(src/app/api/video/generate/route.ts:22-84), which inserts a video_jobs row and sends a
"video/generate.requested" event with jobId to Inngest.
2. The generateVideo function in src/inngest/functions.ts:33-75 calls runVideoPipeline;
when runVideoPipeline finishes, generateVideo updates videoJobsTable to set videoUrl and
videoType from result.videoUrl and result.videoType at lines 60-71, persisting the
returned videoUrl string in the database.
3. Inside runVideoPipeline (src/lib/video/pipeline.ts:243-250),
uploadVideo(outputLocation, objectName) is called; uploadVideo in
src/lib/video/storage.ts:19-49 uploads the MP4 and then creates a Supabase signed URL via
createSignedUrl(objectName, SIGNED_URL_EXPIRY_SECONDS) at line 42, where
SIGNED_URL_EXPIRY_SECONDS is defined as 60 * 60 (1 hour) at line 8, and returns this
short-lived signedUrl.
4. A client streams GET /api/video/status?jobId=… from
src/app/api/video/status/route.ts:31-151, receiving JobSnapshot events that include
videoUrl and videoType (lines 101-109); the client uses snapshot.videoUrl for playback. If
the user revisits the job after more than one hour, the SSE stream still reports status
"completed" and the same stored videoUrl from videoJobsTable, but Supabase rejects
requests to that expired signed URL, so the completed video can no longer be viewed even
though the job is marked successful.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/video/storage.ts
**Line:** 8:8
**Comment:**
*Api Mismatch: The function now returns a 1-hour signed URL and that URL is persisted in `video_jobs`, so later status reads will serve an expired link and completed videos become unusable after expiry. Store a stable object key in the job record and generate a fresh signed URL when clients request status/result instead of persisting a short-lived signed URL.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/lib/video/storage.ts (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture the
createSignedUrlerror for diagnosability.Only
datais destructured, so a failure surfaces as the generic "Failed to create signed URL" without the underlying cause. Destructureerrortoo and include its message.🪵 Include underlying error
- const { data } = await supabase.storage + const { data, error: signError } = await supabase.storage .from(VIDEO_BUCKET) .createSignedUrl(objectName, SIGNED_URL_EXPIRY_SECONDS); - if (!data?.signedUrl) { - throw new Error("Failed to create signed URL for uploaded video"); + if (signError || !data?.signedUrl) { + throw new Error( + `Failed to create signed URL for uploaded video${signError ? `: ${signError.message}` : ""}`, + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/video/storage.ts` around lines 41 - 47, The signed URL creation path in the storage helper only checks data from createSignedUrl, so failures lose the underlying cause; update the createSignedUrl call in the video storage logic to also destructure the returned error and include its message in the thrown error. Keep the fix localized to the signed URL generation flow in the storage module, and make sure the existing validation around signedUrl still guards the happy path.src/app/api/doubts/route.ts (1)
416-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the lookup typed as a select row.
existingClassroomTagscomes fromdb.select().from(tagsTable), so the map values are read rows, not insert payloads. The$inferInsertcast weakens the compiler check and makes the latersavedTags.pushcast unnecessary.♻️ Suggested fix
- const match = existingTagsMap.get(name) as typeof tagsTable.$inferInsert | undefined; - if (match) { - savedTags.push(match as typeof tagsTable.$inferSelect); + const match = existingTagsMap.get(name) as typeof tagsTable.$inferSelect | undefined; + if (match) { + savedTags.push(match);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/doubts/route.ts` around lines 416 - 418, The lookup in the tags reuse flow is typed too loosely by casting the map value to an insert shape, which then forces an unnecessary cast when pushing into savedTags. Update the existingTagsMap / match typing in the doubts route so the values are treated as select rows from tagsTable (the result of db.select().from(tagsTable)), and remove the $inferInsert cast from the lookup; this should let savedTags.push accept the row type directly without the extra $inferSelect cast.src/app/api/invites/[token]/join/route.ts (1)
102-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
@ts-ignore/anycast here.db.execute<{ membership_id: number }>(...)already types the returnedrows, so the escape hatch is unnecessary on this write path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/invites/`[token]/join/route.ts around lines 102 - 103, Remove the unnecessary TypeScript escape hatch in the join route: the `@ts-ignore` and `as any` around `db.execute` are no longer needed because `db.execute<{ membership_id: number }>(...)` already provides the correct row typing. Update the `join` handler in `route.ts` to call `db.execute` directly with the generic type and keep the `joinResult` usage typed through that inferred result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/inngest/functions.ts`:
- Around line 42-58: The cleanup logic in runVideoPipeline is scanning a fixed
temp directory name that does not match the actual artifacts created by
fs.mkdtempSync and the video render output, so it never removes stale files.
Update the cleanup block to iterate over os.tmpdir() and match both
doubtdesk-audio-* directories and video-*.mp4 files, then delete those matches
recursively for directories and directly for files. Use the existing
tempDir/tempDir pattern code in functions.ts to keep the cleanup aligned with
the actual locations created by the pipeline.
In `@src/lib/video/pipeline.ts`:
- Around line 244-250: The video render cleanup in pipeline.ts is only happening
after uploadVideo succeeds, so failed uploads leave outputLocation behind in
os.tmpdir() and also leave the mkdtemp-created tempDir directory behind. Update
the upload flow around uploadVideo in the pipeline return path to use
try/finally so local cleanup always runs, and make sure the finally block
removes both the rendered output file and its tempDir directory regardless of
whether uploadVideo throws.
In `@src/lib/video/storage.ts`:
- Around line 7-8: The signed video URL is being persisted and later returned
after it may have expired, so update the flow around `src/inngest/functions.ts`,
`src/app/api/video/status/route.ts`, and `src/lib/video/storage.ts` to avoid
storing the signed URL itself in `video_jobs.videoUrl`. Instead, persist the
storage object key (or another stable identifier) and generate a fresh signed
URL when reading the job status, or ensure the job record lifecycle is limited
to `SIGNED_URL_EXPIRY_SECONDS` and always references the current URL.
---
Nitpick comments:
In `@src/app/api/doubts/route.ts`:
- Around line 416-418: The lookup in the tags reuse flow is typed too loosely by
casting the map value to an insert shape, which then forces an unnecessary cast
when pushing into savedTags. Update the existingTagsMap / match typing in the
doubts route so the values are treated as select rows from tagsTable (the result
of db.select().from(tagsTable)), and remove the $inferInsert cast from the
lookup; this should let savedTags.push accept the row type directly without the
extra $inferSelect cast.
In `@src/app/api/invites/`[token]/join/route.ts:
- Around line 102-103: Remove the unnecessary TypeScript escape hatch in the
join route: the `@ts-ignore` and `as any` around `db.execute` are no longer
needed because `db.execute<{ membership_id: number }>(...)` already provides the
correct row typing. Update the `join` handler in `route.ts` to call `db.execute`
directly with the generic type and keep the `joinResult` usage typed through
that inferred result.
In `@src/lib/video/storage.ts`:
- Around line 41-47: The signed URL creation path in the storage helper only
checks data from createSignedUrl, so failures lose the underlying cause; update
the createSignedUrl call in the video storage logic to also destructure the
returned error and include its message in the thrown error. Keep the fix
localized to the signed URL generation flow in the storage module, and make sure
the existing validation around signedUrl still guards the happy path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d91a95e-1d8a-4d8e-9006-a5f682b96f4d
📒 Files selected for processing (7)
src/__tests__/configs/db.test.tssrc/app/api/doubts/route.tssrc/app/api/invites/[token]/join/route.tssrc/app/api/video/generate/route.tssrc/inngest/functions.tssrc/lib/video/pipeline.tssrc/lib/video/storage.ts
💤 Files with no reviewable changes (1)
- src/app/api/video/generate/route.ts
…l, and signed URL expiry - Fix cleanupTempAssets to scan /tmp/doubtdesk-audio-* directories - Remove empty temp directories after cleanup - Wrap rendered output deletion in finally block - Store object key in DB; generate fresh signed URL on status read
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/api/video/status/route.ts (1)
112-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog signing failures instead of silently swallowing them.
The catch block discards all errors with no logging. If Supabase Storage is misconfigured or unavailable, completed videos will silently serve broken links with no trace for operators. A
console.erroris trivial to add and critical for production observability.♻️ Proposed refactor
try { snapshot.videoUrl = await getVideoSignedUrl(snapshot.videoUrl); } catch { - // keep stored object key; client will see a broken link rather than no status + console.error("Failed to sign video URL:", snapshot.videoUrl, err); + // keep stored object key; client will see a broken link rather than no status }Note: the catch parameter needs to be captured:
- } catch { + } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/video/status/route.ts` around lines 112 - 118, The signing failure in the completed-video branch is being swallowed in the route handler, so add error logging in the catch block instead of ignoring it. Update the try/catch around getVideoSignedUrl in the status route to capture the thrown error and log it with console.error (or the existing logger) while preserving the current fallback behavior of keeping snapshot.videoUrl unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/video/storage.ts`:
- Around line 52-57: The signed URL creation path in createSignedVideoUrl is
dropping the Supabase failure details by only checking data?.signedUrl. Update
the createSignedUrl call to also read the returned error and, when signing
fails, throw an exception that includes error?.message alongside the existing
context so bucket/object/permission issues are actionable.
---
Nitpick comments:
In `@src/app/api/video/status/route.ts`:
- Around line 112-118: The signing failure in the completed-video branch is
being swallowed in the route handler, so add error logging in the catch block
instead of ignoring it. Update the try/catch around getVideoSignedUrl in the
status route to capture the thrown error and log it with console.error (or the
existing logger) while preserving the current fallback behavior of keeping
snapshot.videoUrl unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5f69579-4ffd-4b0b-beb9-357fa69238e4
📒 Files selected for processing (4)
src/app/api/video/status/route.tssrc/inngest/functions.tssrc/lib/video/pipeline.tssrc/lib/video/storage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/inngest/functions.ts
- src/lib/video/pipeline.ts
github-actions
Bot
added
size/l
and removed
merge-conflict
Dasmat13
force-pushed
the
fix/strike-reset-on-block-expiry
branch
from
5b25b64 to
92b9f8e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/api/admin/moderation/action/route.ts (1)
127-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 127 is now unreachable dead code.
With the Zod enum validation at line 15 (
z.enum(["dismiss", "warn", "block"])),actionis guaranteed to be one of the three values before reaching the control flow. All three are handled by theifblocks above, making this fallback impossible to reach.♻️ Optional: remove unreachable fallback
return NextResponse.json({ success: true, message: "User blocked successfully" }); } - - return NextResponse.json({ error: "Invalid action" }, { status: 400 }); } catch (error: unknown) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/admin/moderation/action/route.ts` at line 127, The fallback in the moderation action route is unreachable because `action` is already constrained by the Zod enum in the `route.ts` handler and all valid cases are covered by the existing `if` branches. Remove the dead `NextResponse.json({ error: "Invalid action" }, { status: 400 })` path from the `action` handling logic so the control flow only contains the reachable `dismiss`, `warn`, and `block` cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/api/admin-moderation-action.test.ts`:
- Around line 76-105: The current POST test only covers Zod validation failures,
but it misses the malformed JSON path in parseAndValidateRequest. Add a separate
test case in the admin moderation action suite that sends invalid JSON to POST
and asserts the 400 response with success false and error "Invalid JSON format",
alongside the existing validation cases. Use the existing POST helper and
parseAndValidateRequest flow as the reference points so the new case targets the
distinct JSON parsing branch.
---
Nitpick comments:
In `@src/app/api/admin/moderation/action/route.ts`:
- Line 127: The fallback in the moderation action route is unreachable because
`action` is already constrained by the Zod enum in the `route.ts` handler and
all valid cases are covered by the existing `if` branches. Remove the dead
`NextResponse.json({ error: "Invalid action" }, { status: 400 })` path from the
`action` handling logic so the control flow only contains the reachable
`dismiss`, `warn`, and `block` cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0da95e46-be60-4aa0-af90-bce7baf29c8a
📒 Files selected for processing (8)
src/__tests__/api/admin-moderation-action.test.tssrc/__tests__/video/pipeline-cleanup.test.tssrc/app/api/admin/moderation/action/route.tssrc/app/api/doubts/route.tssrc/app/api/video/status/route.tssrc/inngest/functions.tssrc/lib/video/pipeline.tssrc/lib/video/storage.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/api/doubts/route.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/api/video/status/route.ts
- src/lib/video/storage.ts
- src/inngest/functions.ts
Comment on lines
+76
to
+105
| it("returns 400 when body parameters violate the Zod schema", async () => { | ||
| requireAdminMock.mockResolvedValue({}); | ||
| currentUserMock.mockResolvedValue({ primaryEmailAddress: { emailAddress: "admin@example.com" } }); | ||
|
|
||
| const testCases = [ | ||
| // Missing all fields | ||
| {}, | ||
| // Invalid email | ||
| { logId: 1, userEmail: "invalid-email", action: "dismiss" }, | ||
| // Negative logId | ||
| { logId: -1, userEmail: "test@example.com", action: "dismiss" }, | ||
| // Float logId | ||
| { logId: 1.5, userEmail: "test@example.com", action: "dismiss" }, | ||
| // Invalid action enum | ||
| { logId: 1, userEmail: "test@example.com", action: "invalid-action" }, | ||
| ]; | ||
|
|
||
| for (const testCase of testCases) { | ||
| const req = new NextRequest("http://localhost/api/admin/moderation/action", { | ||
| method: "POST", | ||
| body: JSON.stringify(testCase), | ||
| }); | ||
|
|
||
| const res = await POST(req); | ||
| expect(res.status).toBe(400); | ||
| const json = await res.json(); | ||
| expect(json.success).toBe(false); | ||
| expect(json.error).toBe("Validation failed"); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing test case for invalid JSON body.
parseAndValidateRequest has a distinct code path for malformed JSON that returns { success: false, error: "Invalid JSON format" } with status 400 — separate from the Zod validation failure path (error: "Validation failed"). The current test loop only covers Zod schema violations. Adding an invalid-JSON case would close this coverage gap.
💚 Suggested addition to testCases
const testCases = [
// Missing all fields
{},
// Invalid email
{ logId: 1, userEmail: "invalid-email", action: "dismiss" },
// Negative logId
{ logId: -1, userEmail: "test@example.com", action: "dismiss" },
// Float logId
{ logId: 1.5, userEmail: "test@example.com", action: "dismiss" },
// Invalid action enum
{ logId: 1, userEmail: "test@example.com", action: "invalid-action" },
];
for (const testCase of testCases) {
const req = new NextRequest("http://localhost/api/admin/moderation/action", {
method: "POST",
body: JSON.stringify(testCase),
});
const res = await POST(req);
expect(res.status).toBe(400);
const json = await res.json();
expect(json.success).toBe(false);
expect(json.error).toBe("Validation failed");
}
+
+ // Invalid JSON body
+ const invalidJsonReq = new NextRequest("http://localhost/api/admin/moderation/action", {
+ method: "POST",
+ body: "not valid json{",
+ });
+ const invalidJsonRes = await POST(invalidJsonReq);
+ expect(invalidJsonRes.status).toBe(400);
+ const invalidJsonBody = await invalidJsonRes.json();
+ expect(invalidJsonBody.success).toBe(false);
+ expect(invalidJsonBody.error).toBe("Invalid JSON format");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("returns 400 when body parameters violate the Zod schema", async () => { | |
| requireAdminMock.mockResolvedValue({}); | |
| currentUserMock.mockResolvedValue({ primaryEmailAddress: { emailAddress: "admin@example.com" } }); | |
| const testCases = [ | |
| // Missing all fields | |
| {}, | |
| // Invalid email | |
| { logId: 1, userEmail: "invalid-email", action: "dismiss" }, | |
| // Negative logId | |
| { logId: -1, userEmail: "test@example.com", action: "dismiss" }, | |
| // Float logId | |
| { logId: 1.5, userEmail: "test@example.com", action: "dismiss" }, | |
| // Invalid action enum | |
| { logId: 1, userEmail: "test@example.com", action: "invalid-action" }, | |
| ]; | |
| for (const testCase of testCases) { | |
| const req = new NextRequest("http://localhost/api/admin/moderation/action", { | |
| method: "POST", | |
| body: JSON.stringify(testCase), | |
| }); | |
| const res = await POST(req); | |
| expect(res.status).toBe(400); | |
| const json = await res.json(); | |
| expect(json.success).toBe(false); | |
| expect(json.error).toBe("Validation failed"); | |
| } | |
| }); | |
| it("returns 400 when body parameters violate the Zod schema", async () => { | |
| requireAdminMock.mockResolvedValue({}); | |
| currentUserMock.mockResolvedValue({ primaryEmailAddress: { emailAddress: "admin@example.com" } }); | |
| const testCases = [ | |
| // Missing all fields | |
| {}, | |
| // Invalid email | |
| { logId: 1, userEmail: "invalid-email", action: "dismiss" }, | |
| // Negative logId | |
| { logId: -1, userEmail: "test@example.com", action: "dismiss" }, | |
| // Float logId | |
| { logId: 1.5, userEmail: "test@example.com", action: "dismiss" }, | |
| // Invalid action enum | |
| { logId: 1, userEmail: "test@example.com", action: "invalid-action" }, | |
| ]; | |
| for (const testCase of testCases) { | |
| const req = new NextRequest("http://localhost/api/admin/moderation/action", { | |
| method: "POST", | |
| body: JSON.stringify(testCase), | |
| }); | |
| const res = await POST(req); | |
| expect(res.status).toBe(400); | |
| const json = await res.json(); | |
| expect(json.success).toBe(false); | |
| expect(json.error).toBe("Validation failed"); | |
| } | |
| // Invalid JSON body | |
| const invalidJsonReq = new NextRequest("http://localhost/api/admin/moderation/action", { | |
| method: "POST", | |
| body: "not valid json{", | |
| }); | |
| const invalidJsonRes = await POST(invalidJsonReq); | |
| expect(invalidJsonRes.status).toBe(400); | |
| const invalidJsonBody = await invalidJsonRes.json(); | |
| expect(invalidJsonBody.success).toBe(false); | |
| expect(invalidJsonBody.error).toBe("Invalid JSON format"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/api/admin-moderation-action.test.ts` around lines 76 - 105, The
current POST test only covers Zod validation failures, but it misses the
malformed JSON path in parseAndValidateRequest. Add a separate test case in the
admin moderation action suite that sends invalid JSON to POST and asserts the
400 response with success false and error "Invalid JSON format", alongside the
existing validation cases. Use the existing POST helper and
parseAndValidateRequest flow as the reference points so the new case targets the
distinct JSON parsing branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Fixes #802
Summary
./public/into OS tmp directory/tmponlySecurity Impact
Prevents path traversal risk and public exposure of generated media files.
CodeAnt-AI Description
Keep generated videos private and serve them through signed links
What Changed
Impact
✅ No public access to generated videos✅ Fewer broken video links in production✅ Safer handling of temporary media files💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes