Open
Conversation
added 9 commits
February 2, 2026 14:08
…ent caching (same as images)
📝 WalkthroughWalkthroughThis change implements HTTP Range request support for asset streaming. The interface, delegate, implementation, and controller are updated to accept optional range headers, return extended metadata including content range information and partial content flags, and properly handle 206 Partial Content responses with appropriate HTTP headers. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as AssetController
participant Logic as PooledImmichFrameLogic
participant API as ImmichApi
participant HTTP as HttpClient
rect rgba(100, 150, 255, 0.5)
Note over Client,HTTP: Range Request Flow (206 Partial Content)
Client->>Controller: GET /asset?id=X<br/>Range: bytes=0-1023
Controller->>Controller: Read Range header
Controller->>Logic: GetAsset(id, assetType, rangeHeader)
Logic->>Logic: Detect rangeHeader present
alt Video Asset
Logic->>HTTP: GET with Range header
HTTP-->>Logic: 206 response + contentRange
Logic-->>Controller: (fileName, ContentType, stream,<br/>contentRange, isPartial=true, ...)
else Image Asset
Logic->>API: GetImageAsset(id)
API-->>Logic: (fileName, ContentType, stream)
Logic-->>Controller: (fileName, ContentType, stream,<br/>contentRange=null, isPartial=false, ...)
end
Controller->>Controller: Set Status 206
Controller->>Controller: Set Content-Range header
Controller->>Controller: Set Accept-Ranges header
Controller->>Client: Stream partial content
end
rect rgba(150, 255, 100, 0.5)
Note over Client,HTTP: Normal Request Flow (200 OK)
Client->>Controller: GET /asset?id=Y<br/>(no Range header)
Controller->>Logic: GetAsset(id, assetType, null)
Logic->>API: Standard asset retrieval
API-->>Logic: (fileName, ContentType, stream)
Logic-->>Controller: Full asset tuple
Controller->>Controller: Send webhook notification
Controller->>Client: File(...) response (200 OK)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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.
🧹 Nitpick comments (5)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
16-28: SharingHttpClientbetween generated client and manual requests is fine, but worth a brief note.
_httpClientis the same instance that's passed toImmichApi. This is desirable (same auth, base config), but it means any default headers or handler changes on the shared client affect both paths. A short comment on line 27 clarifying the intentional reuse would aid readability.🤖 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 16 - 28, Add a short inline comment near the HttpClient assignment/usage (around where _httpClient is set and new ImmichApi(...) is constructed) clarifying that the HttpClient instance returned from httpClientFactory and passed into ImmichApi is intentionally shared for consistent auth/configuration (via UseApiKey) and that changes to default headers or message handlers will affect both the generated ImmichApi client and any manual requests made with _httpClient; reference the symbols _httpClient, httpClientFactory.CreateClient("ImmichApiAccountClient"), UseApiKey(accountSettings.ApiKey), and ImmichApi to locate the spot.
166-191: Hardcoded API path is a maintenance risk; also consider propagatingCancellationToken.The endpoint
/assets/{id}/video/playbackis hardcoded (Line 168), duplicating knowledge that's otherwise encapsulated in the generated API client. If the Immich API path changes, this will silently break. A brief code comment noting this coupling would help future maintainers.Also,
GetVideoAsset(line 193) andGetAsset(line 86) don't accept or propagate aCancellationToken, so if a client disconnects mid-stream, the upstream HTTP call inPlayVideoWithRangecan't be cancelled. Consider threadingCancellationTokenthrough from the controller (e.g., viaHttpContext.RequestAborted).♻️ Minor: add a comment about the hardcoded path
private async Task<FileResponse> PlayVideoWithRange(Guid id, string rangeHeader, CancellationToken cancellationToken = default) { + // NOTE: This path must stay in sync with the Immich API's video playback endpoint + // (also used by the generated client's PlayAssetVideoAsync). var url = $"{_immichApi.BaseUrl.TrimEnd('/')}/assets/{id}/video/playback";🤖 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 166 - 191, PlayVideoWithRange currently constructs a hardcoded path using _immichApi.BaseUrl and doesn't fully propagate a CancellationToken; add a short inline comment near the url construction in PlayVideoWithRange calling out the coupling to the Immich API path ("/assets/{id}/video/playback") so future maintainers notice the dependency, and then thread CancellationToken through the call chain by adding a CancellationToken parameter to GetVideoAsset and GetAsset (and any callers, e.g., controller actions) and passing that token into PlayVideoWithRange and into SendAsync/ReadAsStreamAsync calls so upstream HTTP requests can be cancelled (use HttpContext.RequestAborted from controllers when invoking GetVideoAsset/GetAsset).ImmichFrame.WebApi/Controllers/AssetController.cs (2)
139-148:asset.disposenot disposed inGetRandomImageAndInfo.The
GetAssetcall returns a tuple with adisposemember, but onlyfileStreamis consumed here. For the IMAGE pathdisposeis currentlynull, so no leak today — but if a future code path returns a non-null disposable, this will leak. Defensive disposal is cheap.♻️ Suggested 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 (var memoryStream = new MemoryStream()) + try { - await asset.fileStream.CopyToAsync(memoryStream); - randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); + using (var memoryStream = new MemoryStream()) + { + await asset.fileStream.CopyToAsync(memoryStream); + randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); + } + } + finally + { + asset.fileStream.Dispose(); + asset.dispose?.Dispose(); }🤖 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, The tuple returned by _logic.GetAsset (captured in local variable asset) includes a disposable member named dispose that is not being disposed in GetRandomImageAndInfo; update the method to ensure the returned dispose is cleaned up (e.g., wrap the disposable in a using or call dispose?.Dispose() after consuming asset.fileStream) so any future non-null disposables aren't leaked while keeping the existing fileStream handling and webhook send logic unchanged.
91-108: Partial content path: the stream and response disposal ordering looks correct, butfileStreamis disposed twice.On line 102,
await using (asset.fileStream)disposes the stream. Then on line 106,asset.dispose?.Dispose()disposes theFileResponse/HttpResponseMessage, which also internally disposes its content stream. Double-disposal is tolerated byStreamimplementations in .NET (it's a no-op), so this isn't a bug, but the intent could be clearer by only disposing the owning response object.Also, if
CopyToAsyncthrows,asset.disposeon line 106 won't be reached. Consider wrapping both in atry/finallyor usingResponse.RegisterForDisposehere as well.♻️ Suggested cleanup for the partial content path
if (asset.isPartial && !string.IsNullOrEmpty(asset.contentRange)) { Response.Headers["Content-Range"] = asset.contentRange; Response.StatusCode = 206; Response.ContentType = asset.ContentType; if (asset.fileStream is { CanSeek: true } && asset.fileStream.Length > 0) Response.ContentLength = asset.fileStream.Length; else if (!string.IsNullOrEmpty(asset.contentLength) && long.TryParse(asset.contentLength, out var length)) Response.ContentLength = length; - await using (asset.fileStream) - { - await asset.fileStream.CopyToAsync(Response.Body); - } - asset.dispose?.Dispose(); - return new EmptyResult(); + try + { + await asset.fileStream.CopyToAsync(Response.Body); + } + finally + { + asset.dispose?.Dispose(); + } + return new EmptyResult(); }🤖 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 91 - 108, The partial-content branch currently disposes asset.fileStream via "await using (asset.fileStream)" and then calls asset.dispose?.Dispose(), risking double-disposal and missing dispose if CopyToAsync throws; remove the "await using (asset.fileStream)" and instead ensure the owning disposable is always cleaned up by registering or finally-disposing the owner (use Response.RegisterForDispose(asset.dispose) or wrap Response.Body.CopyToAsync in try/finally and call asset.dispose?.Dispose() in finally), keep setting Response.Headers/StatusCode/ContentType/ContentLength and return EmptyResult as before so the stream itself is not separately disposed inside this block.ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
12-12: Consider replacing the 7-element tuple with a named record/class.This tuple is becoming unwieldy and hard to maintain across the interface, two implementations, and the controller. A dedicated type would improve readability, make future additions non-breaking, and fix the inconsistent naming (
ContentTypeis PascalCase while the rest are camelCase).♻️ Suggested record type
public record AssetResult( string FileName, string ContentType, Stream FileStream, string? ContentRange, bool IsPartial, IDisposable? ResponseDisposable, string? ContentLength);Then the signature simplifies to:
-public Task<(string fileName, string ContentType, Stream fileStream, string? contentRange, bool isPartial, IDisposable? dispose, string? contentLength)> GetAsset(Guid id, AssetTypeEnum? assetType = null, string? rangeHeader = null); +public Task<AssetResult> GetAsset(Guid id, AssetTypeEnum? assetType = null, string? rangeHeader = 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 named record to improve readability and maintainability: define a record (suggested name AssetResult) with PascalCase properties (FileName, ContentType, FileStream, ContentRange, IsPartial, ResponseDisposable, ContentLength), change the IImmichFrameLogic.GetAsset signature to return Task<AssetResult>, and update all implementations and the controller usages to construct and consume the AssetResult instead of the tuple (ensuring you forward the disposable via ResponseDisposable and preserve semantics for partial responses and content length).
🤖 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/Interfaces/IImmichFrameLogic.cs`:
- Line 12: Replace the 7-element tuple return from IImmichFrameLogic.GetAsset
with a named record to improve readability and maintainability: define a record
(suggested name AssetResult) with PascalCase properties (FileName, ContentType,
FileStream, ContentRange, IsPartial, ResponseDisposable, ContentLength), change
the IImmichFrameLogic.GetAsset signature to return Task<AssetResult>, and update
all implementations and the controller usages to construct and consume the
AssetResult instead of the tuple (ensuring you forward the disposable via
ResponseDisposable and preserve semantics for partial responses and content
length).
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 16-28: Add a short inline comment near the HttpClient
assignment/usage (around where _httpClient is set and new ImmichApi(...) is
constructed) clarifying that the HttpClient instance returned from
httpClientFactory and passed into ImmichApi is intentionally shared for
consistent auth/configuration (via UseApiKey) and that changes to default
headers or message handlers will affect both the generated ImmichApi client and
any manual requests made with _httpClient; reference the symbols _httpClient,
httpClientFactory.CreateClient("ImmichApiAccountClient"),
UseApiKey(accountSettings.ApiKey), and ImmichApi to locate the spot.
- Around line 166-191: PlayVideoWithRange currently constructs a hardcoded path
using _immichApi.BaseUrl and doesn't fully propagate a CancellationToken; add a
short inline comment near the url construction in PlayVideoWithRange calling out
the coupling to the Immich API path ("/assets/{id}/video/playback") so future
maintainers notice the dependency, and then thread CancellationToken through the
call chain by adding a CancellationToken parameter to GetVideoAsset and GetAsset
(and any callers, e.g., controller actions) and passing that token into
PlayVideoWithRange and into SendAsync/ReadAsStreamAsync calls so upstream HTTP
requests can be cancelled (use HttpContext.RequestAborted from controllers when
invoking GetVideoAsset/GetAsset).
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 139-148: The tuple returned by _logic.GetAsset (captured in local
variable asset) includes a disposable member named dispose that is not being
disposed in GetRandomImageAndInfo; update the method to ensure the returned
dispose is cleaned up (e.g., wrap the disposable in a using or call
dispose?.Dispose() after consuming asset.fileStream) so any future non-null
disposables aren't leaked while keeping the existing fileStream handling and
webhook send logic unchanged.
- Around line 91-108: The partial-content branch currently disposes
asset.fileStream via "await using (asset.fileStream)" and then calls
asset.dispose?.Dispose(), risking double-disposal and missing dispose if
CopyToAsync throws; remove the "await using (asset.fileStream)" and instead
ensure the owning disposable is always cleaned up by registering or
finally-disposing the owner (use Response.RegisterForDispose(asset.dispose) or
wrap Response.Body.CopyToAsync in try/finally and call asset.dispose?.Dispose()
in finally), keep setting Response.Headers/StatusCode/ContentType/ContentLength
and return EmptyResult as before so the stream itself is not separately disposed
inside this block.
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 by CodeRabbit
Release Notes
New Features
Refactor