feat: refactoring video generation into async job#731
Merged
Conversation
|
CodeAnt AI is reviewing your PR. |
|
@madsysharma 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. |
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
WalkthroughRefactors video generation into an asynchronous job flow with queued persistence, background execution, SSE progress streaming, and a client hook for tracking job state. ChangesAsync video generation pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GenerateRoute as /api/video/generate
participant Redis
participant DB as video_jobs
participant Inngest
participant GenerateVideo as generateVideo
participant StatusRoute as /api/video/status
participant Hook as useVideoGeneration
Client->>GenerateRoute: POST content/imageUrl
GenerateRoute->>Redis: set lockKey nx ex
GenerateRoute->>DB: insert queued job
GenerateRoute->>Inngest: send video/generate.requested
GenerateRoute-->>Client: 202 jobId
Hook->>StatusRoute: EventSource(jobId)
Inngest->>GenerateVideo: event payload
GenerateVideo->>DB: update progress/status
StatusRoute->>DB: poll job row
StatusRoute-->>Hook: SSE progress updates
GenerateVideo->>DB: update completed/failed
Possibly related PRs
Suggested labels: 🚥 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 |
Contributor
Author
|
Hi @knoxiboy , please review this PR. Thank you. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
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: 13
🧹 Nitpick comments (1)
src/app/api/video/status/route.ts (1)
129-139: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a heartbeat to keep idle SSE connections alive.
Because of the change-detection at Line 111, no bytes are written while the job sits in the same state. If a stage (e.g. Remotion render) stays unchanged for tens of seconds, intermediaries (proxies/load balancers) may drop the idle connection before
MAX_STREAM_MS. Emitting a periodic comment ping (e.g.: ping\n\n) prevents idle timeouts and lets clients detect dead connections.♻️ Example heartbeat
interval = setInterval(() => { poll().catch(() => { send({ status: "failed", error: "Status stream error" }); cleanup(); }); }, POLL_INTERVAL_MS); + + // Keep the connection alive through proxies when state is unchanged. + const heartbeat = setInterval(() => { + if (!closed) controller.enqueue(encoder.encode(`: ping\n\n`)); + }, 15000);Remember to clear
heartbeatincleanup().🤖 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 129 - 139, The SSE stream in the status route can go idle between meaningful updates, so add a periodic heartbeat to keep the connection alive. In the polling flow inside the route handler, introduce a heartbeat timer that sends a comment ping like a lightweight SSE keepalive whenever no status change has been emitted, and make sure it is cleared alongside the existing timers in cleanup(). Use the existing polling/send/cleanup logic in the route to wire this in without changing the status update behavior.
🤖 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 `@drizzle/meta/_journal.json`:
- Line 21: Add the missing drizzle/meta/0016_snapshot.json file so the migration
referenced by _journal.json for 0016_video_jobs has a matching snapshot and the
migration chain stays complete. Use the existing drizzle meta snapshot format
from nearby numbered snapshots as the template, and ensure the snapshot content
aligns with the 0016_video_jobs migration entry.
In `@src/app/api/video/generate/route.ts`:
- Around line 62-64: The baseUrl construction in the video generation route is
deriving the worker origin from attacker-controlled request headers, so replace
the protocol/host logic with a configured, allowlisted application URL instead.
Update the code in the generate route around the baseUrl setup to read from the
existing app/site URL configuration source used elsewhere in the app, and pass
that trusted origin into the background pipeline instead of req.headers values.
- Around line 91-95: The enqueue error handling in the video generation route
currently returns error.message to clients, which can leak internal DB, queue,
or config details. Keep the detailed error only in the console.error logging,
and change the NextResponse.json payload in the generate route to always return
a stable generic enqueue failure message instead of using the thrown Error
message. Use the existing error handling block around the enqueue logic in the
route handler to make this change.
- Around line 36-37: The block check in the video generate route ignores lookup
failures because it only returns when isBlocked is true. Update the logic around
checkUserBlock in the route handler to return blockResponse whenever
errorResponse is present, and only continue enqueueing after a successful block
check with no errorResponse. Use the existing checkUserBlock destructuring and
the route handler flow to locate the fix.
- Around line 42-57: The lock acquisition in the video generation route is
non-atomic because `setnx` and `expire` are separate steps, which can leave
`video_lock:${user.id}` stuck if the TTL write fails. Update the lock logic in
`route.ts` to use a single atomic Redis `SET` with `NX` and `EX 300` in the same
call, and keep the existing 429 early return in the same `generate` flow.
In `@src/app/api/video/status/route.ts`:
- Around line 126-134: The polling flow in start() is treating transient DB
errors as terminal stream failures, which can both reject the initial await
poll() and incorrectly send { status: "failed" } from the setInterval poll loop.
Update the error handling around poll() in src/app/api/video/status/route.ts so
the initial emission is guarded and retryable, and make repeated polling
failures non-terminal until a small threshold is reached. Keep the stream alive
on transient errors by logging or sending a non-terminal error frame instead of
cleanup() and failed, so use-video-generation.ts does not stop on a temporary
blip.
In `@src/hooks/use-video-generation.ts`:
- Around line 82-86: The useVideoGeneration hook currently only relies on
EventSource auto-reconnect and has no fatal error path, so queued/processing can
get stuck. Update the EventSource setup in useVideoGeneration to add an onerror
handler that detects non-recoverable failures from the /api/video/status stream
(such as 401/404/500), clears isGenerating, and moves the video generation state
out of queued/processing. Keep the existing closeStream cleanup flow and use the
hook’s existing state setters/helpers to finalize the failure state
consistently.
In `@src/inngest/functions.ts`:
- Around line 401-412: The stale-job cleanup in the video job timeout flow is
using createdAt instead of the last activity timestamp, which can fail active
jobs that recently reported progress. Update the cleanup query in the function
that builds the timed-out failure update so it checks videoJobsTable.updatedAt
against the cutoff, while keeping the existing status filter and failure update
behavior in place.
- Around line 387-390: The lock release in the Inngest job cleanup is deleting
the Redis key without verifying ownership, which can remove a newer user lock.
Update the generation flow in `route.ts` and the cleanup in `functions.ts` to
write and carry a per-job token instead of a constant value. In `video/generate`
and the corresponding finally block that uses `lockKey`, compare the stored
token before deleting, and only call `redisClient.del` when the token still
matches the current job’s ownership.
In `@src/lib/video/pipeline.ts`:
- Around line 132-134: Normalize the classifier output in pipeline.ts before
using videoType, since the current JSON.parse result is trusted directly and can
branch into unsupported script shapes; use the existing classification parsing
logic around videoType to trim/case-normalize and validate against the allowed
types before assigning. Also guard the later rawScenes handling so it is
verified as an array before any .map() usage, and fall back or reject invalid
LLM output in the same flow to keep the rest of the pipeline safe.
- Around line 202-239: The video rendering flow in pipeline.ts currently returns
a local /videos URL from the public/videos directory, which is not durable in
serverless/background execution. Update the render path around
renderMedia/selectComposition so the MP4 is uploaded to durable object storage
after rendering, then return and persist that stored URL instead of
path.basename(outputLocation). Use the existing pipeline identifiers like
outputLocation, bundleLocation, renderMedia, and the final return { videoUrl,
videoType } to locate and replace the local-filesystem assumption.
- Around line 201-208: The Remotion bundling setup in the video pipeline uses
the wrong entrypoint path. Update the entryPoint in the pipeline function to
point at the actual remotion index file under src/lib/video/remotion/index.tsx
so `@remotion/bundler` resolves the correct source before calling bundle().
- Around line 186-189: The Google TTS fetches in the audio generation flow can
hang indefinitely, which can leave the job stuck in processing. Update the axios
request inside the Promise.all map in the video pipeline code to use a
reasonable timeout for each URL fetch, and make sure the timeout is applied
wherever those audioBuffers are loaded so stalled requests fail fast and the
pipeline can handle the error path cleanly.
---
Nitpick comments:
In `@src/app/api/video/status/route.ts`:
- Around line 129-139: The SSE stream in the status route can go idle between
meaningful updates, so add a periodic heartbeat to keep the connection alive. In
the polling flow inside the route handler, introduce a heartbeat timer that
sends a comment ping like a lightweight SSE keepalive whenever no status change
has been emitted, and make sure it is cleared alongside the existing timers in
cleanup(). Use the existing polling/send/cleanup logic in the route to wire this
in without changing the status update behavior.
🪄 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: 26822b48-7653-4728-b348-71c52305d812
📒 Files selected for processing (9)
drizzle/0016_video_jobs.sqldrizzle/meta/_journal.jsonsrc/app/api/inngest/route.tssrc/app/api/video/generate/route.tssrc/app/api/video/status/route.tssrc/configs/schema.tssrc/hooks/use-video-generation.tssrc/inngest/functions.tssrc/lib/video/pipeline.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/configs/schema.ts (1)
34-52: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winKeep
classroomsTable.organizationIduntil all consumers are updatedThis removes
classroomsTable.organizationIdand its FK/index, butsrc/app/api/analytics/route.tsandsrc/app/api/rooms/route.tsstill query that column. This breaks type-checking/build for the current tree. Update those routes in the same change set, or keep the column until the org-scoped flow is migrated.🤖 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/configs/schema.ts` around lines 34 - 52, The schema change in classroomsTable removed organizationId, but current consumers still depend on it. Either restore organizationId in the classroomsTable definition with its FK/index, or update the remaining callers in route handlers like analytics route and rooms route to stop querying it before removing it from the schema. Use classroomsTable as the anchor for the change and keep the schema aligned with all active query paths.
🤖 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 @.env.example:
- Around line 24-28: Reorder the example env entries so APP_URL appears before
NEXT_PUBLIC_APP_URL in the .env.example block; keep the existing comments with
APP_URL and move the public URL entry below it to satisfy dotenv-linter. This is
only an ordering change, so update the two variable declarations around APP_URL
and NEXT_PUBLIC_APP_URL without changing their values or comments.
In `@src/configs/schema.ts`:
- Around line 522-526: Add a database-level constraint for videoJobsTable.status
because it is currently just a varchar and can accept invalid states. Update the
schema in schema.ts near the videoJobsTable definition to use a pgEnum for the
allowed lifecycle values or add a check constraint, and make sure the status
field only permits the queued/processing/completed/failed states used by the job
flow.
In `@src/lib/ratelimit.ts`:
- Around line 127-136: The nx handling in set ignores expiration, so expired
entries in memoryMap still block new writes forever. Update the mock lock logic
in set to check the stored reset timestamp before treating a key as present, and
allow nx writes once the previous entry has expired; keep the behavior aligned
with the TTL passed via opts?.ex so the lock self-releases correctly.
In `@src/lib/video/storage.ts`:
- Around line 43-44: The current video URL handling in storage.ts persists the
result of getPublicUrl() from VIDEO_BUCKET, which only works for a public bucket
and will be unusable if the bucket is private. Update the
uploadVideo/getPublicUrl flow so the bucket is guaranteed public or, if privacy
must remain, stop storing the upload-time public URL and instead generate a
signed URL at playback time using the relevant storage helper.
---
Outside diff comments:
In `@src/configs/schema.ts`:
- Around line 34-52: The schema change in classroomsTable removed
organizationId, but current consumers still depend on it. Either restore
organizationId in the classroomsTable definition with its FK/index, or update
the remaining callers in route handlers like analytics route and rooms route to
stop querying it before removing it from the schema. Use classroomsTable as the
anchor for the change and keep the schema aligned with all active query paths.
🪄 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: f9fd0eda-a765-4a37-9696-da9b98156fad
📒 Files selected for processing (11)
.env.exampledrizzle/0016_video_jobs.sqlsrc/app/api/inngest/route.tssrc/app/api/video/generate/route.tssrc/app/api/video/status/route.tssrc/configs/schema.tssrc/hooks/use-video-generation.tssrc/inngest/functions.tssrc/lib/ratelimit.tssrc/lib/video/pipeline.tssrc/lib/video/storage.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- drizzle/0016_video_jobs.sql
- src/app/api/inngest/route.ts
- src/app/api/video/status/route.ts
- src/inngest/functions.ts
- src/app/api/video/generate/route.ts
- src/hooks/use-video-generation.ts
Comment on lines
24
to
+28
| NEXT_PUBLIC_APP_URL=http://localhost:3000 | ||
| # Server-side origin used by the background video pipeline to reference generated | ||
| # assets. Must be a configured/allowlisted URL (never derived from request headers). | ||
| # Falls back to NEXT_PUBLIC_APP_URL if unset. | ||
| APP_URL=http://localhost:3000 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reorder APP_URL above NEXT_PUBLIC_APP_URL.
dotenv-linter flags this ordering, so swapping the keys will keep the example file lint-clean.
Proposed fix
+APP_URL=http://localhost:3000
NEXT_PUBLIC_APP_URL=http://localhost:3000
# Server-side origin used by the background video pipeline to reference generated
# assets. Must be a configured/allowlisted URL (never derived from request headers).
# Falls back to NEXT_PUBLIC_APP_URL if unset.
-APP_URL=http://localhost:3000📝 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
| NEXT_PUBLIC_APP_URL=http://localhost:3000 | |
| # Server-side origin used by the background video pipeline to reference generated | |
| # assets. Must be a configured/allowlisted URL (never derived from request headers). | |
| # Falls back to NEXT_PUBLIC_APP_URL if unset. | |
| APP_URL=http://localhost:3000 | |
| APP_URL=http://localhost:3000 | |
| NEXT_PUBLIC_APP_URL=http://localhost:3000 | |
| # Server-side origin used by the background video pipeline to reference generated | |
| # assets. Must be a configured/allowlisted URL (never derived from request headers). | |
| # Falls back to NEXT_PUBLIC_APP_URL if unset. |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 24-24: [UnorderedKey] The NEXT_PUBLIC_APP_URL key should go before the NEXT_PUBLIC_SITE_URL key
(UnorderedKey)
[warning] 28-28: [UnorderedKey] The APP_URL key should go before the NEXT_PUBLIC_APP_URL key
(UnorderedKey)
🤖 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 @.env.example around lines 24 - 28, Reorder the example env entries so
APP_URL appears before NEXT_PUBLIC_APP_URL in the .env.example block; keep the
existing comments with APP_URL and move the public URL entry below it to satisfy
dotenv-linter. This is only an ordering change, so update the two variable
declarations around APP_URL and NEXT_PUBLIC_APP_URL without changing their
values or comments.
Source: Linters/SAST tools
Comment on lines
+522
to
+526
| })); | ||
|
|
||
| // Async video generation jobs (issue #321). Tracks the OCR → AI script → TTS → | ||
| // Remotion render pipeline as a background Inngest job so the request handler | ||
| // returns immediately instead of blocking 30-60s past the serverless timeout. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'export const videoJobsTable = $$$' --lang typescript src/configs/schema.tsRepository: knoxiboy/DoubtDesk
Length of output: 1648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== schema imports and table definition ==\n'
sed -n '1,80p' src/configs/schema.ts
printf '\n== video_jobs table block ==\n'
sed -n '527,546p' src/configs/schema.ts
printf '\n== search for video_jobs constraints/usages ==\n'
rg -n "video_jobs|videoType|status" src -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'Repository: knoxiboy/DoubtDesk
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== migration files mentioning video_jobs/status/video_type ==\n'
rg -n "video_jobs|video_type|queued \| processing \| completed \| failed|check\\(|enum\\(" migrations src/configs -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'
printf '\n== files under migrations ==\n'
find migrations -maxdepth 2 -type f | sort | sed -n '1,120p'Repository: knoxiboy/DoubtDesk
Length of output: 735
Add a DB constraint for videoJobsTable.status. status is currently a plain varchar, so invalid job states can still be written outside the happy path. A pgEnum or check constraint would keep the queued/processing/completed/failed lifecycle consistent at the database layer.
🤖 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/configs/schema.ts` around lines 522 - 526, Add a database-level
constraint for videoJobsTable.status because it is currently just a varchar and
can accept invalid states. Update the schema in schema.ts near the
videoJobsTable definition to use a pgEnum for the allowed lifecycle values or
add a check constraint, and make sure the status field only permits the
queued/processing/completed/failed states used by the job flow.
Comment on lines
+127
to
+136
| set: async ( | ||
| key: string, | ||
| value: unknown, | ||
| opts?: { nx?: boolean; ex?: number }, | ||
| ): Promise<"OK" | null> => { | ||
| if (opts?.nx && memoryMap.has(key)) return null; | ||
| const ttlMs = opts?.ex ? opts.ex * 1000 : 5 * 60 * 1000; | ||
| memoryMap.set(key, { count: 1, reset: Date.now() + ttlMs }); | ||
| return "OK"; | ||
| }, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
NX check ignores TTL expiry, so mock lock never self-releases.
opts?.nx && memoryMap.has(key) treats any existing map entry as "locked" regardless of whether its reset time has already passed. Since nothing else prunes expired entries, once a key is set with nx, this mock implementation will keep returning null for that key forever — even after the intended ex TTL elapses. This defeats the TTL safety net that the video-generate lock (src/app/api/video/generate/route.ts) relies on to recover if the process fails to reach the cleanup del(lockKey) call.
🔒 Proposed fix: honor expiry in the nx check
set: async (
key: string,
value: unknown,
opts?: { nx?: boolean; ex?: number },
): Promise<"OK" | null> => {
- if (opts?.nx && memoryMap.has(key)) return null;
+ const existing = memoryMap.get(key);
+ const isActive = existing ? Date.now() < existing.reset : false;
+ if (opts?.nx && isActive) return null;
const ttlMs = opts?.ex ? opts.ex * 1000 : 5 * 60 * 1000;
memoryMap.set(key, { count: 1, reset: Date.now() + ttlMs });
return "OK";
},📝 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
| set: async ( | |
| key: string, | |
| value: unknown, | |
| opts?: { nx?: boolean; ex?: number }, | |
| ): Promise<"OK" | null> => { | |
| if (opts?.nx && memoryMap.has(key)) return null; | |
| const ttlMs = opts?.ex ? opts.ex * 1000 : 5 * 60 * 1000; | |
| memoryMap.set(key, { count: 1, reset: Date.now() + ttlMs }); | |
| return "OK"; | |
| }, | |
| set: async ( | |
| key: string, | |
| value: unknown, | |
| opts?: { nx?: boolean; ex?: number }, | |
| ): Promise<"OK" | null> => { | |
| const existing = memoryMap.get(key); | |
| const isActive = existing ? Date.now() < existing.reset : false; | |
| if (opts?.nx && isActive) return null; | |
| const ttlMs = opts?.ex ? opts.ex * 1000 : 5 * 60 * 1000; | |
| memoryMap.set(key, { count: 1, reset: Date.now() + ttlMs }); | |
| return "OK"; | |
| }, |
🤖 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/ratelimit.ts` around lines 127 - 136, The nx handling in set ignores
expiration, so expired entries in memoryMap still block new writes forever.
Update the mock lock logic in set to check the stored reset timestamp before
treating a key as present, and allow nx writes once the previous entry has
expired; keep the behavior aligned with the TTL passed via opts?.ex so the lock
self-releases correctly.
Comment on lines
+43
to
+44
| const { data } = supabase.storage.from(VIDEO_BUCKET).getPublicUrl(objectName); | ||
| return data.publicUrl; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Supabase Storage getPublicUrl vs createSignedUrl private bucket behavior
💡 Result:
In Supabase Storage, the behavior of getPublicUrl and createSignedUrl differs fundamentally based on whether a bucket is configured as public or private [1][2]. getPublicUrl The getPublicUrl method is a convenience function used to construct the URL for an asset in a public bucket [3]. It does not perform any server-side verification to check if the bucket is actually public [3]. If you use this method on a file located in a private bucket, the resulting URL will be generated, but any attempt to access that URL will fail (typically returning a 400 or 403 error) because the bucket's access control rules prohibit unauthorized public access [3][4]. createSignedUrl The createSignedUrl method is the required approach for accessing files in private buckets [4]. It generates a time-limited, signed URL that grants temporary access to a specific file [5][4]. Because the URL itself acts as the credential, it allows anyone with the link to download or view the asset until the specified expiration time is reached [4][2]. This is the standard mechanism for sharing private content securely without exposing the bucket publicly [4][2]. Summary of Behavior Bucket Type | getPublicUrl | createSignedUrl --- | --- | --- Public | Returns a functional public URL [3][2]. | Functions as expected (generates a signed URL) [5]. Private | Returns a URL, but access will be denied [3][4]. | Returns a functional, time-limited URL [4][2]. To secure files, you should keep buckets private and use createSignedUrl to grant temporary access, often in conjunction with Row Level Security (RLS) policies to ensure only authorized users can request these signed URLs [1][4][2].
Citations:
- 1: https://supabase.com/docs/guides/storage/buckets/fundamentals
- 2: https://plainform.dev/blog/supabase-storage-for-saas-apps-files-permissions-and-tradeoffs
- 3: https://supabase.com/docs/reference/javascript/storage-from-getpublicurl
- 4: https://www.rapidevelopers.com/supabase-tutorial/how-to-secure-supabase-storage-files
- 5: https://supabase.com/docs/reference/javascript/file-buckets-createsignedurl
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files of interest:\n'
git ls-files 'src/lib/video/*' 'src/lib/**/storage.ts' 'supabase*' '.env*' 'README.md' | sed 's#^`#-` #'
printf '\nOutline for src/lib/video/storage.ts:\n'
ast-grep outline src/lib/video/storage.ts --view expanded || true
printf '\nNearby source for src/lib/video/storage.ts:\n'
wc -l src/lib/video/storage.ts
sed -n '1,220p' src/lib/video/storage.ts
printf '\nSearch for SUPABASE_VIDEO_BUCKET and bucket visibility hints:\n'
rg -n "SUPABASE_VIDEO_BUCKET|createSignedUrl|getPublicUrl|public bucket|private bucket|storage bucket" src README.md .env* supabase* || trueRepository: knoxiboy/DoubtDesk
Length of output: 2800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'README mentions:\n'
rg -n "Supabase|storage|bucket|videoUrl|video" README.md .env.example src/lib/video -g '!*.map' || true
printf '\nVideo storage call sites:\n'
rg -n "uploadVideo\(" src || true
printf '\nAll mentions of public URL helpers in repo:\n'
rg -n "getPublicUrl|createSignedUrl" src README.md .env.example || trueRepository: knoxiboy/DoubtDesk
Length of output: 4294
getPublicUrl() requires a public bucket
uploadVideo() can still write with the service-role key when SUPABASE_VIDEO_BUCKET is private, but the persisted videoUrl will be unusable for playback. Keep this bucket public, or mint signed URLs at playback time instead of saving the upload-time URL.
🤖 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 43 - 44, The current video URL
handling in storage.ts persists the result of getPublicUrl() from VIDEO_BUCKET,
which only works for a public bucket and will be unusable if the bucket is
private. Update the uploadVideo/getPublicUrl flow so the bucket is guaranteed
public or, if privacy must remain, stop storing the upload-time public URL and
instead generate a signed URL at playback time using the relevant storage
helper.
Owner
|
@madsysharma look into the ci test failures and try to fix it |
knoxiboy
requested changes
Jul 2, 2026
knoxiboy
left a comment
knoxiboy
left a comment
Owner
There was a problem hiding this comment.
Implement the suggestions from bots
github-actions
Bot
added
gssoc:approved
knoxiboy
added
size/xl
review-needed
level:advanced
knoxiboy
added
gssoc:approved
anshul23102
added a commit
to anshul23102/DoubtDesk
that referenced
this pull request
Jul 7, 2026
- Restore orgRoleEnum, organizationsTable, organizationMembershipsTable - Add organizationId column to classroomsTable with FK constraint - Re-enable multi-tenant organization feature that was removed in PR knoxiboy#731 - Consumers of these exports (organizations/route.ts, rooms/route.ts, analytics/route.ts) now compile without errors Fixes knoxiboy#777
anshul23102
added a commit
to anshul23102/DoubtDesk
that referenced
this pull request
Jul 7, 2026
- Restore orgRoleEnum, organizationsTable, organizationMembershipsTable - Add organizationId column to classroomsTable with FK constraint - Re-enable multi-tenant organization feature that was removed in PR knoxiboy#731 - Consumers of these exports (organizations/route.ts, rooms/route.ts, analytics/route.ts) now compile without errors Fixes knoxiboy#777
anshul23102
added a commit
to anshul23102/DoubtDesk
that referenced
this pull request
Jul 7, 2026
- Restore orgRoleEnum, organizationsTable, organizationMembershipsTable - Add organizationId column to classroomsTable with FK constraint - Re-enable multi-tenant organization feature that was removed in PR knoxiboy#731 - Consumers of these exports (organizations/route.ts, rooms/route.ts, analytics/route.ts) now compile without errors Fixes knoxiboy#777
anshul23102
added a commit
to anshul23102/DoubtDesk
that referenced
this pull request
Jul 7, 2026
Restores multi-tenant organization support accidentally deleted in PR knoxiboy#731: - Restores orgRoleEnum, organizationsTable, organizationMembershipsTable - Adds organizationId column to classroomsTable with FK constraint - Ensures consumers (organizations/route.ts, rooms/route.ts) compile without errors Also fixes pre-existing test failures and TypeScript errors: - teacher-insights test: update error message assertions to match endpoint returns - digest-functions test: add inngest client mock, fix email result mock - migration integrity: remove orphan migration files with duplicate prefixes - TypeScript: fix implicit-any and readonly-property errors in route handlers Fixes knoxiboy#777 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This was referenced Jul 8, 2026
Merged
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
2 participants
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
Description
Refactors the video generation pipeline (OCR -> AI script -> Google TTS -> Remotion render) from a synchronous request handler into an async Inngest background job with real-time SSE progress. The pipeline takes 30-60s and previously ran inline in
POST /api/video/generate, exceeding the serverless function timeout.Related Issue
Closes #321.
Type of Change
Architecture
What changed
video_jobstable (src/configs/schema.ts+drizzle/0016_video_jobs.sql)id, userEmail, status (queued|processing|completed|failed), progress, step, videoType, videoUrl, error, createdAt, updatedAt, indexed onuserEmailand(status, createdAt). Migration is a hand-written idempotentCREATE TABLE IF NOT EXISTS(and FK/indexes) with a matching_journal.jsonentry, matching the repo's manual-migration convention and passing the migration integrity test.Pipeline extraction (
src/lib/video/pipeline.ts)The exact pipeline logic was moved, behavior-preserving, into
runVideoPipeline({ content, imageUrl, baseUrl }, onProgress), which returns{ videoUrl, videoType }and reports progress between stages. (baseUrlis passed in because there's no request object in the background job; the unusedffmpeg-staticimport was dropped.)Enqueue endpoint (
POST /api/video/generate)Keeps auth, rate-limit, block-check, and validation, then inserts a
queuedjob, emits the Inngest event, and returns{ jobId, status }with 202 immediately. The per-user redis lock is now released by the background job (a 5-min TTL guards against leaks); if enqueue itself fails, the lock is released so the user can retry.Background job and cleanup (
src/inngest/functions.ts, registered in the serve route)generateVideoruns the pipeline, persistsprogress/step/statusto the job row after each stage, markscompleted(withvideoUrl) orfailed(witherror), and releases the lock infinally.cleanupStaleVideoJobs(cron every 15 min) marks jobs stuck inqueued/processingfor >15 min asfailedso the UI never spins forever.SSE progress endpoint (
GET /api/video/status)Owner-only stream that emits the job snapshot whenever it changes and closes on a terminal status, with a 4-minute cap (the browser's
EventSourcetransparently reconnects to keep receiving updates past the cap). Sendstext/event-streamwithno-cache/X-Accel-Buffering: no, and closes on client disconnect (req.signalabort).Client hook (
src/hooks/use-video-generation.ts)There is currently no frontend component that calls the video API (only
middleware.tsxand the API itself reference it), so there was nothing to "update". This hook provides the wiring:generate({content|imageUrl})-> POST -> opens theEventSource-> exposes{ status, progress, step, videoUrl, error, isGenerating }. A component can drop it in to render a progress bar.Backwards compatibility
The success response changed shape: synchronous
{ videoUrl, type }-> async{ jobId, status }(202). Callers must now read progress/result from/api/video/status. Since no in-repo frontend consumed the old response, this has no internal callers to break, but any external client must migrate.Design decisions
EventSourceauto-reconnect bridges that. If the deployment target makes long SSE impractical, the samevideo_jobsrow supports plain polling with no server changes.retries: 0, since it writes files and is not idempotent). Splitting each stage into its own retriablestep.runis a sensible future enhancement.Testing
npx tsc --noEmitintroduces no new errors and the full suite shows no new failures vs a cleanmaincheckout (upstreammainalready has 27 pre-existingtscerrors and 4 failing suites unrelated to this PR: verified identical before/after.Notes
drizzle/0016_video_jobs.sqlwith a_journal.jsonentry (idx 16). PR [ADVANCED] Implement Database-Level Pagination with Cursor-Based Navigation for Doubts Feed #319 also adds a0016_*migration, so whichever lands second must bump to0017(filename + journal idx) to keep version numbers unique (the migration-integrity test added by PR fix(devops): resolve 14 duplicate version-number migration files in drizzle/ #722 enforces this).npx drizzle-kit generatestill cannot run non-interactively onmain(a pre-existingschema.ts<-> snapshot conflict), so that gate is red independent of this PR; the hand-written idempotent migration follows the repo's manual approach.Checklist
CodeAnt-AI Description
Move video generation to a background job with live progress updates
What Changed
Impact
✅ Fewer video generation timeouts✅ Faster video start after submit✅ Clearer progress during long renders💡 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