Conversation
…ent caching (same as images)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGetAsset now accepts an optional Range header and returns extended metadata (contentRange, isPartial, dispose, contentLength). Video streaming uses an injected HttpClient with range support and explicit status/header handling; the Web API controller reads Range, sets Accept-Ranges, and can return 206 Partial Content. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as AssetController
participant Logic as PooledImmichFrameLogic
participant External as ExternalImmichAPI
Client->>Controller: GET /assets/{id} (Range: bytes=..)?
Controller->>Logic: GetAsset(id, assetType, rangeHeader)
Logic->>External: HTTP GET (via _httpClient) with Range header (if provided)
External-->>Logic: 200 OK / 206 Partial + headers (Content-Type, Content-Range, Content-Length) + stream
alt isPartial (206)
Logic-->>Controller: (fileName, ContentType, stream, contentRange, isPartial=true, dispose, contentLength)
Controller->>Client: 206 Partial Content (writes stream, sets Content-Range, Accept-Ranges, Content-Length)
else full (200)
Logic-->>Controller: (fileName, ContentType, stream, null, isPartial=false, dispose, contentLength)
Controller->>Client: 200 OK (FileStreamResult / File(..., enableRangeProcessing:true))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 165-206: In GetVideoAsset (PooledImmichFrameLogic.GetVideoAsset)
move the local cache check (using _downloadLocation, fileName and
_generalSettings.RenewImagesDuration) to run before calling
_immichApi.PlayAssetVideoAsync so we short‑circuit on cache hits; then call the
API only if cache miss or expired, wrap the returned videoResponse (and its
Stream) in a using block to ensure disposal, and on API failure fall back to
returning the cached file if present; mirror the flow used in GetImageAsset to
ensure no leaked streams and proper cache-first behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 176-180: The cache-hit branch in PooledImmichFrameLogic is
returning a hardcoded content-type "video/mp4" for cached files (see filePath,
fileName and the caller that expects a tuple of (fileName, contentType,
Stream)), which can mismatch original responses; fix by persisting the original
content-type when first saving the file (e.g., write a companion metadata file
next to filePath like filePath + ".meta" or embed a suffix) and on cache hit
read that metadata and return the stored content-type instead of the hardcoded
"video/mp4"; also add a safe fallback to "video/mp4" if the metadata is missing
or unreadable and ensure any write/read logic is added where the file is created
and where the cached file is returned.
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
190-192: Consider extracting Content-Type parsing to reduce duplication.The Content-Type extraction logic is duplicated across both branches. A small helper could improve maintainability.
♻️ Optional refactor
+ private static string GetContentType(FileResponse response, string defaultType) + { + return response.Headers.ContainsKey("Content-Type") + ? response.Headers["Content-Type"].FirstOrDefault() ?? defaultType + : defaultType; + } + private async Task<(string fileName, string ContentType, Stream fileStream)> GetVideoAsset(Guid id) { // ... cache check ... using var videoResponse = await _immichApi.PlayAssetVideoAsync(id, string.Empty); if (videoResponse == null) throw new AssetNotFoundException($"Video asset {id} was not found!"); - var contentType = videoResponse.Headers.ContainsKey("Content-Type") - ? videoResponse.Headers["Content-Type"].FirstOrDefault() ?? "video/mp4" - : "video/mp4"; + var contentType = GetContentType(videoResponse, "video/mp4");Also applies to: 208-210
|
Workflow is not running, I don't know why. |
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
I noticed that with this change videos are no longer streamed from Immich but fully loaded into memory (or disk when caching is enabled) before being served.
While I understand this fixes Safari/iOS playback, it means the server is no longer just a proxy. For larger videos this could cause high memory usage and delayed playback start.
Is there a way to keep the original streaming behavior for non-Safari clients, or maybe forward Range requests to Immich instead?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
12-12: Consider replacing the 5-element tuple with a named record/class.A 5-element
ValueTuplehurts readability at every call site and makes future extensions (e.g., addingContent-Length) another breaking change. A small DTO likeAssetResultwith named properties would be clearer and more extensible.public record AssetResult(string FileName, string ContentType, Stream FileStream, string? ContentRange, bool IsPartial);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs` at line 12, Replace the 5-element tuple return from IImmichFrameLogic.GetAsset with a named DTO: create a record (e.g., AssetResult with properties FileName, ContentType, FileStream, ContentRange, IsPartial) and update the method signature from GetAsset(...) returning Task<(string fileName, string ContentType, Stream fileStream, string? contentRange, bool isPartial)> to Task<AssetResult>; update any implementing class/methods (and callers) to construct and consume AssetResult instead of the tuple to improve readability and future extensibility.ImmichFrame.WebApi/Controllers/AssetController.cs (1)
79-81: Range header is passed unsanitized to the logic layer.The raw
Rangeheader value from the client is forwarded directly to the upstream Immich server viaPlayVideoWithRange(which sets it on an outgoingHttpRequestMessage). While HTTP headers are generally safe to forward, consider validating that it matches the expectedbytes=START-ENDformat before passing it through. A malformed or adversarialRangeheader could trigger unexpected behavior in the upstream server.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 79 - 81, The Range header (Request.Headers["Range"] → rangeHeader) is forwarded unsanitized to _logic.GetAsset and ultimately to PlayVideoWithRange; validate and sanitize it first by ensuring it matches the expected bytes=START-END pattern (e.g. regex like ^bytes=(\d*)-(\d*)$) and reject or normalize anything else (either set rangeHeader to null/empty or return a BadRequest) before calling _logic.GetAsset so malformed or adversarial values are not passed through to PlayVideoWithRange.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 167-191: PlayVideoWithRange returns an IDisposable FileResponse
that holds the HttpResponseMessage (with
HttpCompletionOption.ResponseHeadersRead), but callers (GetVideoAsset) discard
it and never dispose it, leaking connections; to fix, change GetVideoAsset to
immediately buffer the FileResponse.Stream into a MemoryStream, reset its
Position to 0, dispose the FileResponse/HttpResponseMessage and then return the
buffered MemoryStream (or otherwise transfer the stream ownership to the
caller), or alternatively update the call chain so the caller that receives
FileResponse disposes it after fully consuming Stream; locate
PlayVideoWithRange, GetVideoAsset, FileResponse and
HttpCompletionOption.ResponseHeadersRead to implement buffering-and-dispose or
ensure deterministic disposal.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 88-95: The partial-content branch currently copies
asset.fileStream to Response.Body without disposing it and doesn't set
Content-Length; update the partial path (the block using asset.isPartial,
asset.contentRange, Response.Headers, Response.StatusCode, Response.ContentType,
await asset.fileStream.CopyToAsync(...), return new EmptyResult()) to (1) set
Response.Headers["Content-Range"] and Response.Headers["Content-Length"] (use
asset.ContentLength supplied by GetVideoAsset/PlayVideoWithRange), (2) copy the
stream and then Dispose/Close asset.fileStream in a try/finally or using pattern
so the underlying connection is released, and (3) if asset.fileStream is
non-seekable ensure GetVideoAsset/PlayVideoWithRange populates
asset.ContentLength by reading the upstream response Content-Length header and
include that length in the asset tuple so the partial branch can use it instead
of stream.Length.
---
Nitpick comments:
In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs`:
- Line 12: Replace the 5-element tuple return from IImmichFrameLogic.GetAsset
with a named DTO: create a record (e.g., AssetResult with properties FileName,
ContentType, FileStream, ContentRange, IsPartial) and update the method
signature from GetAsset(...) returning Task<(string fileName, string
ContentType, Stream fileStream, string? contentRange, bool isPartial)> to
Task<AssetResult>; update any implementing class/methods (and callers) to
construct and consume AssetResult instead of the tuple to improve readability
and future extensibility.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 79-81: The Range header (Request.Headers["Range"] → rangeHeader)
is forwarded unsanitized to _logic.GetAsset and ultimately to
PlayVideoWithRange; validate and sanitize it first by ensuring it matches the
expected bytes=START-END pattern (e.g. regex like ^bytes=(\d*)-(\d*)$) and
reject or normalize anything else (either set rangeHeader to null/empty or
return a BadRequest) before calling _logic.GetAsset so malformed or adversarial
values are not passed through to PlayVideoWithRange.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
169-169: URL construction duplicates knowledge of the Immich API route.The URL
$"{_immichApi.BaseUrl.TrimEnd('/')}/assets/{id}/video/playback"duplicates the route thatPlayAssetVideoAsyncuses internally. If the upstream API path changes, this will silently break while the NSwag-generated client gets updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` at line 169, The code in PooledImmichFrameLogic builds the playback URL manually, duplicating the API route; instead stop hardcoding $"{_immichApi.BaseUrl.TrimEnd('/')}/assets/{id}/video/playback" and use the NSwag-generated client API (e.g., call or build the request via _immichApi.PlayAssetVideoAsync or the client's request/URL builder) so the route is derived from the generated client; update the code that currently constructs the url variable to obtain the playback endpoint from _immichApi (or its request factory) rather than string-concatenating the path.ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
12-12: Consider replacing the 5-element tuple with a named record type.The return tuple
(string fileName, string ContentType, Stream fileStream, string? contentRange, bool isPartial)is becoming unwieldy. A dedicatedAssetResultrecord would improve readability at call sites, make future extensions (e.g., addingContent-Length) non-breaking, and allow the type to implementIDisposableto centralize stream lifecycle management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs` at line 12, The GetAsset method in IImmichFrameLogic returns a 5-element tuple which is hard to read and brittle; create a named record (e.g., AssetResult) containing properties FileName, ContentType, FileStream (Stream), ContentRange (string?), IsPartial (bool) and implement IDisposable on the record to manage the stream lifecycle, then change the interface signature from Task<(string fileName, string ContentType, Stream fileStream, string? contentRange, bool isPartial)> GetAsset(...) to Task<AssetResult> GetAsset(...). Update all implementations of IImmichFrameLogic (methods named GetAsset) and all callers to construct/consume AssetResult instead of the tuple, and add any needed nullability or using/dispose handling per the new IDisposable contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 83-84: The webhook is firing for every byte-range sub-request;
modify the AssetController where AssetRequestedNotification is created so you
only call _logic.SendWebhookNotification when this is the initial request (e.g.,
when rangeHeader is null/empty or indicates the first range like "bytes=0-");
locate the code that builds AssetRequestedNotification and wrap the call to
_logic.SendWebhookNotification(notification) in a conditional that checks
rangeHeader (or equivalent request header variable) and returns/omits the
webhook for subsequent range sub-requests.
---
Duplicate comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 167-191: PlayVideoWithRange currently returns a FileResponse that
holds the HttpResponseMessage (and its live stream) which is never disposed,
risking connection-pool exhaustion; change the consumer logic (GetVideoAsset and
PlayAssetVideoAsync) to fully buffer the response content for range requests
into a MemoryStream, dispose the returned FileResponse/HttpResponseMessage
immediately after buffering, and then return/serve the buffered stream (not the
FileResponse) so the underlying HTTP connection is released; locate symbols
PlayVideoWithRange, GetVideoAsset, PlayAssetVideoAsync, FileResponse and
HttpResponseMessage and implement the buffering+dispose pattern for partial
(range) and full responses as described.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 88-95: The partial-content branch writes asset.fileStream to
Response.Body but never disposes the stream (leaking the underlying
HttpResponseMessage from PlayVideoWithRange); after awaiting
asset.fileStream.CopyToAsync(Response.Body) in the 206 path, ensure the stream
is disposed (preferably via await asset.fileStream.DisposeAsync() or by wrapping
the stream usage in an await using/try/finally) before returning the
EmptyResult; update the code paths that handle asset.fileStream (the
partial-content branch in the action that calls PlayVideoWithRange) so ownership
is consistently released.
---
Nitpick comments:
In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs`:
- Line 12: The GetAsset method in IImmichFrameLogic returns a 5-element tuple
which is hard to read and brittle; create a named record (e.g., AssetResult)
containing properties FileName, ContentType, FileStream (Stream), ContentRange
(string?), IsPartial (bool) and implement IDisposable on the record to manage
the stream lifecycle, then change the interface signature from Task<(string
fileName, string ContentType, Stream fileStream, string? contentRange, bool
isPartial)> GetAsset(...) to Task<AssetResult> GetAsset(...). Update all
implementations of IImmichFrameLogic (methods named GetAsset) and all callers to
construct/consume AssetResult instead of the tuple, and add any needed
nullability or using/dispose handling per the new IDisposable contract.
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Line 169: The code in PooledImmichFrameLogic builds the playback URL manually,
duplicating the API route; instead stop hardcoding
$"{_immichApi.BaseUrl.TrimEnd('/')}/assets/{id}/video/playback" and use the
NSwag-generated client API (e.g., call or build the request via
_immichApi.PlayAssetVideoAsync or the client's request/URL builder) so the route
is derived from the generated client; update the code that currently constructs
the url variable to obtain the playback endpoint from _immichApi (or its request
factory) rather than string-concatenating the path.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
12-12: Consider replacing the 7-element tuple with a dedicated record type.A 7-element value tuple is unwieldy for callers and makes the interface contract harder to understand. A named record (e.g.,
AssetResult) would improve readability, allow adding fields without breaking every call site, and clarify ownership semantics (especially for theIDisposable? disposemember, whose purpose is opaque from the signature alone).Additionally,
ContentTypeuses PascalCase while the remaining elements use camelCase — minor inconsistency.Example record
public record AssetResult( string FileName, string ContentType, Stream FileStream, string? ContentRange = null, bool IsPartial = false, IDisposable? Disposable = null, string? ContentLength = null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs` at line 12, Replace the 7-element tuple return from IImmichFrameLogic.GetAsset with a dedicated record (e.g., AssetResult) to improve clarity and extendability; create a public record AssetResult with clearly named properties (FileName, ContentType, FileStream, ContentRange, IsPartial, Disposable, ContentLength) and change the method signature in IImmichFrameLogic from Task<(string fileName, string ContentType, Stream fileStream, string? contentRange, bool isPartial, IDisposable? dispose, string? contentLength)> GetAsset(...) to Task<AssetResult> GetAsset(...); also normalize casing (use PascalCase for property names) and ensure the nullable/ownership semantics of the IDisposable member are captured in the record name (Disposable) to make its purpose explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs`:
- Around line 45-46: The method declaration for GetAsset in class
MultiImmichFrameLogicDelegate is misaligned (starts at column 0); indent the
GetAsset method so it matches the other class members (use the same 4-space
indentation level as the other methods/properties in
MultiImmichFrameLogicDelegate) while keeping the existing signature and call to
_accountSelectionStrategy.ForAsset(logic => logic.GetAsset(...)).
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 186-189: The HttpResponseMessage named response is never disposed
on the error path which can leak sockets when using
HttpCompletionOption.ResponseHeadersRead; update the method (where response is
awaited and an ApiException thrown) to ensure response is disposed before
throwing — e.g., wrap the success path to return a new FileResponse without
disposing response and wrap the error path in a using or try/finally that calls
response.Dispose() after reading the error body, or restructure to use try/catch
that disposes response on failure while allowing the successful FileResponse to
retain ownership; ensure the ApiException is still constructed with status,
error and headers after disposing the response.
---
Duplicate comments:
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 83-84: The webhook is being sent for every HTTP Range sub-request
which causes duplicate notifications; in AssetController.cs before
creating/sending the AssetRequestedNotification via
_logic.SendWebhookNotification, check the incoming Range header
(Request.Headers["Range"]) and only call SendWebhookNotification when
string.IsNullOrEmpty(rangeHeader) (i.e., when no Range header is present, or
when it indicates a full request), so Safari's multiple Range sub-requests won't
trigger duplicate notifications.
- Around line 107-110: The using (asset.dispose) block disposes the response
stream too early (in AssetController around the File(asset.fileStream,
asset.ContentType, asset.fileName, enableRangeProcessing: true) return), so
remove that using and instead defer disposal until after ASP.NET finishes
sending the response; replace the immediate disposal with registering the
disposer via HttpContext.Response.RegisterForDispose(asset.dispose) (or the
equivalent Response.RegisterForDispose helper) so the asset.fileStream remains
open for the File/FileStreamResult to read, or if RegisterForDispose isn't
available implement deferred disposal via middleware or a response-completed
callback tied to the same asset.dispose.
---
Nitpick comments:
In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs`:
- Line 12: Replace the 7-element tuple return from IImmichFrameLogic.GetAsset
with a dedicated record (e.g., AssetResult) to improve clarity and
extendability; create a public record AssetResult with clearly named properties
(FileName, ContentType, FileStream, ContentRange, IsPartial, Disposable,
ContentLength) and change the method signature in IImmichFrameLogic from
Task<(string fileName, string ContentType, Stream fileStream, string?
contentRange, bool isPartial, IDisposable? dispose, string? contentLength)>
GetAsset(...) to Task<AssetResult> GetAsset(...); also normalize casing (use
PascalCase for property names) and ensure the nullable/ownership semantics of
the IDisposable member are captured in the record name (Disposable) to make its
purpose explicit.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.WebApi/Controllers/AssetController.cs (1)
139-148:⚠️ Potential issue | 🟠 Major
asset.fileStreamandasset.disposeare never disposed inGetRandomImageAndInfo— resource leak.
GetAssetnow returns a tuple that includesIDisposable? dispose. Line 139 callsGetAssetbut neitherasset.fileStreamnorasset.disposeis ever disposed after the copy at line 146. This is a new leak introduced by this PR's expanded return type.🐛 Proposed fix
var asset = await _logic.GetAsset(new Guid(randomAsset.Id), AssetTypeEnum.IMAGE); var notification = new AssetRequestedNotification(new Guid(randomAsset.Id), sanitizedClientIdentifier); _ = _logic.SendWebhookNotification(notification); string randomImageBase64; + using (asset.dispose) + await using var _ = asset.fileStream; using (var memoryStream = new MemoryStream()) { await asset.fileStream.CopyToAsync(memoryStream); randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 139 - 148, GetAsset now returns a tuple containing fileStream and an IDisposable (asset.dispose) but GetRandomImageAndInfo never disposes them, leaking resources; update the code around the var asset = await _logic.GetAsset(...) call to ensure asset.fileStream is disposed after copying (wrap the copy in a using or explicitly call Dispose/DisposeAsync) and also dispose asset.dispose (call Dispose or DisposeAsync based on its type) in a finally or using-like construct; keep the SendWebhookNotification call as-is but ensure both asset.fileStream and asset.dispose are cleaned up before method exit (reference symbols: GetAsset, asset.fileStream, asset.dispose, AssetRequestedNotification, SendWebhookNotification).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Line 74: The ProducesResponseType attribute on the AssetController method is
incorrect for 206 responses: it currently uses typeof(FileStreamResult) but the
206 branch writes directly to Response.Body and returns EmptyResult; update the
attribute on the relevant action in AssetController (the method that writes to
Response.Body and returns EmptyResult) to use typeof(void) or remove the type
argument so Swagger/OpenAPI reflects no response body for the 206 branch instead
of FileStreamResult.
- Around line 102-113: The current code disposes asset.dispose incorrectly
causing leaks on exceptions and premature disposal before ASP.NET executes
FileStreamResult; fix by registering the HttpResponseMessage for deferred
disposal instead of immediate Dispose: before copying or returning, call
HttpContext.Response.RegisterForDispose(asset.dispose) when asset.dispose is
non-null so the framework disposes it after the response is complete; remove the
explicit asset.dispose?.Dispose() after the await using and remove the using
(asset.dispose) block around the File(...) return so FileStreamResult can read
the stream during ExecuteResultAsync; keep await using for asset.fileStream but
ensure asset.dispose is registered for disposal up front (reference symbols:
asset.dispose, asset.fileStream, CopyToAsync, File(...)/FileStreamResult,
HttpContext.Response.RegisterForDispose()).
---
Outside diff comments:
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 139-148: GetAsset now returns a tuple containing fileStream and an
IDisposable (asset.dispose) but GetRandomImageAndInfo never disposes them,
leaking resources; update the code around the var asset = await
_logic.GetAsset(...) call to ensure asset.fileStream is disposed after copying
(wrap the copy in a using or explicitly call Dispose/DisposeAsync) and also
dispose asset.dispose (call Dispose or DisposeAsync based on its type) in a
finally or using-like construct; keep the SendWebhookNotification call as-is but
ensure both asset.fileStream and asset.dispose are cleaned up before method exit
(reference symbols: GetAsset, asset.fileStream, asset.dispose,
AssetRequestedNotification, SendWebhookNotification).
---
Duplicate comments:
In `@ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs`:
- Around line 45-46: The GetAsset method declaration in
MultiImmichFrameLogicDelegate has inconsistent leading/continuation indentation
compared to other expression-bodied methods; open the
MultiImmichFrameLogicDelegate class and align the GetAsset signature and its
continuation (the => _accountSelectionStrategy.ForAsset(...) expression) to
match the same column/indentation used by other methods (e.g., use the same
8-space indent used for similar => expressions), ensuring the declaration for
GetAsset and its continuation line use consistent whitespace; verify the
signature for GetAsset, the _accountSelectionStrategy.ForAsset call, and the
closing punctuation are all aligned with the surrounding methods.
| [Produces("image/jpeg", "image/webp", "video/mp4", "video/quicktime")] | ||
| [ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status200OK)] | ||
| public async Task<FileResult> GetAsset(Guid id, string clientIdentifier = "", AssetTypeEnum? assetType = null) | ||
| [ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status206PartialContent)] |
There was a problem hiding this comment.
ProducesResponseType for 206 incorrectly declares FileStreamResult.
The 206 branch returns EmptyResult (line 107) after writing directly to Response.Body. Annotating it with typeof(FileStreamResult) generates misleading Swagger/OpenAPI metadata. Use typeof(void) or omit the type argument.
♻️ Proposed fix
- [ProducesResponseType(typeof(FileStreamResult), StatusCodes.Status206PartialContent)]
+ [ProducesResponseType(StatusCodes.Status206PartialContent)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs` at line 74, The
ProducesResponseType attribute on the AssetController method is incorrect for
206 responses: it currently uses typeof(FileStreamResult) but the 206 branch
writes directly to Response.Body and returns EmptyResult; update the attribute
on the relevant action in AssetController (the method that writes to
Response.Body and returns EmptyResult) to use typeof(void) or remove the type
argument so Swagger/OpenAPI reflects no response body for the 206 branch instead
of FileStreamResult.
| await using (asset.fileStream) | ||
| { | ||
| await asset.fileStream.CopyToAsync(Response.Body); | ||
| } | ||
| asset.dispose?.Dispose(); | ||
| return new EmptyResult(); | ||
| } | ||
|
|
||
| using (asset.dispose) | ||
| { | ||
| return File(asset.fileStream, asset.ContentType, asset.fileName, enableRangeProcessing: true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine PooledImmichFrameLogic.cs to understand what dispose and fileStream are
find . -name "PooledImmichFrameLogic.cs" -type fRepository: immichFrame/ImmichFrame
Length of output: 117
🏁 Script executed:
# Search for GetImageAsset and GetVideoAsset method implementations
rg -n "GetImageAsset|GetVideoAsset" --type cs -A 15 | head -100Repository: immichFrame/ImmichFrame
Length of output: 4247
🏁 Script executed:
# Check AssetController.cs to see the full context around lines 102-113
fd "AssetController.cs" --type fRepository: immichFrame/ImmichFrame
Length of output: 116
🏁 Script executed:
# Read AssetController.cs to see the context around lines 102-113
cat -n ImmichFrame.WebApi/Controllers/AssetController.cs | head -150 | tail -60Repository: immichFrame/ImmichFrame
Length of output: 3097
🏁 Script executed:
# Get the complete GetImageAsset implementation
rg -n "private async Task.*GetImageAsset" ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs -A 40Repository: immichFrame/ImmichFrame
Length of output: 1659
🏁 Script executed:
# Get the complete GetVideoAsset implementation
rg -n "private async Task.*GetVideoAsset" ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs -A 80Repository: immichFrame/ImmichFrame
Length of output: 1528
🏁 Script executed:
# Find the GetAsset method that's called from the controller
rg -n "public.*Task.*GetAsset" ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs -A 25Repository: immichFrame/ImmichFrame
Length of output: 1777
🏁 Script executed:
# Check if there are other usages of asset in the controller that might affect resource lifecycle
rg -n "asset\." ImmichFrame.WebApi/Controllers/AssetController.cs -B 2 -A 2 | head -80Repository: immichFrame/ImmichFrame
Length of output: 1515
🏁 Script executed:
# Verify what File() returns and search for documentation or usage patterns
rg -n "File\(" ImmichFrame.WebApi/Controllers/AssetController.cs -B 2 -A 2Repository: immichFrame/ImmichFrame
Length of output: 269
Two resource-management bugs: asset.dispose leaks on exception (partial path) and is disposed prematurely before FileStreamResult executes (non-partial path).
Partial path (lines 102–107): asset.dispose?.Dispose() is called after the await using block but outside it. If CopyToAsync throws, the await using correctly cleans up asset.fileStream, but asset.dispose (the HttpResponseMessage for video range requests) is never released — leaking the HTTP connection.
Non-partial path (lines 110–113): For video assets, the using block disposes asset.dispose (the HttpResponseMessage) immediately after the return expression is evaluated, before the FileStreamResult is handed to ASP.NET Core. This closes the underlying stream before FileStreamResult.ExecuteResultAsync() executes, causing a read failure. (Image assets are unaffected because asset.dispose is null.) The safe pattern is to register asset.dispose for deferred disposal via HttpContext.Response.RegisterForDispose().
🐛 Proposed fix — partial path
- await using (asset.fileStream)
- {
- await asset.fileStream.CopyToAsync(Response.Body);
- }
- asset.dispose?.Dispose();
+ try
+ {
+ await using (asset.fileStream)
+ {
+ await asset.fileStream.CopyToAsync(Response.Body);
+ }
+ }
+ finally
+ {
+ asset.dispose?.Dispose();
+ }🐛 Proposed fix — non-partial path
- using (asset.dispose)
- {
- return File(asset.fileStream, asset.ContentType, asset.fileName, enableRangeProcessing: true);
- }
+ if (asset.dispose != null)
+ HttpContext.Response.RegisterForDispose(asset.dispose);
+ return File(asset.fileStream, asset.ContentType, asset.fileName, enableRangeProcessing: true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 102 - 113,
The current code disposes asset.dispose incorrectly causing leaks on exceptions
and premature disposal before ASP.NET executes FileStreamResult; fix by
registering the HttpResponseMessage for deferred disposal instead of immediate
Dispose: before copying or returning, call
HttpContext.Response.RegisterForDispose(asset.dispose) when asset.dispose is
non-null so the framework disposes it after the response is complete; remove the
explicit asset.dispose?.Dispose() after the await using and remove the using
(asset.dispose) block around the File(...) return so FileStreamResult can read
the stream during ExecuteResultAsync; keep await using for asset.fileStream but
ensure asset.dispose is registered for disposal up front (reference symbols:
asset.dispose, asset.fileStream, CopyToAsync, File(...)/FileStreamResult,
HttpContext.Response.RegisterForDispose()).
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (3)
86-108: Consider replacing the 7-element value tuple with a named type.The return type
(string, string, Stream, string?, bool, IDisposable?, string?)is difficult to reason about at call sites — positional access is fragile and the caller must know which element is which. A small record would make the contract self-documenting and reduce the risk of transposed members.Example
public record AssetResult( string FileName, string ContentType, Stream FileStream, string? ContentRange, bool IsPartial, IDisposable? DisposeHandle, string? ContentLength);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 86 - 108, Replace the 7-tuple return from GetAsset with a self-describing record type (e.g., AssetResult) so callers don't rely on positional tuple members; define a record like AssetResult(string FileName, string ContentType, Stream FileStream, string? ContentRange, bool IsPartial, IDisposable? DisposeHandle, string? ContentLength), change the signature of GetAsset to return AssetResult, update the return sites in GetAsset (where it currently constructs the tuple from GetImageAsset and GetVideoAsset) to construct and return an AssetResult, and update GetImageAsset/GetVideoAsset (or their callers) to produce AssetResult-compatible values; keep AssetNotFoundException usage unchanged.
168-168: URL path is duplicated outside the NSwag client.The path
/assets/{id}/video/playbackis hardcoded here and also lives inside the NSwag-generated client. If the Immich API changes this endpoint, this manual construction will silently break while the generated client gets updated. Consider extracting this path as a constant or adding a comment noting the coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` at line 168, The code in PooledImmichFrameLogic manually constructs the endpoint with _immichApi.BaseUrl + "/assets/{id}/video/playback", duplicating a path that also exists in the NSwag-generated client and risking mismatches; replace the hardcoded path by extracting it to a single named constant (e.g., ASSET_VIDEO_PLAYBACK_PATH) in this class or a shared constants file and reference that constant when building the URL, or better yet, call the NSwag client helper/method that returns this route if one exists; also add a short comment noting the coupling to the NSwag-generated endpoint so future API changes are caught in one place.
193-213: ThreadCancellationTokenthrough the call chain to enable cancellation on client disconnect.
PlayVideoWithRangeaccepts aCancellationTokenparameter but always receives the default value becauseGetAssetandGetVideoAssetdo not accept or forward one. Additionally, the ASP.NET controller has access toHttpContext.RequestAborted(the CancellationToken that signals client disconnect), but it is not passed to the logic layer.For long-running video streams, a client disconnect will continue holding the upstream HTTP connection until the stream naturally completes or is garbage-collected. Threading
CancellationTokenthrough the chain—starting from the controller'sHttpContext.RequestAborted→GetAsset→GetVideoAsset→PlayVideoWithRange—allows the stream to be cancelled promptly when the client disconnects.Also consider updating the
CopyToAsynccall at line 104 in the controller to use the overload that accepts aCancellationToken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 193 - 213, GetVideoAsset and its caller GetAsset currently don't accept or forward a CancellationToken so PlayVideoWithRange always runs with CancellationToken.None; add a CancellationToken parameter to GetAsset and GetVideoAsset, pass the token into PlayVideoWithRange, and ensure any upstream call sites (controller action) pass HttpContext.RequestAborted into GetAsset; also update the controller's stream CopyToAsync call to use the overload that accepts the same CancellationToken so the upstream HTTP request/response streams are cancelled promptly on client disconnect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 86-108: Replace the 7-tuple return from GetAsset with a
self-describing record type (e.g., AssetResult) so callers don't rely on
positional tuple members; define a record like AssetResult(string FileName,
string ContentType, Stream FileStream, string? ContentRange, bool IsPartial,
IDisposable? DisposeHandle, string? ContentLength), change the signature of
GetAsset to return AssetResult, update the return sites in GetAsset (where it
currently constructs the tuple from GetImageAsset and GetVideoAsset) to
construct and return an AssetResult, and update GetImageAsset/GetVideoAsset (or
their callers) to produce AssetResult-compatible values; keep
AssetNotFoundException usage unchanged.
- Line 168: The code in PooledImmichFrameLogic manually constructs the endpoint
with _immichApi.BaseUrl + "/assets/{id}/video/playback", duplicating a path that
also exists in the NSwag-generated client and risking mismatches; replace the
hardcoded path by extracting it to a single named constant (e.g.,
ASSET_VIDEO_PLAYBACK_PATH) in this class or a shared constants file and
reference that constant when building the URL, or better yet, call the NSwag
client helper/method that returns this route if one exists; also add a short
comment noting the coupling to the NSwag-generated endpoint so future API
changes are caught in one place.
- Around line 193-213: GetVideoAsset and its caller GetAsset currently don't
accept or forward a CancellationToken so PlayVideoWithRange always runs with
CancellationToken.None; add a CancellationToken parameter to GetAsset and
GetVideoAsset, pass the token into PlayVideoWithRange, and ensure any upstream
call sites (controller action) pass HttpContext.RequestAborted into GetAsset;
also update the controller's stream CopyToAsync call to use the overload that
accepts the same CancellationToken so the upstream HTTP request/response streams
are cancelled promptly on client disconnect.
2 participants
Fix Safari/iOS video playback by buffering videos to seekable streams
Safari requires seekable streams for video range requests. Previously, non-seekable network streams from Immich caused playback failures(error code 4 - MEDIA_ERR_SRC_NOT_SUPPORTED). Now videos are either cached to disk (when DownloadImages=true) or buffered to memory to provide seekability.
Summary by CodeRabbit
New Features
Bug Fixes
Reliability
Behavioral Change