fix: resolve ffmpeg per platform instead of assuming latest has it - #78
Conversation
The installer read /releases/latest and required it to carry the running platform's assets. That holds only while every release publishes every platform, which stopped being true: a new FFmpeg major now ships Linux-first because the Windows binaries are mirrored from a source that trails upstream by weeks, so n9.0 has linux64 and linuxarm64 and nothing else. On Windows that made DownloadVerifiedAsync throw "missing asset ffmpeg-win64.exe". The auto-update path caught it and kept the current binary, but the install path did not, so a fresh Windows install got no ffmpeg at all rather than falling back to the newest release that has it. Resolution now runs against the release list: SelectForAsset takes the newest release carrying both binaries and SHA256SUMS for the platform in hand. Windows lands on n8.1.2 while Linux takes n9.0. Ordering is by parsed tag rather than list position so it does not depend on GitHub's ordering, and drafts and prereleases are filtered because /releases/latest excluded those implicitly.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFFmpeg release lookup now reads the releases collection, filters ineligible entries, and selects the newest release with required platform binaries and ChangesFFmpeg asset-aware release resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FFmpegInstaller
participant GitHubReleasesAPI
participant FFmpegRelease
FFmpegInstaller->>GitHubReleasesAPI: Request releases collection
GitHubReleasesAPI-->>FFmpegInstaller: Return release JSON
FFmpegInstaller->>FFmpegRelease: ParseReleases and SelectForAsset
FFmpegRelease-->>FFmpegInstaller: Return newest compatible release
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 2
🧹 Nitpick comments (1)
src/Sleezer/Core/Model/FFmpegRelease.cs (1)
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the release-endpoint comment.
This eight-line comment contains release history and operational detail. Keep a one- or two-line comment that states why the code queries the release list.
As per path instructions, comments must be navigational/gotcha only (1-2 lines, non-obvious why).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Core/Model/FFmpegRelease.cs` around lines 22 - 29, Shorten the XML summary comment above the release-list endpoint to one or two lines explaining that querying the list allows each platform to resolve the newest release containing its required assets, rather than assuming the latest release supports every platform. Remove the release history and operational details.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Sleezer/Core/Model/FFmpegRelease.cs`:
- Around line 211-216: Update the release-fetching logic in the method
containing JsonDocument.Parse so blank or non-array responses are treated as
protocol failures and propagated rather than returned as an empty releases list.
In ResolveReleaseAsync and EnsureUpToDateAsync, persist the update timestamp
only after a successful fetch and parse, including valid empty or incompatible
release results; do not persist timestamps when fetching or parsing fails.
- Line 30: Update the release-fetching flow that uses ReleasesApiUrl to follow
GitHub pagination links and collect every release page before filtering. Then
select the highest compatible release from the complete collection, preserving
the existing platform-compatibility checks and version ordering.
---
Nitpick comments:
In `@src/Sleezer/Core/Model/FFmpegRelease.cs`:
- Around line 22-29: Shorten the XML summary comment above the release-list
endpoint to one or two lines explaining that querying the list allows each
platform to resolve the newest release containing its required assets, rather
than assuming the latest release supports every platform. Remove the release
history and operational details.
🪄 Autofix
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: 76c8f6c0-9cbc-4b0d-acb5-245ea74ab838
📒 Files selected for processing (3)
src/Sleezer/Core/Model/FFmpegInstaller.cssrc/Sleezer/Core/Model/FFmpegRelease.cstests/Sleezer.Tests/FFmpegReleaseTests.cs
An error object from the feed is valid JSON, so parsing it produced an empty list, which resolved to null and read exactly like "no release carries this platform". The update path then logged that at Debug and moved on having already stamped the throttle, so a rate-limit reply suppressed checks for 24h with nothing to show for it. A blank or non-array body now throws, which the update path already surfaces as a warning with the exception attached, and the install path reports instead of blaming the platform. A genuinely empty feed still parses to an empty list. per_page also goes to the API maximum, so the fallback has the newest 100 releases to search rather than 30.
|
Addressing the body nitpick ( Agreed and fixed in 75230dd. Eight lines of release history in a doc comment is a straight violation of this repo's own path instruction — comments navigational or gotcha only, 1-2 lines. The provenance detail belongs in the PR body, which is where it now lives. Trimmed to: /// <summary>
/// The list, not <c>/releases/latest</c>: the newest release does not always
/// carry every platform, so each falls back to the newest one that has its assets.
/// </summary>All three findings from this round are triaged — two fixed, one declined with reasoning on its thread (the update-stamp reordering, which trades a benign 24h delay for burning an unauthenticated 60 req/hr budget during exactly the outages it is meant to handle). Build clean, 424/424 tests pass. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Sleezer/Core/Model/FFmpegRelease.cs (1)
199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the
ParseReleasesXML comment.Lines 199-205 restate routine behavior in seven lines. Keep one or two lines that state the non-obvious invalid-feed failure behavior.
As per path instructions, “Comments: navigational/gotcha only (1-2 lines, non-obvious why)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Sleezer/Core/Model/FFmpegRelease.cs` around lines 199 - 205, Shorten the XML comment for ParseReleases to one or two lines focused only on the non-obvious behavior: blank or non-array payloads must throw rather than be treated as an empty release feed. Remove the routine details about filtering drafts/prereleases and skipping individual unparseable entries.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/Sleezer/Core/Model/FFmpegRelease.cs`:
- Around line 199-205: Shorten the XML comment for ParseReleases to one or two
lines focused only on the non-obvious behavior: blank or non-array payloads must
throw rather than be treated as an empty release feed. Remove the routine
details about filtering drafts/prereleases and skipping individual unparseable
entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89223ed7-801a-4fa7-9d87-8dff40ca215e
📒 Files selected for processing (3)
src/Sleezer/Core/Model/FFmpegInstaller.cssrc/Sleezer/Core/Model/FFmpegRelease.cstests/Sleezer.Tests/FFmpegReleaseTests.cs
ParseReleases had grown to seven lines restating what the code already shows. Kept the one non-obvious part, that an invalid body throws rather than reading as an empty feed. SelectForAsset had the same problem at five lines and was not flagged; trimmed it in the same pass to the two whys worth keeping, that SHA256SUMS is required because the download is verified against it and that ordering is by tag because the feed's own order can move.
|
Addressing the full review's nitpick ( Agreed and fixed in fe1f577. Seven lines restating what the code already shows. Kept only the non-obvious part: /// <summary>
/// Drops drafts and prereleases, which <c>/releases/latest</c> excluded implicitly.
/// A blank or non-array body throws: a failed request must not read as an empty feed.
/// </summary>One you did not flag, fixed in the same commit. Rather than fix the one line reported, I audited every doc comment this PR adds. /// <summary>
/// Newest release with both binaries plus <c>SHA256SUMS</c>, which the download is
/// verified against. Ordered by parsed tag, not list position — GitHub's can change.
/// </summary>All four doc comments this PR adds are now at or under the two-line cap. Build clean, 424/424 tests. Worth noting for the record that this round only existed because the incremental pass reported |
1 participant
The break
FFmpegInstallerread/releases/latestand required that release to carry the running platform's assets. That was safe only while every release published every platform — which stopped being true today.chodeus/ffmpeg-staticnow ships a new FFmpeg major Linux-first: its Windows binaries are mirrored from BtbN/FFmpeg-Builds, which only publishes the branches in its own build matrix and adds a new one weeks after upstream tags it. Son9.0is currentlyffmpeg-linux64+ffmpeg-linuxarm64and nothing else.Consequences before this change:
EnsureUpToDateAsyncInstallFFmpeg→DownloadLatestAsyncmissing asset ffmpeg-win64.exe, not caught — a fresh Windows install ends up with no ffmpeg at allThe second is the real defect: the correct answer for Windows was never "fail", it was "use n8.1.2", which is sitting right there in the feed.
The fix
Resolve against the release list rather than the single latest release.
SelectForAssetreturns the newest release carrying both binaries andSHA256SUMSfor the platform in hand — the manifest is included because the download is verified against it, so a release without one is unusable even if the binaries are present.Two details worth calling out:
/releases/latestexcluded those implicitly, and moving to the list endpoint would otherwise have quietly widened what gets installed.Verification
dotnet build ./Sleezer.sln -c Release -p:NuGetAudit=false→ 0 warnings, 0 errors. Tests 423/423 pass.Against the live feed, the new selection resolves:
Linux moves onto the new build; Windows falls back past the Linux-only release instead of throwing.
Eight new cases cover the fallback, both completeness requirements, draft/prerelease filtering, and list-order independence. They are not decoration — removing the asset-presence check makes exactly four of them fail, including
SelectForAsset_windows_falls_back_past_a_linux_only_release:Note
No release is cut without a
release:*label on this PR before merge.Summary by CodeRabbit
Bug Fixes
Tests