Conversation
When a user's policy blocks all available media sources (e.g. "720p Only" but only a Remux exists), instead of blocking playback entirely, force server-side transcoding by disabling DirectPlay/DirectStream. - Add FallbackTranscode bool to QualityPolicy - Add ShouldFallbackTranscode + ApplyFallbackTranscode helpers - Wire fallback into MediaSourceResultFilter (PlaybackInfo, Items, hide logic) - Wire fallback into QualityGateController API endpoint - Allow intro playback when fallback is active - Add UI checkbox in policy config page - 10 new unit tests covering all fallback paths - DenyAllPolicy sentinel is explicitly excluded from fallback - Bump version to 3.3.0.0
|
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 8 minutes and 41 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 per-policy fallback-transcode: new QualityPolicy fields, service helpers to detect/apply fallback, request/result filter changes to force or substitute transcoding when policies block all static sources, controller/provider/UI/config updates, tests, and a version bump. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Owner
Author
Testing Results — Jellyfin Dev (geiserback)Plugin: Quality Gate 3.3.0.0 Test Media (Multiversion library)
FallbackTranscode=OFF (baseline)
FallbackTranscode=ON
Item Visibility (User library listing)All 8 movies visible. Movies without 720p show sources with transcode flags instead of being hidden. Other Users
All 54 unit tests passing. Feature verified end-to-end. |
![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 (5)
Jellyfin.Plugin.QualityGate/Services/QualityGateService.cs (1)
221-228:ApplyFallbackTranscode()mutates the originalMediaSourceInfoobjects.The XML comment says this returns a copy, but it only creates a new array around the same instances after flipping their flags. That makes fallback state leak to any other code still holding those objects. Clone each source before mutating, or rename/document this as an in-place operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate/Services/QualityGateService.cs` around lines 221 - 228, ApplyFallbackTranscode currently mutates the original MediaSourceInfo instances (flipping SupportsDirectPlay/SupportsDirectStream) then returns an array of the same objects; change it to operate on clones so callers don't observe mutated originals: inside ApplyFallbackTranscode, for each source (in the Select) create a copy (e.g. use a MediaSourceInfo.Clone() if available or construct a new MediaSourceInfo and copy relevant properties), then set SupportsDirectPlay = false and SupportsDirectStream = false on that copy and return the copies as an array; alternatively, if cloning is not feasible, rename the method to indicate it mutates in-place and document it accordingly.Jellyfin.Plugin.QualityGate.Tests/GetUserPolicyTests.cs (1)
16-29: Use an isolated temp root for this fixture.
Path.GetTempPath()makes these tests share plugin filesystem state with other runs. IfPluginstarts reading or writing underIApplicationPaths, this becomes order-dependent. Mirror the per-test temp-directory setup used in the other new test classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate.Tests/GetUserPolicyTests.cs` around lines 16 - 29, The test fixture uses Path.GetTempPath() which shares state across runs; change the GetUserPolicyTests constructor to create a unique per-fixture temp root (e.g., via a new GUID-based temp directory), configure the IApplicationPaths mock to return that unique directory instead of Path.GetTempPath() (update the mock setup in the constructor where appPaths is created), and ensure Dispose removes that temp directory (or uses a TempDirectory helper) so the Plugin instance (constructed as new Plugin(...)) reads/writes isolated filesystem state and is cleaned up after the test.Jellyfin.Plugin.QualityGate.Tests/FallbackTranscodeTests.cs (1)
131-163: Add a regression check for input mutation.These assertions still pass if
ApplyFallbackTranscodeflips the flags on the originalMediaSourceInfoinstances. Please lock down whether this API returns copies or mutates in place, because callers may reuse the input collection later in the pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate.Tests/FallbackTranscodeTests.cs` around lines 131 - 163, The tests need a regression check to ensure ApplyFallbackTranscode does not mutate its input array items: after calling QualityGateService.ApplyFallbackTranscode(sources) assert that the original MediaSourceInfo instances in the sources array still have their original SupportsDirectPlay and SupportsDirectStream values (and Path for the ApplyFallback_PreservesPath scenario) to guarantee the API returns copies rather than mutating in place; update the relevant tests (e.g., ApplyFallback_DisablesDirectPlayAndStream, ApplyFallback_PreservesPath, and ApplyFallback_EmptyInput_ReturnsEmpty) to capture the original property values before calling ApplyFallbackTranscode and assert they remain unchanged afterward.Jellyfin.Plugin.QualityGate/Configuration/configPage.js (1)
452-459: Rebuild the plugin before validating this checkbox.This page is shipped as an embedded resource, so the new control will not update in Jellyfin until the DLL is rebuilt and redeployed.
As per coding guidelines
Configuration page is an embedded resource; changes require DLL rebuild.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate/Configuration/configPage.js` around lines 452 - 459, The newly added checkbox control (input with class "policy-fallback-transcode" and id built from "policy-fallback-" + index) is part of the embedded Configuration/configPage.js resource and will not be picked up by Jellyfin until the plugin DLL is rebuilt and redeployed; rebuild the project to produce an updated DLL, redeploy the plugin to your Jellyfin instance, then validate the "Fallback Transcode" checkbox behavior in the UI and confirm the change persists and triggers the expected handling in the code paths that read this setting.Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs (1)
103-118: Don't rescan sources afterfiltered.Length == 0.At this point you've already proven that none of
originalpassedIsSourcePlayable. CallingShouldFallbackTranscode(policy, original)repeats the same path and file checks, and the same extra pass shows up in the later fallback branches too.Suggested simplification
- if (filtered.Length == 0 && original.Count > 0) + var canFallback = filtered.Length == 0 + && original.Count > 0 + && policy.FallbackTranscode + && !ReferenceEquals(policy, QualityGateService.DenyAllPolicy); + + if (filtered.Length == 0 && original.Count > 0) { - if (QualityGateService.ShouldFallbackTranscode(policy, original)) + if (canFallback) { playbackInfo.MediaSources = QualityGateService.ApplyFallbackTranscode(original); _logger.LogInformation(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines 103 - 118, The code rescans sources by calling QualityGateService.ShouldFallbackTranscode(policy, original) after you've already determined filtered.Length == 0 via IsSourcePlayable; remove that redundant rescan by deciding fallback based on policy and the already-evaluated original list. Replace the ShouldFallbackTranscode(policy, original) call with either (a) a policy-only check (e.g., QualityGateService.PolicyAllowsFallback(policy)) or (b) add/use a ShouldFallbackTranscode overload that accepts the precomputed result (so it does not re-run IsSourcePlayable) and then call QualityGateService.ApplyFallbackTranscode(original) or set playbackInfo.ErrorCode accordingly; update MediaSourceResultFilter.cs to reference filtered, original, QualityGateService.ShouldFallbackTranscode/ApplyFallbackTranscode, and IsSourcePlayable when making the decision so no duplicate source checks occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs`:
- Around line 103-118: The code in MediaSourceResultFilter that reinserts
blocked sources (using QualityGateService.ApplyFallbackTranscode when
QualityGateService.ShouldFallbackTranscode returns true) changes the filter from
deny-to-hide into allow-via-transcode; instead, keep the filter fail-closed:
remove the branch that calls ApplyFallbackTranscode and ensure when
filtered.Length == 0 && original.Count > 0 you set playbackInfo.MediaSources to
an empty array (or clear it), set playbackInfo.ErrorCode =
PlaybackErrorCode.NotAllowed, and log the NotAllowed outcome. Apply the same
change to the similar branches referenced in BaseItemDto and ShouldHideItem so
neither uses ApplyFallbackTranscode and all return/leave an empty media sources
list when all sources are blocked.
In `@Jellyfin.Plugin.QualityGate/Services/QualityGateService.cs`:
- Around line 206-214: The current ShouldFallbackTranscode(policy, sources)
enables fallback when no source is playable, but playable check
(IsSourcePlayable) includes File.Exists so fallback wrongly triggers when all
sources are missing; update ShouldFallbackTranscode (referencing
ShouldFallbackTranscode, IsSourcePlayable, MediaSourceInfo,
policy.FallbackTranscode, DenyAllPolicy) to first ensure there is at least one
existing source (e.g., require sourceList.Any(s => File.Exists(s.Path)) or an
equivalent "source exists" helper) and only then evaluate whether none are
playable (keep the existing !sourceList.Any(s => IsSourcePlayable(policy,
s.Path)) logic), returning false if no sources exist.
---
Nitpick comments:
In `@Jellyfin.Plugin.QualityGate.Tests/FallbackTranscodeTests.cs`:
- Around line 131-163: The tests need a regression check to ensure
ApplyFallbackTranscode does not mutate its input array items: after calling
QualityGateService.ApplyFallbackTranscode(sources) assert that the original
MediaSourceInfo instances in the sources array still have their original
SupportsDirectPlay and SupportsDirectStream values (and Path for the
ApplyFallback_PreservesPath scenario) to guarantee the API returns copies rather
than mutating in place; update the relevant tests (e.g.,
ApplyFallback_DisablesDirectPlayAndStream, ApplyFallback_PreservesPath, and
ApplyFallback_EmptyInput_ReturnsEmpty) to capture the original property values
before calling ApplyFallbackTranscode and assert they remain unchanged
afterward.
In `@Jellyfin.Plugin.QualityGate.Tests/GetUserPolicyTests.cs`:
- Around line 16-29: The test fixture uses Path.GetTempPath() which shares state
across runs; change the GetUserPolicyTests constructor to create a unique
per-fixture temp root (e.g., via a new GUID-based temp directory), configure the
IApplicationPaths mock to return that unique directory instead of
Path.GetTempPath() (update the mock setup in the constructor where appPaths is
created), and ensure Dispose removes that temp directory (or uses a
TempDirectory helper) so the Plugin instance (constructed as new Plugin(...))
reads/writes isolated filesystem state and is cleaned up after the test.
In `@Jellyfin.Plugin.QualityGate/Configuration/configPage.js`:
- Around line 452-459: The newly added checkbox control (input with class
"policy-fallback-transcode" and id built from "policy-fallback-" + index) is
part of the embedded Configuration/configPage.js resource and will not be picked
up by Jellyfin until the plugin DLL is rebuilt and redeployed; rebuild the
project to produce an updated DLL, redeploy the plugin to your Jellyfin
instance, then validate the "Fallback Transcode" checkbox behavior in the UI and
confirm the change persists and triggers the expected handling in the code paths
that read this setting.
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs`:
- Around line 103-118: The code rescans sources by calling
QualityGateService.ShouldFallbackTranscode(policy, original) after you've
already determined filtered.Length == 0 via IsSourcePlayable; remove that
redundant rescan by deciding fallback based on policy and the already-evaluated
original list. Replace the ShouldFallbackTranscode(policy, original) call with
either (a) a policy-only check (e.g.,
QualityGateService.PolicyAllowsFallback(policy)) or (b) add/use a
ShouldFallbackTranscode overload that accepts the precomputed result (so it does
not re-run IsSourcePlayable) and then call
QualityGateService.ApplyFallbackTranscode(original) or set
playbackInfo.ErrorCode accordingly; update MediaSourceResultFilter.cs to
reference filtered, original,
QualityGateService.ShouldFallbackTranscode/ApplyFallbackTranscode, and
IsSourcePlayable when making the decision so no duplicate source checks occur.
In `@Jellyfin.Plugin.QualityGate/Services/QualityGateService.cs`:
- Around line 221-228: ApplyFallbackTranscode currently mutates the original
MediaSourceInfo instances (flipping SupportsDirectPlay/SupportsDirectStream)
then returns an array of the same objects; change it to operate on clones so
callers don't observe mutated originals: inside ApplyFallbackTranscode, for each
source (in the Select) create a copy (e.g. use a MediaSourceInfo.Clone() if
available or construct a new MediaSourceInfo and copy relevant properties), then
set SupportsDirectPlay = false and SupportsDirectStream = false on that copy and
return the copies as an array; alternatively, if cloning is not feasible, rename
the method to indicate it mutates in-place and document it accordingly.
🪄 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: e415641a-9cb3-4eee-8679-b4dbb5530879
📒 Files selected for processing (13)
Jellyfin.Plugin.QualityGate.Tests/AssemblyInfo.csJellyfin.Plugin.QualityGate.Tests/FallbackTranscodeTests.csJellyfin.Plugin.QualityGate.Tests/FilterMediaSourcesTests.csJellyfin.Plugin.QualityGate.Tests/GetUserPolicyTests.csJellyfin.Plugin.QualityGate.Tests/Jellyfin.Plugin.QualityGate.Tests.csprojJellyfin.Plugin.QualityGate/Api/QualityGateController.csJellyfin.Plugin.QualityGate/Configuration/PluginConfiguration.csJellyfin.Plugin.QualityGate/Configuration/configPage.htmlJellyfin.Plugin.QualityGate/Configuration/configPage.jsJellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.csJellyfin.Plugin.QualityGate/Jellyfin.Plugin.QualityGate.csprojJellyfin.Plugin.QualityGate/Providers/QualityGateIntroProvider.csJellyfin.Plugin.QualityGate/Services/QualityGateService.cs
Comment on lines
103
to
+118
| if (filtered.Length == 0 && original.Count > 0) | ||
| { | ||
| playbackInfo.ErrorCode = MediaBrowser.Model.Dlna.PlaybackErrorCode.NotAllowed; | ||
| _logger.LogInformation( | ||
| "QualityGate: All sources blocked for user {User} (policy: {Policy}) — returning NotAllowed", | ||
| (object)userId, policy.Name); | ||
| if (QualityGateService.ShouldFallbackTranscode(policy, original)) | ||
| { | ||
| playbackInfo.MediaSources = QualityGateService.ApplyFallbackTranscode(original); | ||
| _logger.LogInformation( | ||
| "QualityGate: Fallback transcode for user {User} (policy: {Policy}) — {Count} sources forced to transcode", | ||
| (object)userId, policy.Name, original.Count); | ||
| } | ||
| else | ||
| { | ||
| playbackInfo.ErrorCode = MediaBrowser.Model.Dlna.PlaybackErrorCode.NotAllowed; | ||
| _logger.LogInformation( | ||
| "QualityGate: All sources blocked for user {User} (policy: {Policy}) — returning NotAllowed", | ||
| (object)userId, policy.Name); | ||
| } |
There was a problem hiding this comment.
Keep the result filter fail-closed.
Re-inserting the blocked sources via ApplyFallbackTranscode(original) changes this filter from “hide/deny” to “allow via transcode”. The same behavior is repeated in the BaseItemDto and ShouldHideItem branches below, so the filter now surfaces items that this layer is supposed to suppress.
As per coding guidelines When all sources are blocked by policy, return an empty array (fail-closed), not a fallback to originals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines
103 - 118, The code in MediaSourceResultFilter that reinserts blocked sources
(using QualityGateService.ApplyFallbackTranscode when
QualityGateService.ShouldFallbackTranscode returns true) changes the filter from
deny-to-hide into allow-via-transcode; instead, keep the filter fail-closed:
remove the branch that calls ApplyFallbackTranscode and ensure when
filtered.Length == 0 && original.Count > 0 you set playbackInfo.MediaSources to
an empty array (or clear it), set playbackInfo.ErrorCode =
PlaybackErrorCode.NotAllowed, and log the NotAllowed outcome. Apply the same
change to the similar branches referenced in BaseItemDto and ShouldHideItem so
neither uses ApplyFallbackTranscode and all return/leave an empty media sources
list when all sources are blocked.
Owner
Author
There was a problem hiding this comment.
Intentional design — this is the whole purpose of the FallbackTranscode feature. When a policy has FallbackTranscode=true and all sources are blocked by filename patterns, the filter intentionally allows playback via server-side transcode instead of denying. The fail-closed sentinel (DenyAllPolicy) is explicitly excluded from fallback via PolicyAllowsFallback(), so misconfiguration still blocks everything. Addressed all other findings in 01354e2.
There was a problem hiding this comment.
@GeiserX, understood — the fallback path is the core feature, not a bypass of the fail-closed rule. PolicyAllowsFallback() correctly gates this on FallbackTranscode=true and excludes DenyAllPolicy, so the sentinel behavior is preserved. My concern was misplaced.
I'll update the long-term memory to reflect this. The existing learning that blanket-requires fail-closed behavior conflicts with the new design:
The prior learning that states "return an empty array (fail-closed), not a fallback to originals" is now partially superseded — it still holds for DenyAllPolicy and any policy with FallbackTranscode=false. Would you like me to remove it (ID: e9f60414-9862-4492-ad32-35757c804b9f) to avoid future false positives on this pattern?
✏️ Learnings added
Learnt from: GeiserX
Repo: GeiserX/quality-gate PR: 13
File: Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs:318-336
Timestamp: 2026-04-14T12:28:07.290Z
Learning: In MediaSourceResultFilter.cs and QualityGateService.cs, when all sources are blocked by policy, the fail-closed rule (return empty array / NotAllowed) has an intentional exception: if the QualityPolicy has FallbackTranscode=true, the filter allows playback via server-side transcode by calling QualityGateService.ApplyFallbackTranscode(). This exception is gated by QualityGateService.PolicyAllowsFallback(), which explicitly excludes DenyAllPolicy (the misconfiguration sentinel) so DenyAllPolicy remains fully fail-closed. This applies to the PlaybackInfoResponse, BaseItemDto, and ShouldHideItem branches in MediaSourceResultFilter.cs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/MediaSourceResultFilter.cs : When all sources are blocked by policy, return an empty array (fail-closed), not a fallback to originals
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/MediaSourceResultFilter.cs : Filter must skip policy enforcement for configured intro video playback paths
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/MediaSourceResultFilter.cs : Filter must handle `PlaybackInfoResponse`, `BaseItemDto`, `QueryResult<BaseItemDto>`, and `IEnumerable<BaseItemDto>` response shapes
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/QualityGateService.cs : Resolve MediaSource paths using both the original path and symlink-resolved path against filename patterns
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to {**/MediaSourceResultFilter.cs,**/QualityGateController.cs} : In the result filter, check userId against `Guid.Empty` when received from API key authentication
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/MediaSourceResultFilter.cs : When DTOs lack MediaSources on library listing endpoints, inject `ILibraryManager` and `IMediaSourceManager` to look up actual sources
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/QualityGateService.cs : Policy resolution must implement fail-closed behavior: return deny-all sentinel when policy is missing, disabled, or deleted
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/QualityGateService.cs : Check both original filename and symlink-resolved filename against blocking and allowing patterns in `IsPathAllowed()`
Learnt from: CR
Repo: GeiserX/quality-gate PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-11T17:07:19.388Z
Learning: Applies to **/MediaSourceResultFilter.cs : In result filter `isRelevant` check, match paths containing both `/Items/` (with slash) and ending with `/Items` (no slash)
The previous approach set SupportsDirectPlay/DirectStream=false on the response, but Jellyfin 10.11 only builds TranscodingUrl when StreamBuilder chooses Transcode via the DeviceProfile — flipping flags leaves TranscodingUrl null so the client has nowhere to get the transcoded stream. Switch from IAsyncActionFilter to IAsyncResourceFilter + IAsyncResultFilter: Phase 1 (ResourceFilter, before model binding): for PlaybackInfo POST requests where fallback is needed, parse the request body and strip DirectPlayProfiles from the DeviceProfile. StreamBuilder then finds no DirectPlay match and falls through to Transcode, building proper TranscodingUrls with HLS parameters. Phase 2 (ResultFilter, before serialization): skip filtering for responses where the pipeline was already forced into transcode mode; apply normal source filtering for all other endpoints. Verified on Jellyfin 10.11.8 dev instance with realistic DeviceProfile: - Movies WITH 720p: direct-play 720p only (unchanged) - Movies WITHOUT 720p + fallback ON: proper TranscodingUrl returned - Admin/full-access users: completely unaffected
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs (1)
244-247: Consider elevating error log level.Logging exceptions at
Debuglevel in the catch-all may hide production issues when fallback logic fails unexpectedly. AWarninglevel would surface these without being noisy for expected edge cases (e.g., non-JSON bodies).♻️ Suggested change
catch (Exception ex) { - _logger.LogDebug(ex, "QualityGate: Error in TryForceTranscodeBodyAsync, skipping"); + _logger.LogWarning(ex, "QualityGate: Error in TryForceTranscodeBodyAsync, skipping"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines 244 - 247, The catch-all in TryForceTranscodeBodyAsync logs exceptions at Debug level which can hide production issues; change the logging call in the catch block of the TryForceTranscodeBodyAsync method in MediaSourceResultFilter from _logger.LogDebug(...) to _logger.LogWarning(...) (preserving the exception and message) so unexpected failures are surfaced without being overly noisy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs`:
- Around line 244-247: The catch-all in TryForceTranscodeBodyAsync logs
exceptions at Debug level which can hide production issues; change the logging
call in the catch block of the TryForceTranscodeBodyAsync method in
MediaSourceResultFilter from _logger.LogDebug(...) to _logger.LogWarning(...)
(preserving the exception and message) so unexpected failures are surfaced
without being overly noisy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cbe00ba-2505-4b60-942d-63fddce170bb
📒 Files selected for processing (1)
Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs
- ShouldFallbackTranscode: require at least one existing source file (prevents fallback on dangling symlinks / missing files) - ApplyFallbackTranscode: deep-clone sources via JSON to avoid mutating cached MediaSourceInfo objects across requests - Add PolicyAllowsFallback helper to avoid redundant IsSourcePlayable re-scans in the filter after sources were already evaluated - Upgrade TryForceTranscodeBodyAsync catch log from Debug to Warning - GetUserPolicyTests: use isolated per-fixture temp directory - Add 5 new tests: mutation regression, missing sources, PolicyAllowsFallback
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs (1)
277-293: ExtractItemIdFromPath won't handle paths without trailing slash.If the path is
/Items/{guid}with no trailing segment (like/PlaybackInfo),IndexOf('/', start)returns -1, causing the method to returnGuid.Empty. For PlaybackInfo endpoints (which are/Items/{guid}/PlaybackInfo), this works fine. But if future endpoints use/Items/{guid}directly, this would fail.Consider handling
end < 0by settingend = path.Length:Proposed fix
var end = path.IndexOf('/', start); - if (end <= start) + if (end < 0) + { + end = path.Length; + } + + if (end <= start) { return Guid.Empty; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines 277 - 293, ExtractItemIdFromPath currently fails when the path is "/Items/{guid}" with no trailing slash because path.IndexOf('/', start) returns -1; update ExtractItemIdFromPath to treat a negative end as the string end (set end = path.Length when end < 0), then validate end > start and continue to TryParse the substring/span into itemId (keeping the existing TryParse logic) so standalone "/Items/{guid}" paths return the GUID instead of Guid.Empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs`:
- Around line 277-293: ExtractItemIdFromPath currently fails when the path is
"/Items/{guid}" with no trailing slash because path.IndexOf('/', start) returns
-1; update ExtractItemIdFromPath to treat a negative end as the string end (set
end = path.Length when end < 0), then validate end > start and continue to
TryParse the substring/span into itemId (keeping the existing TryParse logic) so
standalone "/Items/{guid}" paths return the GUID instead of Guid.Empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5166358-5a8d-4ad8-aee5-631970d034ce
📒 Files selected for processing (4)
Jellyfin.Plugin.QualityGate.Tests/FallbackTranscodeTests.csJellyfin.Plugin.QualityGate.Tests/GetUserPolicyTests.csJellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.csJellyfin.Plugin.QualityGate/Services/QualityGateService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Jellyfin.Plugin.QualityGate/Services/QualityGateService.cs
Replace the FallbackTranscode checkbox with a dropdown selector that combines the fallback toggle and resolution cap into one control. Options: Block, 480p, 720p, 1080p, 1440p, 4K, or no cap. Backend adds FallbackMaxHeight to QualityPolicy and injects CodecProfile conditions + MaxStreamingBitrate into the DeviceProfile POST body to enforce the resolution limit during server-side transcoding.
When path is /Items/{guid} without a trailing segment, IndexOf('/',
start) returns -1. Treat that as end-of-string so the GUID is still
parsed correctly.
Addresses CodeRabbit nitpick.
Document Jellyfin 10.11 DeviceProfile body modification approach, CodecProfiles resolution capping, JSON deep-cloning requirement, CollectionFolder guard, and updated policy/enforcement docs.
ShouldSkipIntro only checked PlaybackPositionTicks > 0 (resuming), but after finishing a movie the position resets to 0 with Played=true. Now also checks userData.Played to prevent the intro replaying on every subsequent watch.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs (1)
274-276: CastitemIdto(object)in this structured log.This log already casts
userId, butitemIdis still passed as a rawGuid. Keep Guid placeholders consistent with the repo logging rule.♻️ Proposed fix
_logger.LogInformation( "QualityGate: Forcing transcode via DeviceProfile for item {Item} (user {User}, policy {Policy}, maxHeight {MaxH})", - itemId, (object)userId, policy.Name, policy.FallbackMaxHeight); + (object)itemId, (object)userId, policy.Name, policy.FallbackMaxHeight);As per coding guidelines
Log structured messages with {Placeholder} syntax in C#; cast Guid arguments to (object) to avoid boxing ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines 274 - 276, The LogInformation call in MediaSourceResultFilter.cs passes itemId as a Guid without casting, violating the repo's structured-logging rule; update the _logger.LogInformation invocation that currently logs "QualityGate: Forcing transcode via DeviceProfile for item {Item} (user {User}, policy {Policy}, maxHeight {MaxH})" to cast itemId to (object) like the existing (object)userId so all Guid placeholders are consistently boxed (keep policy.Name and policy.FallbackMaxHeight as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Jellyfin.Plugin.QualityGate/Configuration/configPage.js`:
- Around line 1045-1046: The event handler is still watching the old fallback
selector ".policy-fallback-transcode" so changes to the rendered dropdown
".policy-fallback-mode" don't trigger refreshComputedPreview(view); update the
selector list used in the event.target.matches call (the one that currently
checks '#defaultPolicySelect, .policy-enabled, .policy-fallback-transcode,
.policy-name') to replace ".policy-fallback-transcode" with
".policy-fallback-mode" so changes to the fallback dropdown invoke
refreshComputedPreview(view).
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs`:
- Around line 24-32: Update the XML summary on MediaSourceResultFilter to
reflect the actual behavior: replace the description that says "modifies the
query string to set enableDirectPlay/enableDirectStream" with a statement that
Phase 1 (IAsyncResourceFilter) rewrites the POST body DeviceProfile to force
transcode (see TryForceTranscodeBodyAsync), and keep the rest about removing
blocked MediaSources before serialization; also update or remove any conflicting
lines later in the summary so it matches the implemented methods like
TryForceTranscodeBodyAsync and the resource/filter phases.
- Around line 178-188: The current early-return in MediaSourceResultFilter (the
block using _mediaSourceManager.GetStaticMediaSources and sources.Any(s =>
QualityGateService.IsSourcePlayable(policy, s.Path))) fails to treat configured
intro videos as playable, so TryForceTranscodeBodyAsync can later
force-transcode intros; update that check to also consider intro paths as
playable by comparing each source.Path against the configured intro paths
(DefaultIntroVideoPath and policy.IntroVideoPath) so FilterResult is skipped for
intros too; locate the logic referenced by TryForceTranscodeBodyAsync,
OnResultExecutionAsync and FilterResult and change the sources.Any(...)
condition to include a predicate that returns true when s.Path equals either the
global DefaultIntroVideoPath or the policy.IntroVideoPath (falling back
appropriately) in addition to QualityGateService.IsSourcePlayable.
---
Nitpick comments:
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs`:
- Around line 274-276: The LogInformation call in MediaSourceResultFilter.cs
passes itemId as a Guid without casting, violating the repo's structured-logging
rule; update the _logger.LogInformation invocation that currently logs
"QualityGate: Forcing transcode via DeviceProfile for item {Item} (user {User},
policy {Policy}, maxHeight {MaxH})" to cast itemId to (object) like the existing
(object)userId so all Guid placeholders are consistently boxed (keep policy.Name
and policy.FallbackMaxHeight as-is).
🪄 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: 496fc099-695a-4378-b494-d15f53827741
📒 Files selected for processing (4)
AGENTS.mdJellyfin.Plugin.QualityGate/Configuration/PluginConfiguration.csJellyfin.Plugin.QualityGate/Configuration/configPage.jsJellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Jellyfin.Plugin.QualityGate/Configuration/PluginConfiguration.cs
Comment on lines
+1045
to
1046
| if (event.target.matches('#defaultPolicySelect, .policy-enabled, .policy-fallback-transcode, .policy-name')) { | ||
| refreshComputedPreview(view); |
There was a problem hiding this comment.
Use the new fallback selector class here.
The UI renders .policy-fallback-mode, but this handler still watches .policy-fallback-transcode. Changing the fallback dropdown will not trigger refreshComputedPreview().
🔧 Proposed fix
- if (event.target.matches('#defaultPolicySelect, .policy-enabled, .policy-fallback-transcode, .policy-name')) {
+ if (event.target.matches('#defaultPolicySelect, .policy-enabled, .policy-fallback-mode, .policy-name')) {
refreshComputedPreview(view);
}📝 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
| if (event.target.matches('#defaultPolicySelect, .policy-enabled, .policy-fallback-transcode, .policy-name')) { | |
| refreshComputedPreview(view); | |
| if (event.target.matches('#defaultPolicySelect, .policy-enabled, .policy-fallback-mode, .policy-name')) { | |
| refreshComputedPreview(view); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Jellyfin.Plugin.QualityGate/Configuration/configPage.js` around lines 1045 -
1046, The event handler is still watching the old fallback selector
".policy-fallback-transcode" so changes to the rendered dropdown
".policy-fallback-mode" don't trigger refreshComputedPreview(view); update the
selector list used in the event.target.matches call (the one that currently
checks '#defaultPolicySelect, .policy-enabled, .policy-fallback-transcode,
.policy-name') to replace ".policy-fallback-transcode" with
".policy-fallback-mode" so changes to the fallback dropdown invoke
refreshComputedPreview(view).
Comment on lines
+24
to
+32
| /// Two-phase MVC filter that forces Jellyfin's transcoding pipeline to build proper | ||
| /// TranscodingUrls when fallback-transcode is needed, and removes blocked MediaSources | ||
| /// from API responses before serialization. | ||
| /// | ||
| /// Phase 1 (IAsyncResourceFilter): runs BEFORE model binding. For PlaybackInfo requests | ||
| /// where fallback is needed, modifies the query string to set enableDirectPlay=false and | ||
| /// enableDirectStream=false. Jellyfin's StreamBuilder then chooses Transcode and builds | ||
| /// proper TranscodingUrls — simply flipping flags on the result doesn't work because | ||
| /// TranscodingUrl remains null when StreamBuilder originally chose DirectPlay. |
There was a problem hiding this comment.
Update the XML summary to match the implementation.
Lines 29-32 still describe query-string mutation, but this filter now rewrites DeviceProfile in the POST body. The summary now contradicts TryForceTranscodeBodyAsync() and the later XML docs in this file.
📝 Proposed fix
-/// Phase 1 (IAsyncResourceFilter): runs BEFORE model binding. For PlaybackInfo requests
-/// where fallback is needed, modifies the query string to set enableDirectPlay=false and
-/// enableDirectStream=false. Jellyfin's StreamBuilder then chooses Transcode and builds
-/// proper TranscodingUrls — simply flipping flags on the result doesn't work because
-/// TranscodingUrl remains null when StreamBuilder originally chose DirectPlay.
+/// Phase 1 (IAsyncResourceFilter): runs BEFORE model binding. For PlaybackInfo requests
+/// where fallback is needed, rewrites the POST body `DeviceProfile` to remove
+/// `DirectPlayProfiles`. Jellyfin's StreamBuilder then chooses Transcode and builds
+/// proper `TranscodingUrl` values before the response reaches the result filter.📝 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
| /// Two-phase MVC filter that forces Jellyfin's transcoding pipeline to build proper | |
| /// TranscodingUrls when fallback-transcode is needed, and removes blocked MediaSources | |
| /// from API responses before serialization. | |
| /// | |
| /// Phase 1 (IAsyncResourceFilter): runs BEFORE model binding. For PlaybackInfo requests | |
| /// where fallback is needed, modifies the query string to set enableDirectPlay=false and | |
| /// enableDirectStream=false. Jellyfin's StreamBuilder then chooses Transcode and builds | |
| /// proper TranscodingUrls — simply flipping flags on the result doesn't work because | |
| /// TranscodingUrl remains null when StreamBuilder originally chose DirectPlay. | |
| /// Two-phase MVC filter that forces Jellyfin's transcoding pipeline to build proper | |
| /// TranscodingUrls when fallback-transcode is needed, and removes blocked MediaSources | |
| /// from API responses before serialization. | |
| /// | |
| /// Phase 1 (IAsyncResourceFilter): runs BEFORE model binding. For PlaybackInfo requests | |
| /// where fallback is needed, rewrites the POST body `DeviceProfile` to remove | |
| /// `DirectPlayProfiles`. Jellyfin's StreamBuilder then chooses Transcode and builds | |
| /// proper `TranscodingUrl` values before the response reaches the result filter. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines
24 - 32, Update the XML summary on MediaSourceResultFilter to reflect the actual
behavior: replace the description that says "modifies the query string to set
enableDirectPlay/enableDirectStream" with a statement that Phase 1
(IAsyncResourceFilter) rewrites the POST body DeviceProfile to force transcode
(see TryForceTranscodeBodyAsync), and keep the rest about removing blocked
MediaSources before serialization; also update or remove any conflicting lines
later in the summary so it matches the implemented methods like
TryForceTranscodeBodyAsync and the resource/filter phases.
Comment on lines
+178
to
+188
| var sources = _mediaSourceManager.GetStaticMediaSources(item, false); | ||
| if (sources == null || sources.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // If any source passes the policy, normal filtering is enough — no fallback needed | ||
| if (sources.Any(s => QualityGateService.IsSourcePlayable(policy, s.Path))) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Skip configured intros in phase 1 too.
TryForceTranscodeBodyAsync() can force-transcode intro PlaybackInfo requests because it only checks IsSourcePlayable(). Once ForcedTranscodeKey is set, OnResultExecutionAsync() skips FilterResult(), so the existing intro exemption never runs.
🔧 Proposed fix
var sources = _mediaSourceManager.GetStaticMediaSources(item, false);
if (sources == null || sources.Count == 0)
{
return;
}
+
+ if (sources.Any(s => IsConfiguredIntroPath(s.Path)))
+ {
+ return;
+ }
// If any source passes the policy, normal filtering is enough — no fallback needed
if (sources.Any(s => QualityGateService.IsSourcePlayable(policy, s.Path)))
{
return;As per coding guidelines Filter must skip policy enforcement for intro video playback by checking media source paths against configured intro paths (DefaultIntroVideoPath + per-policy IntroVideoPath).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Jellyfin.Plugin.QualityGate/Filters/MediaSourceResultFilter.cs` around lines
178 - 188, The current early-return in MediaSourceResultFilter (the block using
_mediaSourceManager.GetStaticMediaSources and sources.Any(s =>
QualityGateService.IsSourcePlayable(policy, s.Path))) fails to treat configured
intro videos as playable, so TryForceTranscodeBodyAsync can later
force-transcode intros; update that check to also consider intro paths as
playable by comparing each source.Path against the configured intro paths
(DefaultIntroVideoPath and policy.IntroVideoPath) so FilterResult is skipped for
intros too; locate the logic referenced by TryForceTranscodeBodyAsync,
OnResultExecutionAsync and FilterResult and change the sources.Any(...)
condition to include a predicate that returns true when s.Path equals either the
global DefaultIntroVideoPath or the policy.IntroVideoPath (falling back
appropriately) in addition to QualityGateService.IsSourcePlayable.
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.
Summary
SupportsDirectPlay=false+SupportsDirectStream=falseon all sources, forcing Jellyfin to transcode.DenyAllPolicy(misconfiguration sentinel) is explicitly excluded — always stays fail-closed.Changed files
PluginConfiguration.csFallbackTranscodebool onQualityPolicyQualityGateService.csShouldFallbackTranscode()+ApplyFallbackTranscode()helpersMediaSourceResultFilter.csQualityGateController.csQualityGateIntroProvider.csconfigPage.js+configPage.htmlFallbackTranscodeTests.csTest plan
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation