Footer source and licence credit - #9
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (47)
💤 Files with no reviewable changes (11)
📝 WalkthroughWalkthroughThe change adds configurable source credits, featured-channel selection, promoted-show links, and a durable HLS archive uploader. It removes the legacy MP4 DVR pipeline, raises the PHP baseline to 8.5, and updates related deployment and regression tests. ChangesConfigurable source credits
Featured channels and promoted shows
HLS archive migration
Platform and regression updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ShowPlayer
participant StreamController
participant Source
participant StatusPage
ShowPlayer->>StreamController: request unavailable show page
StreamController->>Source: resolve featured source
StreamController->>StreamController: select accessible promoted show
StreamController->>ShowPlayer: return promoted payload
ShowPlayer->>StatusPage: pass promoted payload
StatusPage->>ShowPlayer: render promoted destination link
sequenceDiagram
participant HLSPlaylists
participant ArchiveUploader
participant SQLiteManifest
participant S3Archive
HLSPlaylists->>ArchiveUploader: discover completed HLS segments
ArchiveUploader->>SQLiteManifest: persist segment and upload state
ArchiveUploader->>S3Archive: upload and verify segments
ArchiveUploader->>S3Archive: upload hourly archive indexes
ArchiveUploader->>HLSPlaylists: reap verified expired segments
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
resources/js/composables/usePromotedShow.js (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named route helper instead of a hardcoded path.
promotedUrlbuilds the URL with a hardcoded template string.ShowEndedStatusPage.vueandShowCancelledStatusPage.vuebuild the equivalent "other show" links withroute('show.view', liveShow.slug). Use the same named route here so the URL stays in sync if the route path ever changes, and so slug values get the same encoding Ziggy applies elsewhere.♻️ Proposed fix to use the named route
const promotedUrl = computed(() => - promoted.value?.slug ? `/show/${promoted.value.slug}` : fallbackUrl, + promoted.value?.slug ? route('show.view', promoted.value.slug) : fallbackUrl, );Since this touches how the Ziggy
route()helper is used outside a Vue component context, please confirmroute()is available as a global at the point this composable runs (it should be, given the same pattern is already relied on inShowPlayer.vue).🤖 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 `@resources/js/composables/usePromotedShow.js` around lines 16 - 18, Update promotedUrl in usePromotedShow to generate the promoted show URL with the global named route helper using the show.view route and promoted.value.slug, while retaining fallbackUrl when no slug exists. Confirm the helper is available in this composable’s execution context, following the existing usage pattern in ShowPlayer.vue.docker/archive-uploader/archive_uploader.py (1)
225-230: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
reapabledeterministic; the unordered LIMIT can starve the reaper.
reapablereturns any verified row, including rows still well insideDVR_WINDOW_SECONDS. Those rows consume the 2000-row limit.reapthen filters by mtime and skips them. Row order is unspecified withoutORDER BY, so the rows that are actually reapable can be excluded on every pass while the disk fills. Order by the oldest verification and exclude rows that cannot yet be reaped.♻️ Proposed query change
- def reapable(self, limit=2000): - with self._conn() as c: - return c.execute( - 'SELECT path, key, size FROM segments WHERE verified_at IS NOT NULL ' - 'LIMIT ?', (limit,) - ).fetchall() + def reapable(self, limit=2000): + # verified_at is set when the copy landed, so anything verified inside the + # window cannot be past it either. Cheap pre-filter before the mtime check. + cutoff = time.time() - DVR_WINDOW_SECONDS + with self._conn() as c: + return c.execute( + 'SELECT path, key, size FROM segments WHERE verified_at IS NOT NULL ' + 'AND verified_at < ? ORDER BY verified_at LIMIT ?', (cutoff, limit) + ).fetchall()Add a matching index if the table grows:
CREATE INDEX ... ON segments (verified_at)already exists.🤖 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 `@docker/archive-uploader/archive_uploader.py` around lines 225 - 230, Update reapable to return only verified segments eligible for reaping, using the existing DVR_WINDOW_SECONDS cutoff, and order results by oldest verified_at before applying the limit so eligible rows cannot be starved. Reuse the existing segments(verified_at) index; do not add another index.resources/views/server-provisioning/origin/srs-config.blade.php (1)
36-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale
on_dvrentries. The route is no longer shipped, and SRS does not invokeon_dvrwhiledvr { enabled off; }is active. Remove the entry from all three configurations to keep the SRS configuration aligned with the available routes.🤖 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 `@resources/views/server-provisioning/origin/srs-config.blade.php` around lines 36 - 45, Remove the stale on_dvr entries from the DVR configuration in resources/views/server-provisioning/origin/srs-config.blade.php (lines 36-45), docker/origin-srs/origin.conf (lines 40-49), and docker/dev/origin-srs.conf (lines 42-51); leave the dvr enabled off settings unchanged.docker/archive-uploader/Dockerfile (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin exact AWS SDK versions.
boto3==1.40.*still permits differentboto3andbotocorepatch releases. Use a checked-in constraints file with tested exact versions forboto3,botocore,s3transfer, andjmespath.🤖 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 `@docker/archive-uploader/Dockerfile` at line 5, Update the Dockerfile’s boto3 installation to use a checked-in constraints file that pins exact tested versions of boto3, botocore, s3transfer, and jmespath; replace the unconstrained install in the RUN command while preserving no-cache installation 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 `@app/Http/Controllers/StreamController.php`:
- Around line 626-627: Update the `promoted` prop construction in
`StreamController::show()` to call `resolvePromotedShow($user, $show)` only when
the show status is not `live`; return the existing empty/null-equivalent value
for live shows so the live-player path avoids all promoted-show queries while
scheduled, ended, and cancelled shows retain their current behavior.
In `@app/Models/Source.php`:
- Around line 65-70: Update app/Models/Source.php lines 65-70 around the saved
hook so selecting a featured source and demoting all other featured sources
occur within one transaction while locking the relevant rows, preventing
concurrent promotions from leaving no explicit featured source. Update
database/migrations/2026_08_03_030000_add_is_featured_to_sources_table.php lines
23-36 to add an at-most-one-featured database constraint supported by the
deployment database.
In `@config/stream.php`:
- Line 70: Update the environment variable name in the `.env.example` template
from `STREAM_IMAGE_DVR_UPLOADER` to `STREAM_IMAGE_ARCHIVE_UPLOADER`, matching
the `archive_uploader` configuration key and preventing operators from setting
an ignored variable.
In `@docker/archive-uploader/archive_uploader.py`:
- Around line 9-10: Update the module docstring in archive_uploader.py to remove
the obsolete reference to uploader.py and the SRS MP4 DVR cold backup,
accurately stating that the segment archive is the only copy as documented in
docs/dvr-archive-plan.md.
- Around line 518-519: Update the zero-size handling in the surrounding
segment-processing flow so it logs the condition and removes the corresponding
pending row instead of returning immediately. Ensure the deletion updates the
same record used by the sweep and lets pending_count converge, while preserving
the existing behavior for non-empty segments.
- Around line 632-640: Update the pending-upload loop over playlists so each
rendition’s key uses the canonical entry’s hour, matching the hour stored by
Indexer.add, rather than entry.hour from the current rendition. Remove the
unused rendition loop variable while preserving per-entry path checks and
manifest.record_pending behavior.
- Around line 642-645: Replace the per-row Thread creation in the sweep around
manifest.pending_uploads() with a bounded worker pool, and maintain an in-flight
set keyed by path (or equivalent upload identity) so queued or active uploads
are skipped on subsequent sweeps and cannot be submitted twice. Remove the
now-redundant upload_semaphore handling inside upload_segment, and ensure
completed or failed tasks clear their in-flight entries while preserving
existing upload behavior.
In `@resources/js/Pages/Manage/Sources/Form.vue`:
- Around line 139-145: Initialize the edit form’s is_featured field from
props.source.is_featured in the useForm setup, ensuring existing featured state
is submitted unchanged when the checkbox is not modified while preserving the
create form’s default.
In `@tests/Feature/SrsWebhookAuthenticationTest.php`:
- Around line 82-88: Create a `Source` fixture with slug `livestream` in
`SrsWebhookAuthenticationTest` for source-key authentication scenarios, ensuring
requests reach key and permission validation. Remove the obsolete user-key test
cases and related requests, since publisher authentication no longer uses
`User::streamkey`; preserve coverage for valid and invalid source keys and
assignment/permission conditions.
---
Nitpick comments:
In `@docker/archive-uploader/archive_uploader.py`:
- Around line 225-230: Update reapable to return only verified segments eligible
for reaping, using the existing DVR_WINDOW_SECONDS cutoff, and order results by
oldest verified_at before applying the limit so eligible rows cannot be starved.
Reuse the existing segments(verified_at) index; do not add another index.
In `@docker/archive-uploader/Dockerfile`:
- Line 5: Update the Dockerfile’s boto3 installation to use a checked-in
constraints file that pins exact tested versions of boto3, botocore, s3transfer,
and jmespath; replace the unconstrained install in the RUN command while
preserving no-cache installation behavior.
In `@resources/js/composables/usePromotedShow.js`:
- Around line 16-18: Update promotedUrl in usePromotedShow to generate the
promoted show URL with the global named route helper using the show.view route
and promoted.value.slug, while retaining fallbackUrl when no slug exists.
Confirm the helper is available in this composable’s execution context,
following the existing usage pattern in ShowPlayer.vue.
In `@resources/views/server-provisioning/origin/srs-config.blade.php`:
- Around line 36-45: Remove the stale on_dvr entries from the DVR configuration
in resources/views/server-provisioning/origin/srs-config.blade.php (lines
36-45), docker/origin-srs/origin.conf (lines 40-49), and
docker/dev/origin-srs.conf (lines 42-51); leave the dvr enabled off settings
unchanged.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2405d9b-62bc-4a86-bced-c102663fc9b8
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
.env.dvr-process.example.github/workflows/laravel.ymlDockerfileapp/Console/Commands/ExtractDvrSegments.phpapp/Http/Controllers/Api/SrsDvrController.phpapp/Http/Controllers/Manage/SourceController.phpapp/Http/Controllers/ScheduleController.phpapp/Http/Controllers/StreamController.phpapp/Http/Requests/Manage/SourceRequest.phpapp/Models/Source.phpapp/Services/DvrExtractorService.phpcomposer.jsonconfig/stream.phpdatabase/migrations/2026_08_03_030000_add_is_featured_to_sources_table.phpdocker-compose.dev.ymldocker/archive-uploader/Dockerfiledocker/archive-uploader/archive_uploader.pydocker/dev/origin-srs.confdocker/dvr-uploader/Dockerfiledocker/dvr-uploader/uploader.pydocker/origin-srs/origin.confdocs/dev-stack.mddocs/dvr-archive-plan.mddvr-extract.shdvr-process.shresources/js/Components/Livestream/StatusPages/ShowCancelledStatusPage.vueresources/js/Components/Livestream/StatusPages/ShowEndedStatusPage.vueresources/js/Components/Livestream/StatusPages/ShowScheduledStatusPage.vueresources/js/Layouts/AuthenticatedLayout.vueresources/js/Pages/Manage/Sources/Form.vueresources/js/Pages/ShowPlayer.vueresources/js/composables/usePromotedShow.jsresources/views/server-provisioning/origin/docker-compose.blade.phpresources/views/server-provisioning/origin/srs-config.blade.phproutes/api.phpscripts/dev-stack.shtests/Feature/Api/CommandControllerTest.phptests/Feature/Api/SrsCallbackControllerTest.phptests/Feature/AutoModeEndToEndTest.phptests/Feature/InertiaComponentTest.phptests/Feature/Manage/ShowsTest.phptests/Feature/Manage/SourcesTest.phptests/Feature/PageLoadTest.phptests/Feature/PromotedShowTest.phptests/Feature/SrsWebhookAuthenticationTest.phptests/Feature/StreamReconnectingFlowTest.phptests/Unit/Commands/CommandSystemTest.php
💤 Files with no reviewable changes (11)
- dvr-extract.sh
- tests/Feature/Api/CommandControllerTest.php
- .env.dvr-process.example
- app/Http/Controllers/Api/SrsDvrController.php
- dvr-process.sh
- app/Services/DvrExtractorService.php
- routes/api.php
- docker/dvr-uploader/uploader.py
- app/Console/Commands/ExtractDvrSegments.php
- docker/dvr-uploader/Dockerfile
- resources/js/Layouts/AuthenticatedLayout.vue
| // Somewhere to go when this show is not watchable. See resolvePromotedShow(). | ||
| 'promoted' => $this->resolvePromotedShow($user, $show), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid running resolvePromotedShow() for live shows.
resolvePromotedShow() runs up to 3 database queries. The show() action calls it unconditionally for every show status, including live. resources/js/Pages/ShowPlayer.vue only passes :promoted to ShowScheduledStatusPage, ShowEndedStatusPage, and ShowCancelledStatusPage. The live-player branch never uses promoted. The method's own docblock states its purpose: point a viewer somewhere "when the show they opened is not watchable." A live, watchable show does not need this data.
Live shows are the primary traffic path for this application. Running 3 unnecessary queries on every live-show page view adds avoidable database load on the hottest path.
Gate the call on the show status.
⚡ Proposed fix to skip promoted-show resolution for live shows
'availableShows' => $availableShows,
// Somewhere to go when this show is not watchable. See resolvePromotedShow().
- 'promoted' => $this->resolvePromotedShow($user, $show),
+ 'promoted' => $show->status !== 'live' ? $this->resolvePromotedShow($user, $show) : null,📝 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.
| // Somewhere to go when this show is not watchable. See resolvePromotedShow(). | |
| 'promoted' => $this->resolvePromotedShow($user, $show), | |
| // Somewhere to go when this show is not watchable. See resolvePromotedShow(). | |
| 'promoted' => $show->status !== 'live' ? $this->resolvePromotedShow($user, $show) : null, |
🤖 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 `@app/Http/Controllers/StreamController.php` around lines 626 - 627, Update the
`promoted` prop construction in `StreamController::show()` to call
`resolvePromotedShow($user, $show)` only when the show status is not `live`;
return the existing empty/null-equivalent value for live shows so the
live-player path avoids all promoted-show queries while scheduled, ended, and
cancelled shows retain their current behavior.
| static::saved(function ($source) { | ||
| if ($source->is_featured && $source->wasChanged('is_featured')) { | ||
| static::where('id', '!=', $source->id) | ||
| ->where('is_featured', true) | ||
| ->update(['is_featured' => false]); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make featured-source promotion atomic.
The post-save hook performs demotion after the selected source update commits. Concurrent requests can each demote the other selected source, leaving no explicit featured source. The application then promotes the priority fallback instead of either administrator’s selection.
app/Models/Source.php#L65-L70: move promote-and-demote into one transaction that locks the relevant source rows.database/migrations/2026_08_03_030000_add_is_featured_to_sources_table.php#L23-L36: add a database-level at-most-one-featured safeguard supported by the deployment database.
🧰 Tools
🪛 PHPStan (2.2.6)
[error] 67-67: Call to an undefined static method App\Models\Source::where().
(staticMethod.notFound)
📍 Affects 2 files
app/Models/Source.php#L65-L70(this comment)database/migrations/2026_08_03_030000_add_is_featured_to_sources_table.php#L23-L36
🤖 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 `@app/Models/Source.php` around lines 65 - 70, Update app/Models/Source.php
lines 65-70 around the saved hook so selecting a featured source and demoting
all other featured sources occur within one transaction while locking the
relevant rows, preventing concurrent promotions from leaving no explicit
featured source. Update
database/migrations/2026_08_03_030000_add_is_featured_to_sources_table.php lines
23-36 to add an at-most-one-featured database constraint supported by the
deployment database.
| 'images' => [ | ||
| 'ffmpeg_hls' => env('STREAM_IMAGE_FFMPEG_HLS', 'ffmpeg-hls:latest'), | ||
| 'dvr_uploader' => env('STREAM_IMAGE_DVR_UPLOADER', 'dvr-uploader:latest'), | ||
| 'archive_uploader' => env('STREAM_IMAGE_ARCHIVE_UPLOADER', 'archive-uploader:latest'), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find stale references to the renamed image key and env variable.
set -uo pipefail
echo '--- old config key ---'
rg -n "stream\.images\.dvr_uploader|images'\]\['dvr_uploader|dvr_uploader" \
-g '!**/node_modules/**' -g '!**/vendor/**' || echo 'none'
echo '--- old env name ---'
rg -n 'STREAM_IMAGE_DVR_UPLOADER' -g '!**/node_modules/**' -g '!**/vendor/**' || echo 'none'
echo '--- new key readers ---'
rg -n 'stream\.images\.archive_uploader|STREAM_IMAGE_ARCHIVE_UPLOADER' \
-g '!**/node_modules/**' -g '!**/vendor/**'
echo '--- env templates ---'
fd -H -t f '^\.env' -x sh -c 'echo "== {}"; rg -n "STREAM_IMAGE" {} || true'Repository: Thiritin/streaming
Length of output: 184
🏁 Script executed:
set -uo pipefail
echo '--- config/stream.php ---'
sed -n '1,130p' config/stream.php
echo '--- all relevant references ---'
rg -n -i \
'dvr_uploader|archive_uploader|STREAM_IMAGE_(DVR|ARCHIVE)_UPLOADER|stream\.images' \
-g '!**/node_modules/**' -g '!**/vendor/**' . || true
echo '--- environment templates and compose-related files ---'
fd -H -t f '^\.env' . -x sh -c 'echo "== $1"; rg -n "STREAM_IMAGE_(DVR|ARCHIVE)_UPLOADER|STREAM_IMAGE" "$1" || true' sh {}
fd -H -t f -i '.*(compose|docker).*' . -x sh -c 'echo "== $1"; rg -n "dvr_uploader|archive_uploader|STREAM_IMAGE_(DVR|ARCHIVE)_UPLOADER|stream\.images" "$1" || true' sh {}
echo '--- config key access patterns ---'
rg -n -U \
"(config|env)\([^)]*(stream\.images|STREAM_IMAGE_(DVR|ARCHIVE)_UPLOADER)|\[['\"](dvr_uploader|archive_uploader)['\"]\]" \
-g '!**/node_modules/**' -g '!**/vendor/**' . || trueRepository: Thiritin/streaming
Length of output: 8232
🏁 Script executed:
set -uo pipefail
echo '--- .env.example image settings ---'
sed -n '138,153p' .env.example
echo '--- provisioning template context ---'
sed -n '76,91p' resources/views/server-provisioning/origin/docker-compose.blade.php
echo '--- repository changes ---'
git diff --stat
git status --short
echo '--- old-name occurrences excluding historical documentation ---'
rg -n 'STREAM_IMAGE_DVR_UPLOADER|stream\.images\.dvr_uploader|dvr_uploader' \
-g '!docs/**' -g '!**/node_modules/**' -g '!**/vendor/**' . || trueRepository: Thiritin/streaming
Length of output: 1376
Update the environment template.
.env.example:148 still uses STREAM_IMAGE_DVR_UPLOADER. Replace it with STREAM_IMAGE_ARCHIVE_UPLOADER so operators do not configure an ignored variable.
🤖 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 `@config/stream.php` at line 70, Update the environment variable name in the
`.env.example` template from `STREAM_IMAGE_DVR_UPLOADER` to
`STREAM_IMAGE_ARCHIVE_UPLOADER`, matching the `archive_uploader` configuration
key and preventing operators from setting an ignored variable.
| <FormField | ||
| v-model="form.is_featured" | ||
| label="Featured channel" | ||
| type="checkbox" | ||
| :error="form.errors.is_featured" | ||
| helper="Owns the hero on the landing page, and is where an ended show sends viewers. Only one source can be featured; turning this on turns it off elsewhere." | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initialize form.is_featured for edit requests.
The edit branch does not copy props.source.is_featured into useForm(). If an administrator saves a featured source without changing this checkbox, SourceRequest::validated() converts the missing value to false. The update then clears the featured flag.
Proposed fix
? {
name: props.source.name,
priority: props.source.priority,
+ is_featured: props.source.is_featured,
description: props.source.description ?? '',
}🤖 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 `@resources/js/Pages/Manage/Sources/Form.vue` around lines 139 - 145,
Initialize the edit form’s is_featured field from props.source.is_featured in
the useForm setup, ensuring existing featured state is submitted unchanged when
the checkbox is not modified while preserving the create form’s default.
| $response = $this->postJson('/api/srs/auth', [ | ||
| 'app' => 'live', | ||
| 'app' => 'ingress', | ||
| 'stream' => 'livestream', | ||
| 'tcUrl' => 'rtmp://localhost/live', | ||
| 'tcUrl' => 'rtmp://localhost/ingress', | ||
| 'pageUrl' => '', | ||
| 'param' => '?secret=invalid_streamkey_456', | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the intended source-authentication conditions.
No Source with slug livestream exists in this test setup. The authentication requests therefore return 403 before they evaluate the supplied key, user server assignment, or user permission. The unpublish request succeeds for an unknown stream, so it also does not test the user key.
Create a source fixture for source-key cases. Remove the obsolete user-key cases, because publisher authentication no longer uses User::streamkey.
Also applies to: 99-105, 122-127, 149-154, 209-214
🤖 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 `@tests/Feature/SrsWebhookAuthenticationTest.php` around lines 82 - 88, Create
a `Source` fixture with slug `livestream` in `SrsWebhookAuthenticationTest` for
source-key authentication scenarios, ensuring requests reach key and permission
validation. Remove the obsolete user-key test cases and related requests, since
publisher authentication no longer uses `User::streamkey`; preserve coverage for
valid and invalid source keys and assignment/permission conditions.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🧹 Nitpick comments (4)
resources/js/composables/usePromotedShow.js (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named route helper instead of a hardcoded path.
promotedUrlbuilds the URL with a hardcoded template string.ShowEndedStatusPage.vueandShowCancelledStatusPage.vuebuild the equivalent "other show" links withroute('show.view', liveShow.slug). Use the same named route here so the URL stays in sync if the route path ever changes, and so slug values get the same encoding Ziggy applies elsewhere.♻️ Proposed fix to use the named route
const promotedUrl = computed(() => - promoted.value?.slug ? `/show/${promoted.value.slug}` : fallbackUrl, + promoted.value?.slug ? route('show.view', promoted.value.slug) : fallbackUrl, );Since this touches how the Ziggy
route()helper is used outside a Vue component context, please confirmroute()is available as a global at the point this composable runs (it should be, given the same pattern is already relied on inShowPlayer.vue).🤖 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 `@resources/js/composables/usePromotedShow.js` around lines 16 - 18, Update promotedUrl in usePromotedShow to generate the promoted show URL with the global named route helper using the show.view route and promoted.value.slug, while retaining fallbackUrl when no slug exists. Confirm the helper is available in this composable’s execution context, following the existing usage pattern in ShowPlayer.vue.docker/archive-uploader/archive_uploader.py (1)
225-230: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
reapabledeterministic; the unordered LIMIT can starve the reaper.
reapablereturns any verified row, including rows still well insideDVR_WINDOW_SECONDS. Those rows consume the 2000-row limit.reapthen filters by mtime and skips them. Row order is unspecified withoutORDER BY, so the rows that are actually reapable can be excluded on every pass while the disk fills. Order by the oldest verification and exclude rows that cannot yet be reaped.♻️ Proposed query change
- def reapable(self, limit=2000): - with self._conn() as c: - return c.execute( - 'SELECT path, key, size FROM segments WHERE verified_at IS NOT NULL ' - 'LIMIT ?', (limit,) - ).fetchall() + def reapable(self, limit=2000): + # verified_at is set when the copy landed, so anything verified inside the + # window cannot be past it either. Cheap pre-filter before the mtime check. + cutoff = time.time() - DVR_WINDOW_SECONDS + with self._conn() as c: + return c.execute( + 'SELECT path, key, size FROM segments WHERE verified_at IS NOT NULL ' + 'AND verified_at < ? ORDER BY verified_at LIMIT ?', (cutoff, limit) + ).fetchall()Add a matching index if the table grows:
CREATE INDEX ... ON segments (verified_at)already exists.🤖 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 `@docker/archive-uploader/archive_uploader.py` around lines 225 - 230, Update reapable to return only verified segments eligible for reaping, using the existing DVR_WINDOW_SECONDS cutoff, and order results by oldest verified_at before applying the limit so eligible rows cannot be starved. Reuse the existing segments(verified_at) index; do not add another index.resources/views/server-provisioning/origin/srs-config.blade.php (1)
36-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale
on_dvrentries. The route is no longer shipped, and SRS does not invokeon_dvrwhiledvr { enabled off; }is active. Remove the entry from all three configurations to keep the SRS configuration aligned with the available routes.🤖 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 `@resources/views/server-provisioning/origin/srs-config.blade.php` around lines 36 - 45, Remove the stale on_dvr entries from the DVR configuration in resources/views/server-provisioning/origin/srs-config.blade.php (lines 36-45), docker/origin-srs/origin.conf (lines 40-49), and docker/dev/origin-srs.conf (lines 42-51); leave the dvr enabled off settings unchanged.docker/archive-uploader/Dockerfile (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin exact AWS SDK versions.
boto3==1.40.*still permits differentboto3andbotocorepatch releases. Use a checked-in constraints file with tested exact versions forboto3,botocore,s3transfer, andjmespath.🤖 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 `@docker/archive-uploader/Dockerfile` at line 5, Update the Dockerfile’s boto3 installation to use a checked-in constraints file that pins exact tested versions of boto3, botocore, s3transfer, and jmespath; replace the unconstrained install in the RUN command while preserving no-cache installation 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 `@app/Http/Controllers/StreamController.php`:
- Around line 626-627: Update the `promoted` prop construction in
`StreamController::show()` to call `resolvePromotedShow($user, $show)` only when
the show status is not `live`; return the existing empty/null-equivalent value
for live shows so the live-player path avoids all promoted-show queries while
scheduled, ended, and cancelled shows retain their current behavior.
In `@app/Models/Source.php`:
- Around line 65-70: Update app/Models/Source.php lines 65-70 around the saved
hook so selecting a featured source and demoting all other featured sources
occur within one transaction while locking the relevant rows, preventing
concurrent promotions from leaving no explicit featured source. Update
database/migrations/2026_08_03_030000_add_is_featured_to_sources_table.php lines
23-36 to add an at-most-one-featured database constraint supported by the
deployment database.
In `@config/stream.php`:
- Line 70: Update the environment variable name in the `.env.example` template
from `STREAM_IMAGE_DVR_UPLOADER` to `STREAM_IMAGE_ARCHIVE_UPLOADER`, matching
the `archive_uploader` configuration key and preventing operators from setting
an ignored variable.
In `@docker/archive-uploader/archive_uploader.py`:
- Around line 9-10: Update the module docstring in archive_uploader.py to remove
the obsolete reference to uploader.py and the SRS MP4 DVR cold backup,
accurately stating that the segment archive is the only copy as documented in
docs/dvr-archive-plan.md.
- Around line 518-519: Update the zero-size handling in the surrounding
segment-processing flow so it logs the condition and removes the corresponding
pending row instead of returning immediately. Ensure the deletion updates the
same record used by the sweep and lets pending_count converge, while preserving
the existing behavior for non-empty segments.
- Around line 632-640: Update the pending-upload loop over playlists so each
rendition’s key uses the canonical entry’s hour, matching the hour stored by
Indexer.add, rather than entry.hour from the current rendition. Remove the
unused rendition loop variable while preserving per-entry path checks and
manifest.record_pending behavior.
- Around line 642-645: Replace the per-row Thread creation in the sweep around
manifest.pending_uploads() with a bounded worker pool, and maintain an in-flight
set keyed by path (or equivalent upload identity) so queued or active uploads
are skipped on subsequent sweeps and cannot be submitted twice. Remove the
now-redundant upload_semaphore handling inside upload_segment, and ensure
completed or failed tasks clear their in-flight entries while preserving
existing upload behavior.
In `@resources/js/Pages/Manage/Sources/Form.vue`:
- Around line 139-145: Initialize the edit form’s is_featured field from
props.source.is_featured in the useForm setup, ensuring existing featured state
is submitted unchanged when the checkbox is not modified while preserving the
create form’s default.
In `@tests/Feature/SrsWebhookAuthenticationTest.php`:
- Around line 82-88: Create a `Source` fixture with slug `livestream` in
`SrsWebhookAuthenticationTest` for source-key authentication scenarios, ensuring
requests reach key and permission validation. Remove the obsolete user-key test
cases and related requests, since publisher authentication no longer uses
`User::streamkey`; preserve coverage for valid and invalid source keys and
assignment/permission conditions.
---
Nitpick comments:
In `@docker/archive-uploader/archive_uploader.py`:
- Around line 225-230: Update reapable to return only verified segments eligible
for reaping, using the existing DVR_WINDOW_SECONDS cutoff, and order results by
oldest verified_at before applying the limit so eligible rows cannot be starved.
Reuse the existing segments(verified_at) index; do not add another index.
In `@docker/archive-uploader/Dockerfile`:
- Line 5: Update the Dockerfile’s boto3 installation to use a checked-in
constraints file that pins exact tested versions of boto3, botocore, s3transfer,
and jmespath; replace the unconstrained install in the RUN command while
preserving no-cache installation behavior.
In `@resources/js/composables/usePromotedShow.js`:
- Around line 16-18: Update promotedUrl in usePromotedShow to generate the
promoted show URL with the global named route helper using the show.view route
and promoted.value.slug, while retaining fallbackUrl when no slug exists.
Confirm the helper is available in this composable’s execution context,
following the existing usage pattern in ShowPlayer.vue.
In `@resources/views/server-provisioning/origin/srs-config.blade.php`:
- Around line 36-45: Remove the stale on_dvr entries from the DVR configuration
in resources/views/server-provisioning/origin/srs-config.blade.php (lines
36-45), docker/origin-srs/origin.conf (lines 40-49), and
docker/dev/origin-srs.conf (lines 42-51); leave the dvr enabled off settings
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2405d9b-62bc-4a86-bced-c102663fc9b8
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
.env.dvr-process.example.github/workflows/laravel.ymlDockerfileapp/Console/Commands/ExtractDvrSegments.phpapp/Http/Controllers/Api/SrsDvrController.phpapp/Http/Controllers/Manage/SourceController.phpapp/Http/Controllers/ScheduleController.phpapp/Http/Controllers/StreamController.phpapp/Http/Requests/Manage/SourceRequest.phpapp/Models/Source.phpapp/Services/DvrExtractorService.phpcomposer.jsonconfig/stream.phpdatabase/migrations/2026_08_03_030000_add_is_featured_to_sources_table.phpdocker-compose.dev.ymldocker/archive-uploader/Dockerfiledocker/archive-uploader/archive_uploader.pydocker/dev/origin-srs.confdocker/dvr-uploader/Dockerfiledocker/dvr-uploader/uploader.pydocker/origin-srs/origin.confdocs/dev-stack.mddocs/dvr-archive-plan.mddvr-extract.shdvr-process.shresources/js/Components/Livestream/StatusPages/ShowCancelledStatusPage.vueresources/js/Components/Livestream/StatusPages/ShowEndedStatusPage.vueresources/js/Components/Livestream/StatusPages/ShowScheduledStatusPage.vueresources/js/Layouts/AuthenticatedLayout.vueresources/js/Pages/Manage/Sources/Form.vueresources/js/Pages/ShowPlayer.vueresources/js/composables/usePromotedShow.jsresources/views/server-provisioning/origin/docker-compose.blade.phpresources/views/server-provisioning/origin/srs-config.blade.phproutes/api.phpscripts/dev-stack.shtests/Feature/Api/CommandControllerTest.phptests/Feature/Api/SrsCallbackControllerTest.phptests/Feature/AutoModeEndToEndTest.phptests/Feature/InertiaComponentTest.phptests/Feature/Manage/ShowsTest.phptests/Feature/Manage/SourcesTest.phptests/Feature/PageLoadTest.phptests/Feature/PromotedShowTest.phptests/Feature/SrsWebhookAuthenticationTest.phptests/Feature/StreamReconnectingFlowTest.phptests/Unit/Commands/CommandSystemTest.php
💤 Files with no reviewable changes (11)
- dvr-extract.sh
- tests/Feature/Api/CommandControllerTest.php
- .env.dvr-process.example
- app/Http/Controllers/Api/SrsDvrController.php
- dvr-process.sh
- app/Services/DvrExtractorService.php
- routes/api.php
- docker/dvr-uploader/uploader.py
- app/Console/Commands/ExtractDvrSegments.php
- docker/dvr-uploader/Dockerfile
- resources/js/Layouts/AuthenticatedLayout.vue
🛑 Comments failed to post (4)
docker/archive-uploader/archive_uploader.py (4)
9-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale docstring:
uploader.pyno longer exists.Lines 9-10 state that
uploader.pykeeps handling the SRS MP4 DVR as a cold backup. This PR removes that uploader, its compose service, and the SRSdvrblock.docs/dvr-archive-plan.mdnow states the segment archive is the only copy. An operator reading this header would assume a second copy exists.📝 Proposed docstring correction
-This runs alongside uploader.py, which keeps handling the SRS MP4 DVR as a cold -backup. The two watch different volumes and share nothing but the image. +This is the only recording path. SRS DVR is off and the MP4 uploader is gone, so a +segment that never reaches S3 is lost. See docs/dvr-archive-plan.md.📝 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.This is the only recording path. SRS DVR is off and the MP4 uploader is gone, so a segment that never reaches S3 is lost. See docs/dvr-archive-plan.md.🤖 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 `@docker/archive-uploader/archive_uploader.py` around lines 9 - 10, Update the module docstring in archive_uploader.py to remove the obsolete reference to uploader.py and the SRS MP4 DVR cold backup, accurately stating that the segment archive is the only copy as documented in docs/dvr-archive-plan.md.
518-519: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A zero-byte segment stays pending forever and is silently retried.
The playlist already declared this segment complete, so a zero length file will not grow. The row keeps
verified_at IS NULL, so every sweep dispatches another upload for it, andpending_countnever returns to zero. That gives the backlog guard inreporta permanent non-zero floor. No log records the condition.Log it and drop the row so the sweep converges.
🐛 Proposed fix
size = local.stat().st_size if size == 0: + # Complete per the playlist, yet empty: the transcoder produced + # nothing for it. Retrying forever would keep it in the backlog. + logger.warning('Zero-length segment, not uploading: %s', path) + manifest.forget(path) return📝 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.if size == 0: # Complete per the playlist, yet empty: the transcoder produced # nothing for it. Retrying forever would keep it in the backlog. logger.warning('Zero-length segment, not uploading: %s', path) manifest.forget(path) return🤖 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 `@docker/archive-uploader/archive_uploader.py` around lines 518 - 519, Update the zero-size handling in the surrounding segment-processing flow so it logs the condition and removes the corresponding pending row instead of returning immediately. Ensure the deletion updates the same record used by the sweep and lets pending_count converge, while preserving the existing behavior for non-empty segments.
632-640: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The S3 prefix for non-canonical renditions can disagree with the index entry.
The key uses each rendition's own
entry.hour, which comes from that rendition's PDT.Indexer.addwrites one entry per logical segment into the hour directory of the canonical entry and stores only{source}_%v_{session}_{n:06d}.ts. A consumer substitutes%vand keeps the index directory as the hour.Renditions are aligned on cut boundaries, not on identical PDT microseconds. At an hour boundary a non-canonical rendition can report the next hour. The bytes then land under
.../16/while the index entry sits in the hour 15 playlist, and the resolved segment URL points at a key that does not exist. Nothing detects it, becauseassert_renditions_alignedcompares only(session, n).Derive the hour from the canonical entry for every rendition. This also removes the unused
renditionloop variable that Ruff B007 reports.🐛 Proposed fix
# Every rendition's bytes still have to be uploaded individually, even though # one index entry covers all of them. - for rendition, entries in playlists.items(): + # The hour comes from the canonical entry, never from the rendition's own PDT: + # the index entry stores %v against the canonical hour, so a rendition whose + # PDT crosses the boundary first must still be filed under the same hour. + hours = {(e.session, e.n): e.hour for e in canonical} + for entries in playlists.values(): for entry in entries: path = Path(HLS_PATH) / entry.name if manifest.known(path): continue - key = f'{ARCHIVE_PREFIX}/{source}/{entry.hour}/{entry.name}' + hour = hours.get((entry.session, entry.n)) + if hour is None: + continue # not indexed yet; a later sweep picks it up + key = f'{ARCHIVE_PREFIX}/{source}/{hour}/{entry.name}' manifest.record_pending(path, key, source)📝 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.# Every rendition's bytes still have to be uploaded individually, even though # one index entry covers all of them. # The hour comes from the canonical entry, never from the rendition's own PDT: # the index entry stores %v against the canonical hour, so a rendition whose # PDT crosses the boundary first must still be filed under the same hour. hours = {(e.session, e.n): e.hour for e in canonical} for entries in playlists.values(): for entry in entries: path = Path(HLS_PATH) / entry.name if manifest.known(path): continue hour = hours.get((entry.session, entry.n)) if hour is None: continue # not indexed yet; a later sweep picks it up key = f'{ARCHIVE_PREFIX}/{source}/{hour}/{entry.name}' manifest.record_pending(path, key, source)🧰 Tools
🪛 Ruff (0.16.0)
[warning] 634-634: Loop control variable
renditionnot used within loop body(B007)
🤖 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 `@docker/archive-uploader/archive_uploader.py` around lines 632 - 640, Update the pending-upload loop over playlists so each rendition’s key uses the canonical entry’s hour, matching the hour stored by Indexer.add, rather than entry.hour from the current rendition. Remove the unused rendition loop variable while preserving per-entry path checks and manifest.record_pending behavior.Source: Linters/SAST tools
642-645: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unbounded thread creation and duplicate uploads on every sweep.
pending_uploadsreturns every row withverified_at IS NULL, which includes rows whose upload is already in flight or queued onupload_semaphore. The semaphore bounds concurrency, not thread creation. With a 500-row backlog, the sweep creates 500 threads everySWEEP_INTERVAL(5s), so about 6000 threads per minute. Each thread blocks on the semaphore, uploads an object another thread is already uploading, and opens its own thread-local sqlite connection inManifest._connthat is never closed.This fails precisely when the backlog grows, which is the condition
reportexists to warn about. Use a bounded pool and track in-flight paths.🔒️ Proposed fix
+from concurrent.futures import ThreadPoolExecutor + +upload_pool = ThreadPoolExecutor(max_workers=MAX_CONCURRENT_UPLOADS, + thread_name_prefix='upload') +in_flight = set() +in_flight_lock = threading.Lock() + + +def _upload_tracked(manifest, path, key): + try: + upload_segment(manifest, path, key) + finally: + with in_flight_lock: + in_flight.discard(path)- for path, key, _ in manifest.pending_uploads(): - threading.Thread( - target=upload_segment, args=(manifest, path, key), daemon=True - ).start() + for path, key, _ in manifest.pending_uploads(): + with in_flight_lock: + if path in in_flight: + continue + in_flight.add(path) + upload_pool.submit(_upload_tracked, manifest, path, key)With a pool,
upload_semaphoreinsideupload_segmentbecomes redundant and can be dropped.📝 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.from concurrent.futures import ThreadPoolExecutor upload_pool = ThreadPoolExecutor( max_workers=MAX_CONCURRENT_UPLOADS, thread_name_prefix="upload" ) in_flight = set() in_flight_lock = threading.Lock() def _upload_tracked(manifest, path, key): try: upload_segment(manifest, path, key) finally: with in_flight_lock: in_flight.discard(path) for path, key, _ in manifest.pending_uploads(): with in_flight_lock: if path in in_flight: continue in_flight.add(path) upload_pool.submit(_upload_tracked, manifest, path, key)🤖 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 `@docker/archive-uploader/archive_uploader.py` around lines 642 - 645, Replace the per-row Thread creation in the sweep around manifest.pending_uploads() with a bounded worker pool, and maintain an in-flight set keyed by path (or equivalent upload identity) so queued or active uploads are skipped on subsequent sweeps and cannot be submitted twice. Remove the now-redundant upload_semaphore handling inside upload_segment, and ensure completed or failed tasks clear their in-flight entries while preserving existing upload behavior.Source: Linters/SAST tools
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 6 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 6 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
togglefield type for the settings registry, stored as 1 or 0, deleted when it matches the shipped defaultSummary by CodeRabbit
New Features
Bug Fixes
Chores