Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces an asset request tracking system that logs when assets are accessed through the API, stores these records in-memory, and exposes them through new endpoints and a UI page displaying recent requests with metadata enrichment. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AssetController
participant IAssetRequestTracker
participant AssetRequestsController
participant ImmichFrameLogic
participant AssetRequestTracker as InMemoryAssetRequestTracker
Client->>AssetController: GET /api/assets/{id}
AssetController->>ImmichFrameLogic: GetAsset(assetId)
ImmichFrameLogic-->>AssetController: Asset
AssetController->>AssetRequestTracker: RecordAssetRequest(clientId, assetId, endpoint, type)
AssetRequestTracker->>AssetRequestTracker: Enqueue record, prune to 1000
AssetController-->>Client: Asset (image/metadata)
Client->>AssetRequestsController: GET /api/AssetRequests/RecentAssetRequests
AssetRequestsController->>AssetRequestTracker: GetAssetRequests(filters, limit)
AssetRequestTracker-->>AssetRequestsController: IReadOnlyCollection<AssetRequestRecord>
AssetRequestsController->>ImmichFrameLogic: GetAssetInfoById(assetId) [concurrent]
ImmichFrameLogic-->>AssetRequestsController: Exif, location data
AssetRequestsController->>AssetRequestsController: Enrich with metadata
AssetRequestsController-->>Client: RecentAssetRequestDetailsRecord[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
docker/docker-compose.override.local.yml (1)
1-13: Local development setup looks reasonable overall.A few observations for improved developer experience:
Volume path:
./Configis relative to wheredocker-composeis executed. If run from thedocker/directory, it expectsdocker/Config/. Consider documenting the expected execution context or using an absolute path reference.Prerequisite clarity: The image
immichframe:localmust be built before running this compose file. Consider adding a comment or documentation on how to build it (e.g.,docker build -t immichframe:local .).Config requirement: Per
ConfigLoader.cs, the app will throwImmichFrameException("Failed to load configuration")if noSettings.jsonorSettings.ymlexists in the mounted Config directory. Consider adding a sample config or documenting this requirement.💡 Suggested improvements for portability
name: immichframe-local services: immichframe: container_name: immichframe-local image: immichframe:local restart: unless-stopped ports: - "8081:8080" volumes: - ./Config:/app/Config:ro environment: - TZ: "Europe/London" + TZ: "${TZ:-Europe/London}"This allows developers to override TZ via a
.envfile or shell environment while providing a sensible default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/docker-compose.override.local.yml` around lines 1 - 13, The compose override uses a relative volume path and a hard-coded image/TZ without documenting prerequisites: update the docker-compose override to reference an explicit project-root-aware path (e.g., use an absolute or compose variable like ${COMPOSE_PROJECT_DIR}/Config) instead of ./Config, add a brief comment that the image immichframe:local must be built beforehand (e.g., docker build -t immichframe:local .) and note that the mounted Config must contain Settings.json or Settings.yml (or include a sample config in the repo), and make TZ configurable via an env var fallback (reference the service name immichframe, container_name immichframe-local, image immichframe:local, volumes ./Config and environment TZ to locate the exact lines to change)..gitignore (1)
457-459: Consider removing redundant ignore entries for clarity.With a root-scoped
/Config/rule,ImmichFrame.WebApi/Config/Settings.jsonis either unnecessary (if nested config should be tracked) or better handled by a deliberate scoped rule. Keeping both broad and specific entries makes intent harder to read.Also applies to: 461-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 457 - 459, Remove the redundant .gitignore entries by choosing one clear rule: either keep the broad "/Config/" root-scoped ignore or keep the explicit "ImmichFrame.WebApi/Config/Settings.json" line for a deliberate scoped ignore; delete the duplicate line(s) accordingly (references: the entries "ImmichFrame/Settings.json" and "ImmichFrame.WebApi/Config/Settings.json" and any existing "/Config/" rule) so the intent is unambiguous and avoid having both broad and specific ignores for the same path.immichFrame.Web/src/routes/recent-requests/+page.svelte (1)
107-142: Consider adding keyboard escape to close modal.The modal correctly uses ARIA attributes for accessibility, but users cannot close it with the Escape key. Consider adding keyboard handling:
♻️ Proposed enhancement for keyboard accessibility
+ const handleKeydown = (event: KeyboardEvent) => { + if (event.key === 'Escape' && selectedRequest) { + closeModal(); + } + };Then add to the component:
<svelte:window on:keydown={handleKeydown} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/routes/recent-requests/`+page.svelte around lines 107 - 142, The modal doesn't close on Escape; add keyboard handling by defining a handler (e.g., handleKeydown) that checks for event.key === 'Escape' and calls the existing closeModal() when a request is open (selectedRequest); then include <svelte:window on:keydown={handleKeydown} /> in the component root so the window keydown is captured while the modal is visible. Ensure the handler references the existing selectedRequest and closeModal symbols so it only closes when the modal is open.ImmichFrame.WebApi.Tests/Controllers/AssetRequestsControllerTests.cs (1)
92-112: Consider adding edge-case tests for robustness.The test verifies the happy path well. Consider adding tests for:
- Empty request history (no prior asset requests)
- Filtering by client identifier
- Limit parameter behavior
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi.Tests/Controllers/AssetRequestsControllerTests.cs` around lines 92 - 112, Add new unit tests alongside GetRecentAssetRequests_ReturnsLocationAndTakenTime to cover edge cases: (1) an empty history test that uses SetupRecentAssetRequestMocks or clears mocks then calls GET /api/AssetRequests/RecentAssetRequests?limit=10 and asserts an empty list, (2) a client-identifier filter test that issues GET /api/Asset/RandomImageAndInfo?clientIdentifier=KitchenPanel and then GET /api/AssetRequests/RecentAssetRequests?limit=10&clientIdentifier=KitchenPanel to assert only requests from that client are returned, and (3) a limit parameter test that seeds more than N requests via SetupRecentAssetRequestMocks and calls GET /api/AssetRequests/RecentAssetRequests?limit=N to assert exactly N items are returned and ordering is correct; add these as separate [Test] methods near GetRecentAssetRequests_ReturnsLocationAndTakenTime using the same factory/client patterns and response parsing to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 461-462: The .gitignore entry "Config/" currently ignores any
Config directory at any depth; change both entries to be rooted by replacing
"Config/" and "!Config/.gitkeep" with "/Config/" and "/Config/.gitkeep" so only
the top-level runtime Config folder is ignored while nested folders (e.g.,
ImmichFrame.WebApi/Config/) are not masked.
In `@immichFrame.Web/static/manifest.webmanifest`:
- Line 4: The PWA start_url in manifest ("start_url": "/recent-requests") will
land users on /recent-requests with no way back to the main frame; either add
explicit navigation from the recent-requests view to the main slideshow or
document the behavior as intentional. Update the recent-requests page
(component/page handling the route "/recent-requests") to include a visible
link/button that routes to "/" (the main photo frame/slideshow) and ensure it
appears in the mobile/installed PWA layout, or if this is designed as a separate
companion app, add a comment in manifest.webmanifest and project docs stating
that start_url="/recent-requests" is intentional and describe navigation
limitations for installers.
In `@ImmichFrame.WebApi/Controllers/AssetRequestsController.cs`:
- Around line 33-34: The current fan-out uses Task.WhenAll on
uniqueRecentRequests which can flood downstream GetAssetInfoById calls; change
the call site in AssetRequestsController to run EnrichAssetRequestAsync with
bounded concurrency (e.g., a SemaphoreSlim or Parallel.ForEachAsync) instead of
Task.WhenAll, using a configurable degreeOfParallelism (e.g., 5) to throttle
concurrent calls, await each enriched result into a list preserving order, and
ensure semaphore disposal or cancellation token propagation so downstream
metadata calls are protected.
- Around line 17-20: The GetAssetRequests endpoint currently forwards the raw
limit to tracker.GetAssetRequests allowing negative or huge values; fix by
validating/clamping limit inside the GetAssetRequests method (e.g., define a
MAX_LIMIT constant and use Math.Clamp or manual logic to ensure limit is within
a safe range like 1..1000) and pass the sanitizedLimit to
tracker.GetAssetRequests; alternatively return BadRequest if limit is <=0 or
exceeds MAX_LIMIT—refer to the GetAssetRequests method, the limit parameter, and
the tracker.GetAssetRequests call when making the change.
- Around line 28-32: The selection of a “recent” request is non-deterministic
because uniqueRecentRequests groups the results from tracker.GetAssetRequests
and uses GroupBy(...).First() without ordering; update the pipeline so requests
are ordered by RequestedAtUtc descending before the GroupBy to ensure the
First() in each group is the most recent (i.e. call OrderByDescending(r =>
r.RequestedAtUtc) on the sequence returned from GetAssetRequests prior to
GroupBy, then keep the existing Select(...).Take(...)).
- Around line 15-17: The GetAssetRequests action uses an explicit route segment
"[HttpGet(\"AssetRequests\")]" which, combined with the class route
"api/[controller]", produces a duplicated path
(/api/AssetRequests/AssetRequests); change the attribute on the GetAssetRequests
method to a parameterless [HttpGet] so the endpoint maps to the collection route
/api/AssetRequests (ensure the method signature for GetAssetRequests remains
unchanged and remove the extra route string).
---
Nitpick comments:
In @.gitignore:
- Around line 457-459: Remove the redundant .gitignore entries by choosing one
clear rule: either keep the broad "/Config/" root-scoped ignore or keep the
explicit "ImmichFrame.WebApi/Config/Settings.json" line for a deliberate scoped
ignore; delete the duplicate line(s) accordingly (references: the entries
"ImmichFrame/Settings.json" and "ImmichFrame.WebApi/Config/Settings.json" and
any existing "/Config/" rule) so the intent is unambiguous and avoid having both
broad and specific ignores for the same path.
In `@docker/docker-compose.override.local.yml`:
- Around line 1-13: The compose override uses a relative volume path and a
hard-coded image/TZ without documenting prerequisites: update the docker-compose
override to reference an explicit project-root-aware path (e.g., use an absolute
or compose variable like ${COMPOSE_PROJECT_DIR}/Config) instead of ./Config, add
a brief comment that the image immichframe:local must be built beforehand (e.g.,
docker build -t immichframe:local .) and note that the mounted Config must
contain Settings.json or Settings.yml (or include a sample config in the repo),
and make TZ configurable via an env var fallback (reference the service name
immichframe, container_name immichframe-local, image immichframe:local, volumes
./Config and environment TZ to locate the exact lines to change).
In `@immichFrame.Web/src/routes/recent-requests/`+page.svelte:
- Around line 107-142: The modal doesn't close on Escape; add keyboard handling
by defining a handler (e.g., handleKeydown) that checks for event.key ===
'Escape' and calls the existing closeModal() when a request is open
(selectedRequest); then include <svelte:window on:keydown={handleKeydown} /> in
the component root so the window keydown is captured while the modal is visible.
Ensure the handler references the existing selectedRequest and closeModal
symbols so it only closes when the modal is open.
In `@ImmichFrame.WebApi.Tests/Controllers/AssetRequestsControllerTests.cs`:
- Around line 92-112: Add new unit tests alongside
GetRecentAssetRequests_ReturnsLocationAndTakenTime to cover edge cases: (1) an
empty history test that uses SetupRecentAssetRequestMocks or clears mocks then
calls GET /api/AssetRequests/RecentAssetRequests?limit=10 and asserts an empty
list, (2) a client-identifier filter test that issues GET
/api/Asset/RandomImageAndInfo?clientIdentifier=KitchenPanel and then GET
/api/AssetRequests/RecentAssetRequests?limit=10&clientIdentifier=KitchenPanel to
assert only requests from that client are returned, and (3) a limit parameter
test that seeds more than N requests via SetupRecentAssetRequestMocks and calls
GET /api/AssetRequests/RecentAssetRequests?limit=N to assert exactly N items are
returned and ordering is correct; add these as separate [Test] methods near
GetRecentAssetRequests_ReturnsLocationAndTakenTime using the same factory/client
patterns and response parsing to keep consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5dfd00b-6f84-47dc-bf14-e9e1032b1909
📒 Files selected for processing (11)
.gitignoreImmichFrame.WebApi.Tests/Controllers/AssetRequestsControllerTests.csImmichFrame.WebApi/Controllers/AssetController.csImmichFrame.WebApi/Controllers/AssetRequestsController.csImmichFrame.WebApi/Models/ClientActivityModels.csImmichFrame.WebApi/Program.csImmichFrame.WebApi/Services/IAssetRequestTracker.csImmichFrame.WebApi/Services/InMemoryAssetRequestTracker.csdocker/docker-compose.override.local.ymlimmichFrame.Web/src/routes/recent-requests/+page.svelteimmichFrame.Web/static/manifest.webmanifest
Comment on lines
+461
to
+462
| Config/ | ||
| !Config/.gitkeep |
There was a problem hiding this comment.
Scope Config/ to repo root to avoid over-ignoring nested config directories.
Line 461 currently ignores any Config directory at any depth. If the intent is only the top-level runtime config folder, use /Config/ and /Config/.gitkeep so nested folders like ImmichFrame.WebApi/Config/ aren’t unintentionally masked for new files.
Suggested patch
-Config/
-!Config/.gitkeep
+/Config/
+!/Config/.gitkeep📝 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
| Config/ | |
| !Config/.gitkeep | |
| /Config/ | |
| !/Config/.gitkeep |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 461 - 462, The .gitignore entry "Config/" currently
ignores any Config directory at any depth; change both entries to be rooted by
replacing "Config/" and "!Config/.gitkeep" with "/Config/" and
"/Config/.gitkeep" so only the top-level runtime Config folder is ignored while
nested folders (e.g., ImmichFrame.WebApi/Config/) are not masked.
| "name": "ImmichFrame", | ||
| "short_name": "ImmichFrame", | ||
| "start_url": "/", | ||
| "start_url": "/recent-requests", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
git ls-files | head -30Repository: immichFrame/ImmichFrame
Length of output: 1198
🏁 Script executed:
# Find the manifest file and related web files
find . -name "manifest.webmanifest" -o -name "*.web" -o -path "*/static/*" -type f | head -20Repository: immichFrame/ImmichFrame
Length of output: 612
🏁 Script executed:
# Look at the manifest.webmanifest file and its context
cat -n immichFrame.Web/static/manifest.webmanifest 2>/dev/null || echo "File not found, searching..."Repository: immichFrame/ImmichFrame
Length of output: 663
🏁 Script executed:
# Search for recent-requests page/component
rg -i "recent-requests|recent.requests" --type html --type tsx --type ts | head -20Repository: immichFrame/ImmichFrame
Length of output: 94
🏁 Script executed:
# Check for routing configuration and navigation
rg -i "route|navigate|href|router" -A 2 -B 2 | grep -i "recent\|^/" | head -30Repository: immichFrame/ImmichFrame
Length of output: 49
🏁 Script executed:
# Look at the structure of the Web project
ls -la immichFrame.Web/static/ 2>/dev/null || ls -la immichFrame.Web/ 2>/dev/null || find . -type d -name "Web" | head -5Repository: immichFrame/ImmichFrame
Length of output: 704
🏁 Script executed:
# Explore the immichFrame.Web directory structure
ls -la immichFrame.Web/ | head -30Repository: immichFrame/ImmichFrame
Length of output: 1485
🏁 Script executed:
# Find Svelte, HTML, or JS files that might define routes
find immichFrame.Web -type f \( -name "*.svelte" -o -name "*.html" -o -name "*.js" -o -name "*.json" \) | head -30Repository: immichFrame/ImmichFrame
Length of output: 1462
🏁 Script executed:
# Look for route definitions and page files
find immichFrame.Web -type d -name "routes" -o -type d -name "pages" -o -type d -name "src"Repository: immichFrame/ImmichFrame
Length of output: 113
🏁 Script executed:
# Search for "recent-requests" in the codebase
rg "recent-requests" -iRepository: immichFrame/ImmichFrame
Length of output: 401
🏁 Script executed:
# Look for navigation or links in the codebase
rg "href|navigate" -A 1 -B 1 | head -40Repository: immichFrame/ImmichFrame
Length of output: 2288
🏁 Script executed:
# Check if there's a git history for the manifest change
git log --oneline immichFrame.Web/static/manifest.webmanifest | head -10Repository: immichFrame/ImmichFrame
Length of output: 90
🏁 Script executed:
# Check the main page (/) implementation
cat -n immichFrame.Web/src/routes/+page.svelteRepository: immichFrame/ImmichFrame
Length of output: 290
🏁 Script executed:
# Check the recent-requests page implementation
cat -n immichFrame.Web/src/routes/recent-requests/+page.svelteRepository: immichFrame/ImmichFrame
Length of output: 11606
🏁 Script executed:
# Check the layout to see if there's navigation between pages
cat -n immichFrame.Web/src/routes/+layout.svelteRepository: immichFrame/ImmichFrame
Length of output: 575
🏁 Script executed:
# Get more detailed git history for the manifest change
git log --oneline -20 -- immichFrame.Web/static/manifest.webmanifestRepository: immichFrame/ImmichFrame
Length of output: 90
🏁 Script executed:
# Check git diff for this specific change
git show HEAD:immichFrame.Web/static/manifest.webmanifest > /tmp/current_manifest.json 2>/dev/null || echo "Not available"
git show HEAD~1:immichFrame.Web/static/manifest.webmanifest 2>/dev/null || echo "Not available"Repository: immichFrame/ImmichFrame
Length of output: 80
🏁 Script executed:
# Search for any comments or documentation about the PWA start_url
rg "start.url|start_url|PWA" -i -A 2 -B 2Repository: immichFrame/ImmichFrame
Length of output: 5118
Add navigation between / and /recent-requests or document this as intentional.
The start_url change to /recent-requests is a breaking change for PWA users. The recent-requests page has no navigation link back to / (the main photo frame slideshow), trapping users who install the PWA. Either add a navigation link back to the main frame on the recent-requests page, or clarify if this is intended as a separate companion app experience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@immichFrame.Web/static/manifest.webmanifest` at line 4, The PWA start_url in
manifest ("start_url": "/recent-requests") will land users on /recent-requests
with no way back to the main frame; either add explicit navigation from the
recent-requests view to the main slideshow or document the behavior as
intentional. Update the recent-requests page (component/page handling the route
"/recent-requests") to include a visible link/button that routes to "/" (the
main photo frame/slideshow) and ensure it appears in the mobile/installed PWA
layout, or if this is designed as a separate companion app, add a comment in
manifest.webmanifest and project docs stating that start_url="/recent-requests"
is intentional and describe navigation limitations for installers.
Comment on lines
+15
to
+17
| [HttpGet("AssetRequests")] | ||
| [Produces("application/json")] | ||
| public ActionResult<IReadOnlyCollection<AssetRequestRecord>> GetAssetRequests(string? clientIdentifier = null, Guid? assetId = null, int limit = 100) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify route template and usage of expected endpoint paths.
rg -n -C2 '\[Route\("api/\[controller\]"\)\]|\[HttpGet\("AssetRequests"\)\]' ImmichFrame.WebApi/Controllers/AssetRequestsController.cs
rg -n -C2 '/api/AssetRequests(/AssetRequests)?' -g '!**/bin/**' -g '!**/obj/**'Repository: immichFrame/ImmichFrame
Length of output: 1596
🏁 Script executed:
#!/bin/bash
# Search for any calls to /api/AssetRequests (excluding RecentAssetRequests)
# Check tests, frontend, and API client code
rg -n '/api/AssetRequests(?!/RecentAssetRequests)' -g '!**/bin/**' -g '!**/obj/**' --type ts --type js --type cs --type svelte
echo "---"
echo "Also search for any reference to GetAssetRequests method:"
rg -n 'GetAssetRequests' -g '!**/bin/**' -g '!**/obj/**'Repository: immichFrame/ImmichFrame
Length of output: 1272
🏁 Script executed:
#!/bin/bash
# Search for fetch or HTTP calls to AssetRequests endpoint in tests and frontend
echo "=== Search for fetch calls to AssetRequests ==="
rg -n 'fetch.*AssetRequests|GetAsync.*AssetRequests|get.*AssetRequests' -i -g '!**/bin/**' -g '!**/obj/**'
echo ""
echo "=== Check test file for GetAssetRequests test cases ==="
cat ImmichFrame.WebApi.Tests/Controllers/AssetRequestsControllerTests.cs | head -150Repository: immichFrame/ImmichFrame
Length of output: 7736
🏁 Script executed:
#!/bin/bash
# Check for comments or TODOs around the GetAssetRequests method
echo "=== Check for comments around GetAssetRequests in controller ==="
sed -n '10,25p' ImmichFrame.WebApi/Controllers/AssetRequestsController.cs
echo ""
echo "=== Check entire AssetRequestsController.cs for context ==="
wc -l ImmichFrame.WebApi/Controllers/AssetRequestsController.csRepository: immichFrame/ImmichFrame
Length of output: 1108
🏁 Script executed:
#!/bin/bash
# Read the entire controller file to see full context
cat -n ImmichFrame.WebApi/Controllers/AssetRequestsController.csRepository: immichFrame/ImmichFrame
Length of output: 4514
Fix duplicated route segment for the base list endpoint.
With class route api/[controller], [HttpGet("AssetRequests")] maps this action to GET /api/AssetRequests/AssetRequests instead of the conventional GET /api/AssetRequests. This creates an unintuitive, duplicated route path. The endpoint should use [HttpGet] to follow RESTful conventions for collection endpoints.
🔧 Proposed fix
- [HttpGet("AssetRequests")]
+ [HttpGet]
[Produces("application/json")]
public ActionResult<IReadOnlyCollection<AssetRequestRecord>> GetAssetRequests(string? clientIdentifier = null, Guid? assetId = null, int limit = 100)📝 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
| [HttpGet("AssetRequests")] | |
| [Produces("application/json")] | |
| public ActionResult<IReadOnlyCollection<AssetRequestRecord>> GetAssetRequests(string? clientIdentifier = null, Guid? assetId = null, int limit = 100) | |
| [HttpGet] | |
| [Produces("application/json")] | |
| public ActionResult<IReadOnlyCollection<AssetRequestRecord>> GetAssetRequests(string? clientIdentifier = null, Guid? assetId = null, int limit = 100) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.WebApi/Controllers/AssetRequestsController.cs` around lines 15 -
17, The GetAssetRequests action uses an explicit route segment
"[HttpGet(\"AssetRequests\")]" which, combined with the class route
"api/[controller]", produces a duplicated path
(/api/AssetRequests/AssetRequests); change the attribute on the GetAssetRequests
method to a parameterless [HttpGet] so the endpoint maps to the collection route
/api/AssetRequests (ensure the method signature for GetAssetRequests remains
unchanged and remove the extra route string).
Comment on lines
+17
to
+20
| public ActionResult<IReadOnlyCollection<AssetRequestRecord>> GetAssetRequests(string? clientIdentifier = null, Guid? assetId = null, int limit = 100) | ||
| { | ||
| var sanitizedClientIdentifier = clientIdentifier?.SanitizeString() ?? string.Empty; | ||
| return Ok(tracker.GetAssetRequests(sanitizedClientIdentifier, assetId, limit)); |
There was a problem hiding this comment.
Clamp and validate limit on the list endpoint.
limit is passed directly to the tracker; negative or very large values can cause avoidable load/risk.
🔧 Proposed fix
public ActionResult<IReadOnlyCollection<AssetRequestRecord>> GetAssetRequests(string? clientIdentifier = null, Guid? assetId = null, int limit = 100)
{
var sanitizedClientIdentifier = clientIdentifier?.SanitizeString() ?? string.Empty;
- return Ok(tracker.GetAssetRequests(sanitizedClientIdentifier, assetId, limit));
+ var boundedLimit = Math.Clamp(limit, 1, 1000);
+ return Ok(tracker.GetAssetRequests(sanitizedClientIdentifier, assetId, boundedLimit));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.WebApi/Controllers/AssetRequestsController.cs` around lines 17 -
20, The GetAssetRequests endpoint currently forwards the raw limit to
tracker.GetAssetRequests allowing negative or huge values; fix by
validating/clamping limit inside the GetAssetRequests method (e.g., define a
MAX_LIMIT constant and use Math.Clamp or manual logic to ensure limit is within
a safe range like 1..1000) and pass the sanitizedLimit to
tracker.GetAssetRequests; alternatively return BadRequest if limit is <=0 or
exceeds MAX_LIMIT—refer to the GetAssetRequests method, the limit parameter, and
the tracker.GetAssetRequests call when making the change.
Comment on lines
+28
to
+32
| var requests = tracker.GetAssetRequests(sanitizedClientIdentifier, null, 1000); | ||
| var uniqueRecentRequests = requests | ||
| .GroupBy(request => request.AssetId) | ||
| .Select(group => group.First()) | ||
| .Take(Math.Clamp(limit, 1, 1000)); |
There was a problem hiding this comment.
Make “recent” selection deterministic before deduplication.
The query never orders by RequestedAtUtc before GroupBy(...).First(), so the selected record per asset can drift if tracker order changes.
🔧 Proposed fix
var requests = tracker.GetAssetRequests(sanitizedClientIdentifier, null, 1000);
var uniqueRecentRequests = requests
+ .OrderByDescending(request => request.RequestedAtUtc)
.GroupBy(request => request.AssetId)
.Select(group => group.First())
.Take(Math.Clamp(limit, 1, 1000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.WebApi/Controllers/AssetRequestsController.cs` around lines 28 -
32, The selection of a “recent” request is non-deterministic because
uniqueRecentRequests groups the results from tracker.GetAssetRequests and uses
GroupBy(...).First() without ordering; update the pipeline so requests are
ordered by RequestedAtUtc descending before the GroupBy to ensure the First() in
each group is the most recent (i.e. call OrderByDescending(r =>
r.RequestedAtUtc) on the sequence returned from GetAssetRequests prior to
GroupBy, then keep the existing Select(...).Take(...)).
Comment on lines
+33
to
+34
| var enrichedRequests = await Task.WhenAll(uniqueRecentRequests.Select(EnrichAssetRequestAsync)); | ||
| return Ok(enrichedRequests); |
There was a problem hiding this comment.
Throttle enrichment fan-out to protect downstream metadata calls.
Task.WhenAll over all unique requests can issue a large burst of GetAssetInfoById calls at once.
🔧 Proposed fix
- var enrichedRequests = await Task.WhenAll(uniqueRecentRequests.Select(EnrichAssetRequestAsync));
+ const int maxEnrichmentConcurrency = 8;
+ using var semaphore = new System.Threading.SemaphoreSlim(maxEnrichmentConcurrency);
+ var enrichedTasks = uniqueRecentRequests.Select(async request =>
+ {
+ await semaphore.WaitAsync();
+ try
+ {
+ return await EnrichAssetRequestAsync(request);
+ }
+ finally
+ {
+ semaphore.Release();
+ }
+ });
+ var enrichedRequests = await Task.WhenAll(enrichedTasks);
return Ok(enrichedRequests);📝 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
| var enrichedRequests = await Task.WhenAll(uniqueRecentRequests.Select(EnrichAssetRequestAsync)); | |
| return Ok(enrichedRequests); | |
| const int maxEnrichmentConcurrency = 8; | |
| using var semaphore = new System.Threading.SemaphoreSlim(maxEnrichmentConcurrency); | |
| var enrichedTasks = uniqueRecentRequests.Select(async request => | |
| { | |
| await semaphore.WaitAsync(); | |
| try | |
| { | |
| return await EnrichAssetRequestAsync(request); | |
| } | |
| finally | |
| { | |
| semaphore.Release(); | |
| } | |
| }); | |
| var enrichedRequests = await Task.WhenAll(enrichedTasks); | |
| return Ok(enrichedRequests); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.WebApi/Controllers/AssetRequestsController.cs` around lines 33 -
34, The current fan-out uses Task.WhenAll on uniqueRecentRequests which can
flood downstream GetAssetInfoById calls; change the call site in
AssetRequestsController to run EnrichAssetRequestAsync with bounded concurrency
(e.g., a SemaphoreSlim or Parallel.ForEachAsync) instead of Task.WhenAll, using
a configurable degreeOfParallelism (e.g., 5) to throttle concurrent calls, await
each enriched result into a list preserving order, and ensure semaphore disposal
or cancellation token propagation so downstream metadata calls are protected.
Collaborator
|
Hey @zuripig, thanks for the PR! Before I dive into the review.. did you write this code yourself, or was it AI-generated? |
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
2 participants
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.
For a while I’ve wanted an easy way to check the details of a photo that appears on my frame, especially older ones I haven’t seen in years, without walking across the room to read the small text or searching through my library for it.
This change adds a new page you can visit on another device that shows the most recently displayed photos in a simple grid, and tapping a photo opens extra details such as when it was taken, where it was taken, and what camera was used.
Summary by CodeRabbit
Release Notes