youtubedownloader/.ai/REVIEW.md

REVIEW

Append-only review log. Each task section is appended or updated in place; prior task history is preserved.

---

T-001 — FastAPI backend (app.py) + inline HTML/JS frontend (requirements.txt)

Verdict: PASS_WITH_NOTES

Findings

1. [minor] app.py:147 — On any failure inside download() (including yt-dlp errors), str(exc) is returned verbatim to the browser as the message field. yt-dlp exceptions can include local filesystem paths (the temp dir) or verbose internal detail. Not a required fix for this local/self-hosted tool, but consider mapping to a generic message (e.g. "The video could not be downloaded.") and logging the raw exception server-side instead, before this is exposed beyond local/dev use.
   • Required fix: No.

No blocker or major findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 1 spec for T-001) and diffed it against app.py / requirements.txt.
• python -m py_compile app.py — succeeded.
• Installed fastapi, uvicorn, httpx, python-multipart, yt-dlp into a scratch venv and exercised the app with starlette.testclient.TestClient, mocking yt_dlp.YoutubeDL to avoid live network calls:
  • GET / → 200, body contains "Download MP3" and "Download Video".
  • POST /download with a garbage (non-http) URL → 400 JSON {"message": ...}.
  • POST /download with an empty URL → 400 JSON.
  • POST /download missing the mode field (triggers RequestValidationError handler) → 400 JSON with .message.
  • POST /download with a valid URL, mode mp3, mocked yt-dlp writing a fake file → 200, correct bytes streamed, Content-Disposition: attachment; filename="...", temp dir cleaned up via BackgroundTask.
  • POST /download where mocked yt-dlp raises RuntimeError → 500 JSON {"message": "..."} (no server crash), temp dir cleaned up on the error path.
  • Filename containing non-ASCII characters (e.g. "café mix 🎵.mp3") → correctly RFC 5987-encoded as filename*=utf-8''... by Starlette; no crash or mojibake.

Findings from verification: All acceptance criteria for T-001 hold:

• GET / returns 200 with HTML containing "Download MP3".
• POST /download with a valid URL + mode streams a file.
• Invalid URL returns 400 JSON with .message.

Risks:

• No live network test against real YouTube was performed (would require external network access and a real video URL); mocked yt-dlp calls confirm the FastAPI plumbing (routing, form validation, error handling, file streaming, temp-dir cleanup) is correct, but do not confirm yt-dlp/ffmpeg behavior against a live video. This risk carries into the Docker-level validation planned for T-002 (docker build + smoke test), where a live download should be attempted at least once.
• Large/long-running downloads have no timeout or size cap — acceptable for v1 per ROADMAP.md (out of scope), but worth revisiting if this moves beyond local/dev use.

---

T-002 — Dockerfile + README.md (build/run docs)

Verdict: PASS

Findings

1. [nit] Dockerfile — the container runs as root (no USER directive). Not required for a local/self-hosted v1 tool per ROADMAP.md, but worth adding a non-root user if this is ever exposed beyond a trusted local network.
   • Required fix: No.

No blocker, major, or minor findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 2 spec for T-002) and diffed it against Dockerfile / README.md; matches the planned Dockerfile contents and documented build/run/usage/size-note requirements.
• docker build -t yt-dl-review . — exited 0.
• docker run --rm -d -p 8091:8080 yt-dl-review — container started; curl http://localhost:8091/ returned HTTP 200 with both "Download MP3" and "Download Video" present in the body.
• POST /download with an invalid URL against the running container → 400 JSON {"message": "..."}", confirming the containerized app behaves the same as the local dev checks from T-001; docker logs showed clean request handling with no crash/traceback.
• Live end-to-end test (real network, real YouTube video https://www.youtube.com/watch?v=jNQXAC9IVRw, "Me at the zoo"):
  • mode=mp3 → 200, content-type: audio/mpeg, valid ID3-tagged MP3 file returned (verified with file), Content-Disposition filename correctly derived from video title.
  • mode=video → 200, content-type: video/mp4, valid ISO-Media MP4 file returned (verified with file).
  • This closes the live-download risk flagged in the T-001 review — yt-dlp + ffmpeg inside the container work correctly end-to-end for both modes.
• Cleaned up: stopped test container, removed the yt-dl-review test image, removed downloaded test files.

Findings from verification: All acceptance criteria for T-002 hold:

• docker build -t yt-dl . exits 0.
• docker run --rm -p 8080:8080 yt-dl starts the server.
• curl http://localhost:8080/ returns HTML with both download buttons.
• (Bonus, beyond stated AC) A real MP3 and a real video download both succeed end-to-end through the container.

Risks:

• None outstanding. The live-download risk noted in the T-001 review has been verified and closed here.

---

T-003 — noplaylist option, sanitize_filename helper, title-based filenames, frontend Content-Disposition fix

Verdict: PASS

Findings

No blocker, major, minor, or nit findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 1 spec for T-003) and diffed it against app.py / README.md; implementation matches the plan (noplaylist: True on both modes, sanitize_filename helper with the specified regex, extract_info(..., download=True) used to recover title/id, filename=display_name passed to FileResponse, frontend Content-Disposition-based link.download fix).
• python3 -m py_compile app.py — succeeded.
• Unit-checked sanitize_filename directly (scratch venv, no mocks):
  • sanitize_filename("Song 🔥 Title ✨ (Live)", "abc123") → "Song Title (Live)".
  • sanitize_filename("🔥🔥🔥", "abc123") → "abc123" (fallback to id).
  • sanitize_filename("日本語 タイトル", "abc123") → "日本語 タイトル" (non-Latin preserved).
  • Verified Starlette's FileResponse encodes such filenames via filename*=utf-8''... (RFC 5987) when they contain non-ASCII/space/apostrophe characters, and confirmed the frontend regex (/filename\*?=(?:UTF-8''|")?([^";]+)/i) plus decodeURIComponent correctly recovers the original title from that header format.
• Live end-to-end test, built and ran the actual Docker image (docker build -t yt-dl-review ., docker run -p 8081:8080):
  • GET / → 200, contains "Download MP3".
  • POST /download with a playlist-context URL (watch?v=dQw4w9WgXcQ&list=RDdQw4w9WgXcQ), mode=mp3 → 200; container logs show Downloading just the video dQw4w9WgXcQ because of --no-playlist, confirming only the referenced video is fetched, never the rest of the playlist/mix; response is a single valid MP3 (file confirms ID3 v2.4 MPEG audio) with Content-Disposition: attachment; filename*=utf-8''Rick Astley - Never Gonna Give You Up (Official Video) (4K Remaster).mp3 — title-based, not a UUID.
  • mode=video against a plain (non-playlist) URL → 200, valid MP4 (file confirms ISO Media MP4), filename Me at the zoo.mp4 derived from the video title.
  • POST /download with an invalid (non-http) URL → unchanged 400 JSON {"message": ...} behavior, confirming the existing validation path was not regressed.
  • Cleaned up: stopped/removed the test container and scratch venv, deleted downloaded test files.
• Reviewed README.md changes; the new paragraph accurately describes the playlist-single-video behavior and the title-based/emoji-stripped filename rule, matching observed behavior.

Findings from verification: All acceptance criteria for T-003 hold:

• Pasting a playlist/list= URL downloads only the referenced video (confirmed via yt-dlp's own --no-playlist log line and a single file being produced).
• Saved filename matches the video title with emoji/icons stripped (normal chars, including non-Latin, kept) instead of a UUID, with the correct extension.
• python -m py_compile app.py passes.

Risks:

• None outstanding. sanitize_filename strips characters outside \w/whitespace/- _ . ( ) , '; this only affects the Content-Disposition header value used for the browser's save-as name, not the on-disk path used to read the file, so there is no path-traversal or injection concern from unsanitized titles.

---

T-004 — Job-based POST /download + GET /progress/{job_id} + GET /download/{job_id}/file, in-memory job store, yt-dlp progress_hooks, frontend polling + progress bar UI

Verdict: PASS_WITH_NOTES

Findings

1. [minor] app.py:296-315 (progress_hook) — job.percent is only updated inside if job.total_bytes:, so when yt-dlp can't report a total_bytes/total_bytes_estimate (e.g. some DASH/live formats), the progress bar stays at 0% for the whole download even though downloaded_bytes is advancing and shown as text. Not required: the plan explicitly allows "downloaded size only if total is unknown" as the fallback UX, and this doesn't affect the common case (verified live: percent tracked correctly when total is known).
   • Required fix: No.
2. [minor] app.py:296-367 — progress_hooks only reports raw download percent; ffmpeg post-processing (mp3 extraction, video mux) after the download hits 100% is not reflected, so the bar can sit at 100% for a few extra seconds while the file is finalized before status flips to finished. Acceptable per plan (postprocessor_hooks was offered as one of two options, not mandatory), and verified it does not stall indefinitely or misreport.
   • Required fix: No.
3. [nit] app.py:356-362 — the friendlier "The download could not be prepared." message set by progress_hook on a hook-level status == "error" is generally overwritten by the broader except Exception as exc: ... job.error_message = str(exc) handler in run_job, since yt-dlp raises after invoking the hook. In practice this means the browser sees yt-dlp's raw exception text (confirmed in testing: "ERROR: [generic] not-a-video: Unable to download webpage: HTTP Error 404...") rather than the friendlier hook message. Same class of note as the T-001 review (raw exception text surfaced to the browser); not a regression introduced by this task and not required to fix here.
   • Required fix: No.

No blocker or major findings.

Verification

Steps performed:

• Re-read .ai/PLAN.md (Phase 2 spec for T-004) and diffed it against app.py / README.md; the three-endpoint split, JobState/_jobs/_jobs_lock, progress_hooks-driven updates, run_in_executor fire-and-forget scheduling (_pending_jobs set to avoid GC warnings), stale-job TTL sweep piggybacked on POST /download, and the frontend polling/progress-bar/button-disable/error-handling all match the plan.
• python3 -m py_compile app.py — succeeded.
• Built the actual Docker image (docker build -t yt-dl-review2 .) and ran it, then drove the real HTTP flow end-to-end against real YouTube URLs:
  • POST /download (playlist-context MP3 URL) → {"job_id": ...}; polling GET /progress/{job_id} every ~1s showed status: "downloading" with percent/speed/downloaded_bytes/total_bytes advancing correctly (0 → 61% → 100%), then status: "finished".
  • GET /download/{job_id}/file after finished → 200, correct MP3 bytes (file confirms valid MPEG audio), title-based Content-Disposition filename (reusing T-003's sanitization) — auto-delivered with no extra click needed on the client side.
  • Job cleanup confirmed: GET /progress/{job_id} after the file was served → 404 "Download job not found.", confirming BackgroundTask(cleanup_job, ...) removed the job and temp dir after delivery.
  • GET /progress/<unknown> and GET /download/<unknown>/file → both 404 as specified.
  • Fetched GET /download/{job_id}/file on a still-downloading job → 409 "The download is not ready yet.", matching the plan's pre-completion semantics.
  • Error path: posted a non-YouTube, non-existent URL (http://example.com/not-a-video) → job transitioned to status: "error" with a descriptive error message, no server crash (GET / still returned 200 immediately after), and docker logs showed no traceback — matches "server-side failure shows an error in place of the bar with no crash."
  • Confirmed no temp-dir leak on the error path (docker exec ... ls /tmp showed the errored job's temp dir already removed); a separate job's temp dir that was intentionally never fetched was still present, consistent with the documented "removed after delivery, or after a timeout for abandoned jobs" behavior (not fetching a finished job's file is expected to leave it until TTL/next sweep).
  • Cleaned up: stopped the test container, removed the yt-dl-review2/yt-dl-review test images, removed local scratch files.
• Reviewed README.md changes: accurately documents the job/progress/file endpoint flow, automatic delivery, and non-persistent in-memory job state.

Findings from verification: All acceptance criteria for T-004 hold:

• Progress bar appears on download start and updates with percent/speed/downloaded-total size (verified live with real percentages/speeds).
• File auto-delivers to the browser on completion with the correct sanitized filename (server-side fetch + Content-Disposition flow verified; same header-parsing logic as T-003, already confirmed to round-trip correctly in the browser).
• Server-side failure shows an error in place of the bar with no crash (verified: job marked error, server remained responsive).
• python -m py_compile app.py passes.

Risks:

• Progress percent can remain at 0% for formats where yt-dlp cannot report a total size (noted above, minor, not required).
• The in-memory job store means an app restart mid-download loses all job state/progress for any in-flight browser sessions; this matches the explicitly stated "no persistent storage" design constraint and is now documented in README.md.