feat: Twitch VOD support#551
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Twitch VOD download support: new CLI flags and Docker mount for persistent VOD data, per-channel YAML VOD settings, SQLite-backed VOD state tracking, gRPC VOD listing endpoint and client, yt-dlp + FFmpeg download flow, regenerated protobuf stubs, tests, and README updates. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Config as Config/YAML
participant GrpcSrv as gRPC Server
participant YtDlp as yt-dlp
participant VodDB as SQLite VodDB
participant FFmpeg as FFmpeg
Client->>Config: read streamer (vod, vod_limit)
Client->>GrpcSrv: GetVods(site, user, limit)
GrpcSrv->>YtDlp: list videos (playlist metadata)
YtDlp-->>GrpcSrv: vod metadata
GrpcSrv-->>Client: VodResponse(vods)
loop per VOD candidate
Client->>VodDB: ClaimVOD(id, staleThreshold)
VodDB-->>Client: claimed? (true/false)
alt claimed
Client->>GrpcSrv: GetStream("videos/"+id) → download URL
GrpcSrv-->>Client: download URL
Client->>FFmpeg: spawn ffmpeg (-c:v copy -c:a copy ...)
FFmpeg-->>Client: exit success/failure
alt success
Client->>VodDB: MarkVODCompleted(id)
else failure
Client->>VodDB: MarkVODFailed(id)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add ability to download past broadcasts (VODs) from Twitch channels. Per-channel `vod: true` config toggle switches from live stream to VOD mode, with a SQLite database tracking download state to avoid re-downloading and handle crash recovery via stale threshold detection. - Add GetVods gRPC RPC using yt-dlp for VOD enumeration - Add SQLite VOD tracking with status-based lifecycle (downloading/completed/failed) - Add downloadVOD function with stream copy and VOD-specific filenames - Add -data, -vod-out, -vod-move flags for configurable paths - Wire VOD branch into main tick loop with ShouldDownloadVOD gating - Add VOD phase to integration test - Update README and example config
8 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 133-144: The README currently implies /app/data is the fixed VOD
DB location; update the wording to explicitly state that /app/data is the
default data directory and that it can be overridden with the -data flag (or
equivalent CLI/config option), and instruct users to mount the directory they
specify with -data rather than always mounting /app/data; reference the README
lines describing the SQLite DB path and the Docker volume snippet so you change
those sentences to mention the default and the configurable -data option.
In `@stream_pb2_grpc.py`:
- Around line 19-24: The RuntimeError message in stream_pb2_grpc.py concatenates
several string literals; remove the unnecessary f-string prefix from the second
literal (the one containing " but the generated code in stream_pb2_grpc.py
depends on") because it has no interpolation, leaving the other f-strings with
placeholders ({GRPC_VERSION}, {GRPC_GENERATED_VERSION}) unchanged so the
formatting still works.
In `@stream_pb2.py`:
- Around line 12-19: The generated protobuf runtime check in
_runtime_version.ValidateProtobufRuntimeVersion for 'stream.proto' requires
protobuf >=5.29.0, so update the pyproject.toml protobuf dependency to "protobuf
>=5.29.0,<6.0.0" (leave grpcio as-is since the existing >=1.80.0 already
satisfies the generated requirement); make this change to the project's
dependency manifest and run a quick install/lock to verify the new constraint is
applied.
In `@streamdl.go`:
- Around line 109-149: The VOD download goroutines started by
downloadVOD(streamer.User, ...) send on response but are not counted in the
shutdown drain (only len(urls) for live streams is waited on), so add VOD jobs
to the shutdown tracking: either increment the same live-stream tracking
structure (e.g., append a placeholder to urls) or maintain a shared counter /
sync.WaitGroup that you increment before launching downloadVOD and
decrement/send when the goroutine finishes; update the shutdown path to wait for
both live stream URLs and VOD jobs (or wait on the WaitGroup) so response sends
from VOD jobs are drained before os.Exit. Ensure the change touches the VOD loop
where downloadVOD is invoked and the shutdown/drain logic that currently uses
len(urls).
- Around line 67-72: Currently InitVodDB is called unconditionally during
startup (InitVodDB, vodDB, dataDir), which forces creation/opening of
streamdl.db even when VOD is not enabled and may fail on unwritable default
paths; change the startup flow to check the runtime/config flag that indicates
VOD tracking is enabled (the same config or function that controls vod: true
entries) and only call InitVodDB and defer vodDB.Close() when that flag is set,
otherwise skip DB initialization and the log.Infof message about the VOD DB
entirely; ensure any code paths that reference vodDB are gated by the same flag
so no nil usages occur.
- Around line 174-185: The rate-limit handling around getStream is unreachable
because the inner retry uses an else-if chain that prevents the outer "Rate
Limited Thrice" branch from ever executing; change the logic in the getStream ->
retry -> downloadStream sequence (references: getStream, urls, urlsMu,
downloader call downloadStream, streamer.User) to perform up to three explicit
attempts (e.g., a small loop or a helper that retries up to 3 times) and only
after the third consecutive "rate limited" error log "Rate Limited Thrice" and
skip; ensure each retry sleeps before the next attempt and that successful
attempts update urls/urlsMu and call downloadStream as now.
In `@tests/integration/run.sh`:
- Around line 254-263: The VOD test is flaky because it sets
VOD_CHANNEL="$LIVE_CHANNEL"; change run.sh so VOD_CHANNEL is not reused from
LIVE_CHANNEL but instead comes from a dedicated VOD fixture variable (e.g.
VOD_CHANNEL_FALLBACK or VOD_FIXTURE) or a prioritized list of known channels
with VODs: check an env var like VOD_CHANNEL_OVERRIDE first, then fall back to a
hardcoded reliable VOD channel name or list and select the first available;
update the config generation that writes config.yml (the block referencing
VOD_CHANNEL, quality, vod, vod_limit) to use this new VOD channel variable
(instead of LIVE_CHANNEL) so the VOD phase uses a channel that is guaranteed to
have past broadcasts.
- Around line 250-253: The test currently only removes OUTPUT_DIR outputs but
leaves the persisted VOD SQLite DB in SCRIPT_DIR/data, causing stale state to
skip Phase 5; update the script to also remove or reset the DB files under
"$SCRIPT_DIR/data" before Phase 5 (e.g., delete *.sqlite / *.db or the known VOD
DB filename) so the database state is fresh alongside the OUTPUT_DIR cleanup
(look for usages of OUTPUT_DIR and SCRIPT_DIR in the run.sh snippet to place the
removal).
In `@vod_db_test.go`:
- Around line 84-85: The test currently ignores returned errors from DB calls
like MarkVODStarted and MarkVODFailed (and other DB operations in this test), so
update each call to capture the error and fail the test on error (e.g., err :=
db.MarkVODStarted(...); if err != nil { t.Fatalf("MarkVODStarted failed: %v",
err) } or use require.NoError/AssertNoError), and do the same for MarkVODFailed
and any other DB/test helper calls in this file that drop errors so failures are
surfaced.
In `@vod_db.go`:
- Around line 54-59: Replace the separate check (ShouldDownloadVOD) and claim
(MarkVODStarted) flow with a single atomic DB operation that transitions the row
to "downloading"/"started" and then check rowsAffected to see if we won the
claim; specifically, in the code paths referencing ShouldDownloadVOD and
MarkVODStarted, execute an UPDATE downloaded_vods SET status='downloading',
started_at=? WHERE vod_id=? AND (status!='downloading' OR started_at<?) (using
the passed staleThreshold time for the started_at cutoff) and check
result.RowsAffected()==1 to indicate success; if the row may not exist, attempt
an INSERT ... ON CONFLICT(vod_id) DO UPDATE SET status='downloading',
started_at=EXCLUDED.started_at WHERE downloaded_vods.status!='downloading' OR
downloaded_vods.started_at<? and still check rowsAffected to determine claim
ownership; replace any prior separate QueryRow checks in ShouldDownloadVOD and
separate MarkVODStarted implementations with this single Exec+RowsAffected
pattern.
- Around line 100-121: MarkVODCompleted and MarkVODFailed currently treat
zero-row updates as success; change both to check the Exec result's RowsAffected
(from the result of v.db.Exec) and return an error when rowsAffected == 0 (e.g.,
sql.ErrNoRows or a descriptive fmt.Errorf("vod not found: %s", vodID)), logging
that as a failure; ensure you capture and check the Exec error first, then call
res.RowsAffected() and return/log appropriately in both MarkVODCompleted and
MarkVODFailed.
🪄 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: 8dd6296e-17d3-4029-ba56-0c9ff7e35738
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumprotos/stream.pb.gois excluded by!**/*.pb.goprotos/stream_grpc.pb.gois excluded by!**/*.pb.gouv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
README.mdconfig.goconfig/config.yml.exampleconfig_reader_test.godownload_stream.goentrypoint_client.shgo.modgrpc_client.goprotos/stream.protopyproject.tomlstream_pb2.pystream_pb2_grpc.pystreamdl.gostreamdl_proto_srv.pytests/integration/docker-compose.integration.ymltests/integration/run.shvod_db.govod_db_test.go
Comment on lines
+19
to
+24
| raise RuntimeError( | ||
| f'The grpc package installed is at version {GRPC_VERSION},' | ||
| + f' but the generated code in stream_pb2_grpc.py depends on' | ||
| + f' grpcio>={GRPC_GENERATED_VERSION}.' | ||
| + f' Please upgrade your grpc module to grpcio>={GRPC_GENERATED_VERSION}' | ||
| + f' or downgrade your generated code using grpcio-tools<={GRPC_VERSION}.' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "stream_pb2_grpc.py" -type fRepository: dangeroustech/StreamDL
Length of output: 86
🏁 Script executed:
sed -n '19,24p' ./stream_pb2_grpc.pyRepository: dangeroustech/StreamDL
Length of output: 440
Remove extraneous f prefix from line 21 (Ruff F541).
Line 21 uses an f-string with no placeholder expressions, which violates the F541 rule.
Proposed fix
- + f' but the generated code in stream_pb2_grpc.py depends on'
+ + ' but the generated code in stream_pb2_grpc.py depends on'📝 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
| raise RuntimeError( | |
| f'The grpc package installed is at version {GRPC_VERSION},' | |
| + f' but the generated code in stream_pb2_grpc.py depends on' | |
| + f' grpcio>={GRPC_GENERATED_VERSION}.' | |
| + f' Please upgrade your grpc module to grpcio>={GRPC_GENERATED_VERSION}' | |
| + f' or downgrade your generated code using grpcio-tools<={GRPC_VERSION}.' | |
| raise RuntimeError( | |
| f'The grpc package installed is at version {GRPC_VERSION},' | |
| ' but the generated code in stream_pb2_grpc.py depends on' | |
| f' grpcio>={GRPC_GENERATED_VERSION}.' | |
| f' Please upgrade your grpc module to grpcio>={GRPC_GENERATED_VERSION}' | |
| f' or downgrade your generated code using grpcio-tools<={GRPC_VERSION}.' |
🧰 Tools
🪛 Ruff (0.15.9)
[error] 21-21: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stream_pb2_grpc.py` around lines 19 - 24, The RuntimeError message in
stream_pb2_grpc.py concatenates several string literals; remove the unnecessary
f-string prefix from the second literal (the one containing " but the generated
code in stream_pb2_grpc.py depends on") because it has no interpolation, leaving
the other f-strings with placeholders ({GRPC_VERSION}, {GRPC_GENERATED_VERSION})
unchanged so the formatting still works.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
5-5: Consider removing redundant quotes.The quotes around
"staging"are unnecessary in YAML for simple strings without special characters.✨ Proposed simplification
- - "staging" + - staging🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml at line 5, Remove the unnecessary surrounding quotes from the string "staging" in the YAML list entry so it becomes an unquoted scalar; locate the quoted item "staging" in the .coderabbit.yaml list and replace it with staging (no quotes) to simplify the YAML and avoid redundant quoting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.coderabbit.yaml:
- Line 5: Remove the unnecessary surrounding quotes from the string "staging" in
the YAML list entry so it becomes an unquoted scalar; locate the quoted item
"staging" in the .coderabbit.yaml list and replace it with staging (no quotes)
to simplify the YAML and avoid redundant quoting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a510dbb-590c-4e82-83e5-739fb7e6205f
📒 Files selected for processing (1)
.coderabbit.yaml
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced Apr 14, 2026
VOD downloads now use a WaitGroup so the shutdown path waits for all in-progress VOD jobs before exiting, not just live streams.
Eliminates TOCTOU race by combining the check-and-claim into a single SQL operation that uses ON CONFLICT with a WHERE clause and checks RowsAffected. Also fixes dropped errors in tests.
Return an error when the VOD is not found in the database instead of silently succeeding with zero rows affected.
Only create the SQLite database when a channel with vod:true is encountered, avoiding failures on unwritable default paths when VOD is not configured. Handles config reloads that add VOD channels.
Removes the data directory so the SQLite DB is fresh, preventing previously tracked VODs from causing the test to skip downloads.
Use a known high-profile channel (kaicenat) that reliably has VODs instead of reusing the live stream channel. Overridable via VOD_CHANNEL env var.
Bump minimum protobuf version to match generated code requirements. Add stream_pb2*.py to CodeRabbit path filters since they are auto-generated and should not be reviewed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/run.sh (1)
275-275: Tighten VOD artifact match to.mp4to avoid false positives.Line 275 currently matches any filename containing
_vod_. Since the downloader writes VODs as.mp4, matching that extension makes pass/fail checks more reliable.Proposed patch
- VOD_FILE=$(find "$OUTPUT_DIR/complete" -name "*_vod_*" -size +0c 2>/dev/null | head -1) || true + VOD_FILE=$(find "$OUTPUT_DIR/complete" -name "*_vod_*.mp4" -size +0c 2>/dev/null | head -1) || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run.sh` at line 275, The VOD_FILE find pattern is too broad (currently searching for "*_vod_*") and can pick up non-MP4 artifacts; update the find invocation that sets VOD_FILE to only match MP4 VOD files by restricting the name pattern to include the .mp4 extension (target the VOD_FILE assignment using the existing variable and search directory OUTPUT_DIR/complete so the script only picks files like *_vod_*.mp4).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/run.sh`:
- Line 275: The VOD_FILE find pattern is too broad (currently searching for
"*_vod_*") and can pick up non-MP4 artifacts; update the find invocation that
sets VOD_FILE to only match MP4 VOD files by restricting the name pattern to
include the .mp4 extension (target the VOD_FILE assignment using the existing
variable and search directory OUTPUT_DIR/complete so the script only picks files
like *_vod_*.mp4).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0943bc62-a8b3-4e21-b77e-331029a27611
📒 Files selected for processing (8)
.coderabbit.yamlREADME.mddownload_stream.gopyproject.tomlstreamdl.gotests/integration/run.shvod_db.govod_db_test.go
✅ Files skipped from review due to trivial changes (1)
- .coderabbit.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- pyproject.toml
- README.md
- vod_db_test.go
- streamdl.go
- vod_db.go
- download_stream.go
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/run.sh (1)
271-287: Phase 5 should not require full VOD completion within 180s.
VOD_TIMEOUT=180plus a “completed file only” assertion is brittle for full-length VODs (seestreamdl.goLine 113-150 anddownload_stream.goLine 485-490 flow). Consider making this configurable and treating “download started with non-zero growth” as success for this phase.♻️ Suggested change
-VOD_TIMEOUT=180 +VOD_TIMEOUT="${VOD_TIMEOUT:-600}" VOD_FILE="" +VOD_PROGRESS="" while [ $VOD_ELAPSED -lt $VOD_TIMEOUT ]; do VOD_FILE=$(find "$OUTPUT_DIR/complete" -name "*_vod_*.mp4" -size +0c 2>/dev/null | head -1) || true if [ -n "$VOD_FILE" ]; then break fi + VOD_PROGRESS=$(find "$OUTPUT_DIR/incomplete" -name "*_vod_*.mp4" -size +1000c 2>/dev/null | head -1) || true + if [ -n "$VOD_PROGRESS" ]; then + echo "--- VOD download started: $VOD_PROGRESS ---" + break + fi sleep 5 VOD_ELAPSED=$((VOD_ELAPSED + 5)) done -if [ -z "$VOD_FILE" ]; then - echo "FAIL: No VOD file found after ${VOD_TIMEOUT}s" +if [ -z "$VOD_FILE" ] && [ -z "$VOD_PROGRESS" ]; then + echo "FAIL: No VOD download activity found after ${VOD_TIMEOUT}s" $DC -f "$COMPOSE_FILE" logs client 2>&1 | tail -30 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run.sh` around lines 271 - 287, The test currently requires finding a completed VOD file within a hardcoded VOD_TIMEOUT=180 loop (variables VOD_ELAPSED, VOD_FILE) which is brittle for long VODs; make the timeout configurable (e.g., via env VOD_TIMEOUT or test flag) and change the success condition to detect that a download has started by accepting a non-zero-sized/increasing file (check file size growth across iterations) rather than only a fully completed file; update the loop that uses VOD_ELAPSED/VOD_FILE to read the configurable timeout and to treat a found file with size >0 and observed growth between checks as success so Phase 5 passes earlier while full completion can be validated later (this aligns with the flow in streamdl.go and download_stream.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/run.sh`:
- Around line 255-257: Replace the single default VOD channel assignment
VOD_CHANNEL="${VOD_CHANNEL:-teampgp}" with logic that iterates a prioritized
candidate list (e.g., CANDIDATE_VOD_CHANNELS) and probes each channel for
published VODs; set VOD_CHANNEL to the first candidate that has VODs, and if
none are found, set a flag to skip Phase 5 (the Phase 5 execution block) so the
script exits or continues without running Phase 5. Update the VOD selection code
and the Phase 5 gating logic so Phase 5 only runs when VOD_CHANNEL is set to a
candidate that was verified to have VODs.
---
Nitpick comments:
In `@tests/integration/run.sh`:
- Around line 271-287: The test currently requires finding a completed VOD file
within a hardcoded VOD_TIMEOUT=180 loop (variables VOD_ELAPSED, VOD_FILE) which
is brittle for long VODs; make the timeout configurable (e.g., via env
VOD_TIMEOUT or test flag) and change the success condition to detect that a
download has started by accepting a non-zero-sized/increasing file (check file
size growth across iterations) rather than only a fully completed file; update
the loop that uses VOD_ELAPSED/VOD_FILE to read the configurable timeout and to
treat a found file with size >0 and observed growth between checks as success so
Phase 5 passes earlier while full completion can be validated later (this aligns
with the flow in streamdl.go and download_stream.go).
🪄 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: c24c5dc7-7450-43ed-a356-9d974f181bc8
📒 Files selected for processing (1)
tests/integration/run.sh
Accept in-progress downloads (>1KB in /incomplete) as proof the pipeline works, not just completed files. Makes the test faster and less flaky for long VODs. VOD_TIMEOUT is now configurable via env var.
Iterate a prioritized list of channels (teampgp first) and probe for published VODs using yt-dlp before running Phase 5. Skip gracefully if no candidates have VODs. Overridable via VOD_CHANNEL env var.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/run.sh (2)
275-275: Quote variable in numeric comparison.Shellcheck recommends quoting
$VOD_TIMEOUTto prevent word splitting, even though the risk is minimal here.🔧 Proposed fix
-while [ $VOD_ELAPSED -lt $VOD_TIMEOUT ]; do +while [ "$VOD_ELAPSED" -lt "$VOD_TIMEOUT" ]; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run.sh` at line 275, The numeric comparison in the while loop uses an unquoted variable $VOD_TIMEOUT which can cause word-splitting or empty-variable issues; update the condition while [ $VOD_ELAPSED -lt $VOD_TIMEOUT ]; do to quote the right-hand operand (e.g., while [ "$VOD_ELAPSED" -lt "$VOD_TIMEOUT" ]; do) and also consider quoting $VOD_ELAPSED to be safe, ensuring both VOD_ELAPSED and VOD_TIMEOUT are treated as single numeric tokens in tests/integration/run.sh.
281-286: Consider whether accepting in-progress downloads provides sufficient validation.The test now passes when it finds a VOD download >1KB in the incomplete directory, without waiting for completion. While this speeds up the test and reduces flakiness from long VODs, it means the integration test doesn't verify that:
- VODs can be downloaded to completion
- The completed file is moved to the correct directory
- The VOD database marks the download as completed
This trade-off prioritizes test speed over coverage. If this is intentional, consider adding a comment explaining the rationale, or alternatively, use a smaller timeout for completed files before falling back to checking in-progress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run.sh` around lines 281 - 286, The current check in tests/integration/run.sh uses VOD_PROGRESS (find in "$OUTPUT_DIR/incomplete" >1KB) and accepts an in-progress file, which skips validating completion, file move and DB state; either (A) add a short wait-and-check sequence before falling back: first poll for a completed VOD in "$OUTPUT_DIR/completed" with a small timeout, verify the final filename exists and then verify the VOD DB record is marked completed, and only if that fails fall back to the existing in-progress check, or (B) if accepting in-progress is intentional, add a clear comment above the VOD_PROGRESS block explaining the trade-off and why the test does not wait for completion (including references to "$OUTPUT_DIR/incomplete", "$OUTPUT_DIR/completed", and the VOD DB completion check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/run.sh`:
- Line 275: The numeric comparison in the while loop uses an unquoted variable
$VOD_TIMEOUT which can cause word-splitting or empty-variable issues; update the
condition while [ $VOD_ELAPSED -lt $VOD_TIMEOUT ]; do to quote the right-hand
operand (e.g., while [ "$VOD_ELAPSED" -lt "$VOD_TIMEOUT" ]; do) and also
consider quoting $VOD_ELAPSED to be safe, ensuring both VOD_ELAPSED and
VOD_TIMEOUT are treated as single numeric tokens in tests/integration/run.sh.
- Around line 281-286: The current check in tests/integration/run.sh uses
VOD_PROGRESS (find in "$OUTPUT_DIR/incomplete" >1KB) and accepts an in-progress
file, which skips validating completion, file move and DB state; either (A) add
a short wait-and-check sequence before falling back: first poll for a completed
VOD in "$OUTPUT_DIR/completed" with a small timeout, verify the final filename
exists and then verify the VOD DB record is marked completed, and only if that
fails fall back to the existing in-progress check, or (B) if accepting
in-progress is intentional, add a clear comment above the VOD_PROGRESS block
explaining the trade-off and why the test does not wait for completion
(including references to "$OUTPUT_DIR/incomplete", "$OUTPUT_DIR/completed", and
the VOD DB completion check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75334b66-8399-4157-b2f5-18a49baa764f
📒 Files selected for processing (1)
tests/integration/run.sh
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/run.sh (1)
343-346: Include server logs in the VOD timeout path.A timeout here can come from server-side VOD enumeration or URL resolution, but this branch only dumps client logs. Mirroring the live-phase diagnostics will make VOD failures much easier to debug.
♻️ Suggested improvement
if [ -z "$VOD_FILE" ] && [ -z "$VOD_PROGRESS" ]; then echo "FAIL: No VOD download activity found after ${VOD_TIMEOUT}s" + echo "" + echo "--- Client logs ---" $DC -f "$COMPOSE_FILE" logs client 2>&1 | tail -30 + echo "" + echo "--- Server logs ---" + $DC -f "$COMPOSE_FILE" logs server 2>&1 | tail -30 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/run.sh` around lines 343 - 346, The timeout branch that checks VOD_FILE and VOD_PROGRESS and currently only dumps client logs should also capture server logs to aid debugging; update the failure path where VOD_TIMEOUT is used (the block that runs $DC -f "$COMPOSE_FILE" logs client ...) to also run $DC -f "$COMPOSE_FILE" logs server (and optionally other relevant services) and include their last lines (e.g., tail -30) so server-side VOD enumeration/URL resolution errors are visible alongside client logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/run.sh`:
- Around line 269-305: The probe loop currently treats every ERROR the same as
NO_VODS and allows the script to exit 0; update the logic around the for-loop
that iterates CANDIDATE_VOD_CHANNELS so it tracks whether any probe completed
successfully (e.g., set a flag like PROBE_SUCCEEDED when RESULT is not "ERROR")
while still setting VOD_CHANNEL when RESULT == "HAS_VODS"; after the loop, if
VOD_CHANNEL is empty but PROBE_SUCCEEDED is false, fail the test (exit non-zero)
to surface probe failures, otherwise preserve the existing skip path when probes
ran but returned only NO_VODS.
---
Nitpick comments:
In `@tests/integration/run.sh`:
- Around line 343-346: The timeout branch that checks VOD_FILE and VOD_PROGRESS
and currently only dumps client logs should also capture server logs to aid
debugging; update the failure path where VOD_TIMEOUT is used (the block that
runs $DC -f "$COMPOSE_FILE" logs client ...) to also run $DC -f "$COMPOSE_FILE"
logs server (and optionally other relevant services) and include their last
lines (e.g., tail -30) so server-side VOD enumeration/URL resolution errors are
visible alongside client logs.
🪄 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: 407c521e-8449-4e1a-99b3-bf59dd1843bc
📒 Files selected for processing (1)
tests/integration/run.sh
Include server logs alongside client logs when VOD download times out. Fail the test if all VOD probes errored (connectivity issue) rather than skipping gracefully, which is reserved for when probes succeed but no channels have VODs.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 tasks
This was referenced Apr 14, 2026
4 tasks
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.
Fixes #231
Summary
vod: trueconfig toggle to download Twitch past broadcasts instead of live streamsGetVodsgRPC RPC enumerates VODs via yt-dlp; existingGetStreamresolves each VOD URL through Streamlink/yt-dlp-vod-out/-vod-moveflags allow routing VOD downloads to different directories than live streams{user}_vod_{id}_{title}.mp4naming with stream copy (no re-encoding)Implementation Tasks
vodandvod_limitfields to Streamer structGetVodsRPC,VodRequest,VodResponse,VodInfomessagesGetVodshandler using yt-dlp for enumerationgetVods()functiondownloadVOD()with VOD-specific naming and DB tracking/app/datavolume for SQLite persistenceTest plan
TestParseConfig_VODFields— VOD config parsingTestVodDB_InitAndClose— DB creationTestVodDB_FullLifecycle— started → completed lifecycleTestVodDB_FailedVODIsRetried— failed VODs are retriedTestVodDB_StaleDownloadIsRetried— stale downloads detected and retriedTestVodDB_DifferentVODsAreIndependent— VOD isolationtests/integration/run.shvod: trueand verify VODs download and are tracked in DBSummary by CodeRabbit
New Features
vod: true) with optionalvod_limit-data,-vod-out,-vod-moveto persist and separate VOD output/move pathsDocumentation
Tests