Add yt-dlp web UI import#40
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesVideo URL Download Feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/web-server.ts (1)
363-506: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict download targets to public hosts.
validateDownloadUrlonly checks the scheme, so an exposed/api/download-videoendpoint can fetch loopback, link-local, or RFC1918 URLs server-side and register the result for streaming. Add host/IP blocking, with DNS resolution, before invoking yt-dlp.🤖 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/ui/web-server.ts` around lines 363 - 506, The download URL validation in validateDownloadUrl only enforces http/https, so /api/download-video can still reach loopback, private, or link-local targets server-side. Extend validateDownloadUrl (and the /api/download-video flow that calls it) to reject non-public hosts by resolving the hostname and blocking private/reserved IP ranges before spawning yt-dlp. Make sure the check covers direct IPs and DNS-resolved addresses, and return a 400 with a clear validation error when a target is not allowed.
🤖 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/ui/client/EpisodeWorkspace.jsx`:
- Around line 1118-1119: The “Find best moments” flow in EpisodeWorkspace.jsx
needs to block generation when the source is still a URL and hasn’t been
downloaded yet. Update the button logic near the Generate action to use the
existing isProcessing and sourceIsUrl flags together, so the action is disabled
or short-circuited for un-downloaded URL sources. In the Generate handler/button
wiring, reference the existing sourceIsUrl and isProcessing checks to show a
clear “download the video first” message instead of letting /select-file fail on
the raw URL.
In `@src/ui/web-server.ts`:
- Around line 397-420: The /api/download-video handler is holding the request
open for the full yt-dlp run without exposing the created job to the client.
Update the download-video flow in web-server.ts to return the generated jobId
immediately, and hook the existing jobs/job broadcast pattern so progress and
completion can be polled or streamed like the transcribe job flow. Also add a
req.on("close") cleanup path in this handler to terminate the spawned proc when
the client disconnects, and ensure any status updates keep using the existing
JobState/job tracking symbols.
- Around line 422-444: The yt-dlp spawn setup in web-server.ts is passing a full
Node executable via --js-runtimes, which exposes the web server’s permissions to
remote EJS execution. Update the arguments built in the yt-dlp launcher to avoid
using process.execPath as the runtime; either switch the runtime to a sandboxed
alternative such as Deno or remove the remote component path entirely. If Node
must remain, explicitly gate it behind a trusted-input check and document the
trust boundary in the same spawn/args setup.
---
Outside diff comments:
In `@src/ui/web-server.ts`:
- Around line 363-506: The download URL validation in validateDownloadUrl only
enforces http/https, so /api/download-video can still reach loopback, private,
or link-local targets server-side. Extend validateDownloadUrl (and the
/api/download-video flow that calls it) to reject non-public hosts by resolving
the hostname and blocking private/reserved IP ranges before spawning yt-dlp.
Make sure the check covers direct IPs and DNS-resolved addresses, and return a
400 with a clear validation error when a target is not allowed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10703565-5133-43d9-92d1-b92e8823bef7
📒 Files selected for processing (4)
backend/requirements-runtime.txtbackend/requirements.txtsrc/ui/client/EpisodeWorkspace.jsxsrc/ui/web-server.ts
teethatkamsai
force-pushed
the
codex/yt-dlp-web-ui
branch
from
94fcce3 to
6a1ef2c
Compare
|
Verified all findings against current code; all were still valid and fixed. Fixed:
Validated:
Amended and force-pushed:
|
nmbrthirteen
left a comment
nmbrthirteen
left a comment
There was a problem hiding this comment.
Nice feature, wiring looks clean and filename handling is safe. Two things to look at before merge (details inline). Tested yt-dlp with these exact args locally to confirm.
teethatkamsai
force-pushed
the
codex/yt-dlp-web-ui
branch
from
6a1ef2c to
4ad4ecc
Compare
|
Addressed the review comments and pushed the updated PR branch. Fixed:
Skipped:
Validated:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/ui/client/EpisodeWorkspace.jsx (1)
913-935: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnguarded
downloadStream.resultaccess.
const d = downloadStream.result; setFile(d); setVideoPath(d.file_path);assumesresultis always populated whenstatus === 'done'. If the backend ever marks the job done without a result payload, this throws inside an effect. The siblingbatchStreamhandling elsewhere uses optional chaining (batchStream.result?.results || []) for the same kind of access — worth being consistent here as a defensive measure.- if (downloadStream.status === 'done') { - const d = downloadStream.result; + if (downloadStream.status === 'done') { + const d = downloadStream.result || {}; + if (!d.file_path) { setError('Download completed but no file was returned.'); setDownloadingVideo(false); setDownloadJobId(null); return; } setFile(d); setVideoPath(d.file_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/ui/client/EpisodeWorkspace.jsx` around lines 913 - 935, Guard the `downloadStream.result` access inside `EpisodeWorkspace`’s `useEffect` that handles `downloadStream.status === 'done'`; `result` may be missing even when the job is done, so update the `setFile`/`setVideoPath` flow to safely handle a nullish payload instead of assuming `d.file_path` always exists. Use the existing `downloadStream` effect and its sibling `batchStream` optional-chaining pattern as a guide, and make sure the effect still clears download state cleanly when no result is present.src/ui/web-server.ts (1)
499-513: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilepath detection via "last non-bracket line" is fragile.
outputFilePathis set to any trimmed line that isn't a progress match, doesn't start with[, and isn't aWARNING:. Any other plain stdout/stderr line printed afterafter_move:filepath(e.g. postprocessor notices like "Deleting original file …", or apodcli-progress:line whose percent renders asN/A/---.-and fails the numeric regex) would overwrite the real path and trigger a false "did not report an output file" error at Line 547. Prefer a unique sentinel on the print template and parse that explicitly.♻️ Suggested approach
"--print", - "after_move:filepath", + "after_move:podcli-filepath:%(filepath)s",const trimmed = line.trim(); - if (trimmed && !trimmed.startsWith("[") && !trimmed.startsWith("WARNING:")) { - outputFilePath = trimmed; - } + const fp = trimmed.match(/^podcli-filepath:(.+)$/); + if (fp) outputFilePath = fp[1];🤖 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/ui/web-server.ts` around lines 499 - 513, The current output path detection in readYtDlpOutput is too broad because it treats the last non-bracket, non-warning line as the filepath, which can be overwritten by unrelated yt-dlp output. Update the parsing in readYtDlpOutput and the corresponding after_move handling to look for a unique sentinel/tagged line emitted by the print template, and only assign outputFilePath from that explicit marker. This should prevent postprocessor notices or malformed podcli-progress lines from clobbering the real path and avoid the false missing-output error later in the flow.
🤖 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/ui/client/EpisodeWorkspace.jsx`:
- Around line 902-911: The download flow in downloadVideo leaves
downloadingVideo stuck true whenever api('/download-video') returns an error or
throws, which blocks the workspace UI. Update EpisodeWorkspace’s downloadVideo
handler to always reset downloadingVideo in every failure path, ideally via a
finally block or explicit reset before returning on d.error and in the catch
branch, while preserving the existing success path that sets downloadJobId.
In `@src/ui/web-server.ts`:
- Around line 382-391: The isPublicIp IPv6 handling currently treats IPv4-mapped
IPv6 values like ::ffff:127.0.0.1 as public because they skip the private-range
checks. Update the isPublicIp logic to unwrap mapped/embedded IPv4 addresses
before the family === 6 branch, then apply the existing IPv4 private-range
validation to the extracted IPv4 value. Keep the fix localized around isPublicIp
and its IPv6 normalization path.
- Line 446: The `app.post("/api/download-video", async ...)` handler in
`web-server.ts` does not handle a rejected `mkdir(uploadDir, { recursive: true
})`, so wrap the upload directory setup and subsequent logic in a `try/catch`
and either return a 500 response or pass the error to `next(err)` from that
handler. Keep the fix localized around the `mkdir` call and the surrounding
route logic so failures cannot escape the Express 4.21.0 request lifecycle
unresolved.
---
Nitpick comments:
In `@src/ui/client/EpisodeWorkspace.jsx`:
- Around line 913-935: Guard the `downloadStream.result` access inside
`EpisodeWorkspace`’s `useEffect` that handles `downloadStream.status ===
'done'`; `result` may be missing even when the job is done, so update the
`setFile`/`setVideoPath` flow to safely handle a nullish payload instead of
assuming `d.file_path` always exists. Use the existing `downloadStream` effect
and its sibling `batchStream` optional-chaining pattern as a guide, and make
sure the effect still clears download state cleanly when no result is present.
In `@src/ui/web-server.ts`:
- Around line 499-513: The current output path detection in readYtDlpOutput is
too broad because it treats the last non-bracket, non-warning line as the
filepath, which can be overwritten by unrelated yt-dlp output. Update the
parsing in readYtDlpOutput and the corresponding after_move handling to look for
a unique sentinel/tagged line emitted by the print template, and only assign
outputFilePath from that explicit marker. This should prevent postprocessor
notices or malformed podcli-progress lines from clobbering the real path and
avoid the false missing-output error later in the flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6026df89-cf49-46c8-a11e-1d4c4fe21119
📒 Files selected for processing (4)
backend/requirements-runtime.txtbackend/requirements.txtsrc/ui/client/EpisodeWorkspace.jsxsrc/ui/web-server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/requirements-runtime.txt
- backend/requirements.txt
teethatkamsai
force-pushed
the
codex/yt-dlp-web-ui
branch
from
4ad4ecc to
26e61c5
Compare
|
Verified against current code; all listed findings were still valid and fixed. Fixed:
Validated:
|
2 participants
Summary
Adds yt-dlp support to the web UI so users can paste a YouTube or direct video URL into the existing video source field, download it locally, and continue through the current preview/transcription workflow.
Changes
yt-dlpto backend runtime requirements.POST /api/download-videofor server-side URL validation and downloads.Validation
npm.cmd run buildnpm.cmd test/api/download-video.Summary by CodeRabbit