Added admin page with basic authentication#615
Closed
iCanHazSkillz wants to merge 6 commits intoimmichFrame:mainfrom
Closed
Added admin page with basic authentication#615iCanHazSkillz wants to merge 6 commits intoimmichFrame:mainfrom
iCanHazSkillz wants to merge 6 commits intoimmichFrame:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds admin HTTP basic login and cookie auth, a frame-session registry with command queuing and persistence, admin and frame-session API endpoints and SPA, client-side frame-session integration, asset-pool constructor refactors, authentication/startup and rate-limiting changes, plus tests, Docker/docs updates, and small null-safety fixes. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant AdminBrowser as Admin Browser
participant AdminSessionCtrl as AdminFrameSessionsController
participant Registry as FrameSessionRegistry
end
AdminBrowser->>AdminSessionCtrl: GET /api/admin/frame-sessions
AdminSessionCtrl->>Registry: GetActiveSessions()
Registry-->>AdminSessionCtrl: [FrameSessionStateDto]
AdminSessionCtrl-->>AdminBrowser: 200 OK + sessions
sequenceDiagram
rect rgba(200,255,200,0.5)
participant AdminBrowser as Admin Browser
participant AdminSessionCtrl as AdminFrameSessionsController
participant Registry as FrameSessionRegistry
participant FrameClient as Frame Client
participant FrameSessionsCtrl as FrameSessionsController
end
AdminBrowser->>AdminSessionCtrl: POST /api/admin/frame-sessions/{id}/commands
AdminSessionCtrl->>Registry: EnqueueCommand(id, type)
Registry-->>AdminSessionCtrl: Enqueued + AdminCommandDto
FrameClient->>FrameSessionsCtrl: GET /api/frame-sessions/{id}/commands
FrameSessionsCtrl->>Registry: GetPendingCommands(id)
Registry-->>FrameSessionsCtrl: [AdminCommandDto]
FrameClient->>FrameSessionsCtrl: POST /api/frame-sessions/{id}/commands/{cmd}/ack
FrameSessionsCtrl->>Registry: AcknowledgeCommand(id, cmd)
Registry-->>FrameSessionsCtrl: true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Nitpick comments (12)
Install_Web.md (1)
131-134: Add explicit TLS guidance for admin auth exposure.This section is clear functionally, but it should explicitly warn that
/adminmust be served behind HTTPS (or a TLS-terminating reverse proxy) when exposed beyond trusted local networks.Suggested docs patch
The frame UI and the admin dashboard are both available on `http://HOST:8080`, with the admin dashboard at `http://HOST:8080/admin`. To enable the admin login page, add at least one matching `IMMICHFRAME_AUTH_BASIC_*_USER` and `IMMICHFRAME_AUTH_BASIC_*_HASH` pair to your environment or `.env` file. These env values remain the source of truth for admin users, and the `/admin` page signs in against them with a normal session cookie. + +> [!IMPORTANT] +> If you expose `/admin` outside a trusted local network, place ImmichFrame behind HTTPS (for example via a reverse proxy with TLS termination).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Install_Web.md` around lines 131 - 134, Update the Install_Web.md paragraph that describes admin access on http://HOST:8080 and the IMMICHFRAME_AUTH_BASIC_*_USER / IMMICHFRAME_AUTH_BASIC_*_HASH env pairs to explicitly warn that the /admin endpoint must only be exposed over HTTPS (or behind a TLS-terminating reverse proxy) when reachable outside trusted local networks, and advise configuring TLS (or a reverse proxy like Nginx/Traefik) because the admin login uses session cookies and basic auth credentials which can be intercepted over plain HTTP.ImmichFrame.WebApi/Program.cs (1)
160-160: Remove or document commented-out code.
UseHttpsRedirectionis commented out. If this is intentional (e.g., for reverse proxy setups), add a comment explaining why. Otherwise, remove the dead code.♻️ Suggested documentation
-// app.UseHttpsRedirection(); +// HTTPS redirection is disabled; TLS termination is expected at the reverse proxy layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Program.cs` at line 160, The commented-out app.UseHttpsRedirection() in Program.cs is dead code—either remove the commented line or, if intentionally disabled for a specific deployment (e.g., reverse proxy or TLS termination), replace it with a short explanatory comment above the line describing the reason and the expected deployment scenario; locate the UseHttpsRedirection occurrence in Program.cs and apply one of these two actions so the intent is explicit.ImmichFrame.WebApi/Controllers/AdminAuthController.cs (1)
36-48: Consider adding null-checks or model validation for login credentials.If
request.Usernameorrequest.Passwordare null/empty, this could pass unexpected values toValidateCredentials. EnsureAdminLoginRequesthas[Required]attributes, or add explicit validation here.Additionally, the login endpoint lacks rate limiting, making it susceptible to brute-force attacks.
🛡️ Suggested defensive check
[HttpPost("login")] [AllowAnonymous] public async Task<ActionResult<AdminAuthSessionDto>> Login([FromBody] AdminLoginRequest request) { + if (string.IsNullOrWhiteSpace(request.Username) || string.IsNullOrWhiteSpace(request.Password)) + { + return Unauthorized("Invalid username or password."); + } + if (!_adminBasicAuthService.HasUsers) { return StatusCode(StatusCodes.Status503ServiceUnavailable, "Admin login is not configured."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AdminAuthController.cs` around lines 36 - 48, The Login action in AdminAuthController uses AdminLoginRequest values directly and calls _adminBasicAuthService.ValidateCredentials without guarding against null/empty Username or Password and has no rate-limiting; add validation to reject missing/empty credentials (either annotate AdminLoginRequest properties with [Required] and validate ModelState in Login, or explicitly check string.IsNullOrWhiteSpace for request.Username/request.Password and return BadRequest), then only call _adminBasicAuthService.ValidateCredentials when credentials are present; also integrate or call an existing rate-limiting middleware/service (or return TooManyRequests when thresholds are exceeded) to protect the Login endpoint from brute-force attempts.ImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.cs (1)
28-56: Test isolation: global environment variables may conflict in parallel test runs.The test modifies process-wide environment variables which could cause flakiness or failures when NUnit runs tests in parallel. Consider using a test fixture with
[NonParallelizable]attribute, or refactorAdminBasicAuthServiceto accept anIDictionaryin its constructor for testing purposes (similar to howLoadUsersalready accepts one).♻️ Suggested approach
[Test] +[NonParallelizable] public void ValidateCredentials_SupportsApacheMd5AndSha1() {Or alternatively, extend the service to accept environment variables via constructor injection for testability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.cs` around lines 28 - 56, The test ValidateCredentials_SupportsApacheMd5AndSha1 mutates process-wide Environment variables which can cause flakiness in parallel runs; fix by either marking the test (or its test class) with [NonParallelizable] to prevent concurrent execution, or refactor AdminBasicAuthService to accept an IDictionary<string,string> (or an abstraction) in its constructor and use the existing LoadUsers overload so the test can inject a local dictionary of credentials instead of setting Environment variables; update the test to construct AdminBasicAuthService with the injected dictionary and remove Environment.SetEnvironmentVariable usage.immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
705-715:beforeunloadwithfetch(..., {keepalive: true})may be unreliable.The
keepaliveoption helps ensure the request completes during page unload, but browser support and behavior vary. For critical disconnect signals,navigator.sendBeaconis more reliable. However, since this is a best-effort disconnect (the server will prune stale sessions anyway), the current approach is acceptable.Alternative using sendBeacon (more reliable)
async function handleBeforeUnload() { if (!$clientIdentifierStore) { return; } - try { - await disconnectFrameSession($clientIdentifierStore, true); - } catch (err) { - console.warn('Failed to disconnect frame session during unload:', err); - } + // sendBeacon is more reliable during page unload + navigator.sendBeacon( + `/api/frame-sessions/${encodeURIComponent($clientIdentifierStore)}/disconnect` + ); }Note:
sendBeacondoesn't support custom headers, so the endpoint would need to accept unauthenticated requests for disconnect, or use a different mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 705 - 715, The beforeunload handler handleBeforeUnload currently calls disconnectFrameSession with fetch(..., {keepalive: true}), which can be unreliable across browsers; update handleBeforeUnload to prefer navigator.sendBeacon for the disconnect when available (pack the client id and any minimal required payload into a Blob or URL-encoded body) and fall back to the existing disconnectFrameSession(fetch with keepalive) for browsers without sendBeacon, and if sendBeacon cannot send required auth headers then change the disconnect endpoint or token mechanism to allow beacon-based unauthenticated disconnects (or include a short-lived token in the beacon payload/URL); reference handleBeforeUnload, disconnectFrameSession, $clientIdentifierStore and navigator.sendBeacon when making the change.immichFrame.Web/src/lib/frameSessionApi.ts (2)
38-44: RedundantdisplayNameproperty inFrameSessionStateDto.
FrameSessionStateDtoextendsFrameSessionSnapshotDtowhich already includesdisplayName. The redefinition on Line 40 is redundant. This doesn't cause functional issues but adds noise.Remove redundant property
export interface FrameSessionStateDto extends FrameSessionSnapshotDto { clientIdentifier: string; - displayName?: string | null; connectedAtUtc: string; lastSeenAtUtc: string; userAgent?: string | null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/frameSessionApi.ts` around lines 38 - 44, Remove the redundant displayName declaration from the interface FrameSessionStateDto since it already extends FrameSessionSnapshotDto which defines displayName; edit the FrameSessionStateDto definition to delete the displayName?: string | null line and run TypeScript build/typing checks to ensure no callers relied on the redeclared property (adjust FrameSessionSnapshotDto if needed).
104-113:getFrameSessionCommandsreturns empty array on failure—consider logging.When the fetch fails or returns non-OK, the function silently returns
[]. While this prevents command processing, it may mask connectivity issues. The caller inhome-page.sveltedoesn't log this scenario.Add logging for failed command fetch
export async function getFrameSessionCommands(clientIdentifier: string) { const response = await fetch( `/api/frame-sessions/${encodeURIComponent(clientIdentifier)}/commands`, { headers: getHeaders() } ); - return response.ok ? readJson<AdminCommandDto[]>(response) : []; + if (!response.ok) { + console.warn(`Failed to fetch commands: ${response.status}`); + return []; + } + return readJson<AdminCommandDto[]>(response); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/frameSessionApi.ts` around lines 104 - 113, The getFrameSessionCommands function currently swallows failures and returns [] silently; modify getFrameSessionCommands so it catches fetch/network errors and logs a clear error (including response.status/text when response.ok is false and the caught exception when fetch throws) before returning an empty array; reference the existing symbols getFrameSessionCommands, fetch call, response.ok, readJson<AdminCommandDto[]> and getHeaders to locate the code, and ensure the caller (home-page.svelte) can still rely on an array return while operators can see failures in the logs (use console.error or the project logger you already use).ImmichFrame.WebApi.Tests/Controllers/FrameSessionsControllerTests.cs (1)
22-79: Environment variable manipulation may cause flaky tests with parallel execution.Modifying global environment variables in
SetUpand restoring them inTearDowncreates shared mutable state. If tests run in parallel, they may interfere with each other. Consider usingWebApplicationFactory.WithWebHostBuilderto inject configuration directly instead of relying on environment variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi.Tests/Controllers/FrameSessionsControllerTests.cs` around lines 22 - 79, Tests mutate global environment variables in Setup/TearDown (Environment.SetEnvironmentVariable) which can cause flaky parallel tests; instead inject those values into the test host via WebApplicationFactory.WithWebHostBuilder so state stays local to the factory. Replace the Environment.SetEnvironmentVariable calls in Setup with builder.ConfigureAppConfiguration(...) (inside WithWebHostBuilder) to add an in-memory or KeyValuePair configuration containing IMMICHFRAME_AUTH_BASIC_ADMIN_USER, IMMICHFRAME_AUTH_BASIC_ADMIN_HASH and IMMICHFRAME_APP_DATA_PATH, remove the corresponding restores in TearDown and avoid global cleanup, and keep existing registrations like services.AddSingleton<IFrameSessionRegistry, FrameSessionRegistry>(); ensure the temp directory is still created and disposed locally without using environment variables.ImmichFrame.WebApi/Services/AdminBasicAuthService.cs (1)
107-116: SHA1 password hashing is weak but acceptable for htpasswd compatibility.The
{SHA}format provides htpasswd compatibility but SHA1 is cryptographically weak for password storage. Since bcrypt ($2) and Apache MD5 ($apr1$) are also supported and stronger, consider documenting that users should prefer bcrypt hashes when configuring credentials.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Services/AdminBasicAuthService.cs` around lines 107 - 116, The code path handling "{SHA}" in AdminBasicAuthService.cs (the block using SHA1.Create() that computes "{SHA}" + Convert.ToBase64String(digest) and compares via FixedTimeEquals) is only for htpasswd compatibility and uses weak SHA1; update the source comment or nearby documentation in the same file to explicitly state that "{SHA}" (SHA1) is weak and advise administrators to prefer stronger formats like bcrypt ("$2") or Apache MD5 ("$apr1$") when configuring passwords, mentioning these token strings so maintainers can find and understand the recommendation quickly.ImmichFrame.WebApi/Controllers/AdminFrameSessionsController.cs (1)
28-45: Consider distinguishing "session not found" from "session stale" in response.
EnqueueCommandreturnsnullboth when the session doesn't exist and when it's stale/stopped (per the registry implementation in context snippet 1). Returning 404 for both cases is acceptable, but if the admin dashboard needs to differentiate these scenarios for UX (e.g., "session expired" vs "session unknown"), consider a different approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AdminFrameSessionsController.cs` around lines 28 - 45, EnqueueCommand currently treats both "session not found" and "session stale" as null from _registry.EnqueueCommand; change the registry method to return a result type (e.g., enum or small DTO like EnqueueResult with Status {NotFound, Stale, Enqueued} and the AdminCommandDto when enqueued) and update AdminFrameSessionsController.EnqueueCommand to inspect that result and return distinct responses (e.g., NotFound() for NotFound, StatusCode(410) or a specific ProblemDetail for Stale, and Ok(command) for Enqueued); update any callers/tests that expect the old null behavior accordingly.ImmichFrame.WebApi/Services/FrameSessionRegistry.cs (2)
154-159: UnnecessaryInterlocked.Incrementinside lock.Since this code is already protected by the
_synclock, the atomic increment is redundant. A simple++_nextCommandIdwould suffice and be clearer.♻️ Suggested simplification
var command = new AdminCommandDto { - CommandId = Interlocked.Increment(ref _nextCommandId), + CommandId = ++_nextCommandId, CommandType = commandType, IssuedAtUtc = now };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs` around lines 154 - 159, The code uses Interlocked.Increment(ref _nextCommandId) while already inside the _sync lock, which is redundant; replace the atomic call with a simple increment of _nextCommandId (e.g., ++_nextCommandId) when constructing the AdminCommandDto so the lock protects the counter update, keeping the rest of the block (creating the AdminCommandDto with CommandId, CommandType, IssuedAtUtc) unchanged.
326-331: Consider logging file load failures for debugging.Silently ignoring exceptions is appropriate for resilience, but logging a warning would help administrators diagnose configuration issues (e.g., permission problems, malformed JSON).
♻️ Suggested improvement
Inject
ILogger<FrameSessionRegistry>and log a warning:catch + catch (Exception ex) { - // Ignore a corrupted name-store file and continue with an empty in-memory cache. + // Log but continue with an empty in-memory cache so the app remains functional. + _logger?.LogWarning(ex, "Failed to load persisted display names from {Path}", path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs` around lines 326 - 331, The empty catch in FrameSessionRegistry that swallows exceptions when loading the name-store should log a warning; inject an ILogger<FrameSessionRegistry> into the FrameSessionRegistry (add a constructor parameter and a private readonly _logger field) and replace the bare catch with catch(Exception ex) { _logger?.LogWarning(ex, "Failed to load name-store file, continuing with empty in-memory cache."); } so administrators get diagnostic details without changing the current resiliency behavior.
🤖 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/App_Data/admin-account.json`:
- Around line 1-5: The committed admin credential artifact admin-account.json
must be removed from the repository and excluded going forward: delete the file
from the repo history (e.g., remove it and commit the deletion, or use git rm
--cached then commit) and add a rule to .gitignore to prevent re-adding it; do
not change authentication logic in
ImmichFrame.WebApi/Services/AdminBasicAuthService.cs (the service reads
credentials from IMMICHFRAME_AUTH_BASIC_* environment variables), but ensure no
code references or relies on admin-account.json and, if necessary, remove any
tests or config that expect it so only env-based credentials are used.
In `@ImmichFrame.WebApi/App_Data/frame-session-display-names.json`:
- Around line 1-5: Remove environment-specific session metadata from
frame-session-display-names.json: the concrete session IDs (e.g.
"1b8ebb4859c34bce8df8e713971758ff", "1cda1299cb0b4015869b12903ce6840e",
"bcd063f97ef043b4aed41fd0dcacf85b") and user-facing names ("Michel's Office",
"Vivaldi", "Grandma's Frame") must not be committed; replace the file content
with a non-sensitive template or an empty JSON object (e.g. {}), add
frame-session-display-names.json to .gitignore so runtime state isn't tracked,
and if this data already leaked in history consider removing it from the repo
history or rotating any related secrets.
In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs`:
- Around line 238-251: The CloneDisplayEvent method can throw a
NullReferenceException when displayEvent.Assets is null; update
CloneDisplayEvent to defensively handle a null Assets by using a null-coalescing
fallback before calling Select/ToList (e.g., use displayEvent.Assets ??
Enumerable.Empty<...>() and then Select(CloneDisplayedAsset).ToList()) so Assets
on the returned DisplayEventDto is never null; reference the CloneDisplayEvent
method, the DisplayEventDto type, the Assets property, and the
CloneDisplayedAsset helper when making this change.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 705-715: The beforeunload handler handleBeforeUnload currently
calls disconnectFrameSession with fetch(..., {keepalive: true}), which can be
unreliable across browsers; update handleBeforeUnload to prefer
navigator.sendBeacon for the disconnect when available (pack the client id and
any minimal required payload into a Blob or URL-encoded body) and fall back to
the existing disconnectFrameSession(fetch with keepalive) for browsers without
sendBeacon, and if sendBeacon cannot send required auth headers then change the
disconnect endpoint or token mechanism to allow beacon-based unauthenticated
disconnects (or include a short-lived token in the beacon payload/URL);
reference handleBeforeUnload, disconnectFrameSession, $clientIdentifierStore and
navigator.sendBeacon when making the change.
In `@immichFrame.Web/src/lib/frameSessionApi.ts`:
- Around line 38-44: Remove the redundant displayName declaration from the
interface FrameSessionStateDto since it already extends FrameSessionSnapshotDto
which defines displayName; edit the FrameSessionStateDto definition to delete
the displayName?: string | null line and run TypeScript build/typing checks to
ensure no callers relied on the redeclared property (adjust
FrameSessionSnapshotDto if needed).
- Around line 104-113: The getFrameSessionCommands function currently swallows
failures and returns [] silently; modify getFrameSessionCommands so it catches
fetch/network errors and logs a clear error (including response.status/text when
response.ok is false and the caught exception when fetch throws) before
returning an empty array; reference the existing symbols
getFrameSessionCommands, fetch call, response.ok, readJson<AdminCommandDto[]>
and getHeaders to locate the code, and ensure the caller (home-page.svelte) can
still rely on an array return while operators can see failures in the logs (use
console.error or the project logger you already use).
In `@ImmichFrame.WebApi.Tests/Controllers/FrameSessionsControllerTests.cs`:
- Around line 22-79: Tests mutate global environment variables in Setup/TearDown
(Environment.SetEnvironmentVariable) which can cause flaky parallel tests;
instead inject those values into the test host via
WebApplicationFactory.WithWebHostBuilder so state stays local to the factory.
Replace the Environment.SetEnvironmentVariable calls in Setup with
builder.ConfigureAppConfiguration(...) (inside WithWebHostBuilder) to add an
in-memory or KeyValuePair configuration containing
IMMICHFRAME_AUTH_BASIC_ADMIN_USER, IMMICHFRAME_AUTH_BASIC_ADMIN_HASH and
IMMICHFRAME_APP_DATA_PATH, remove the corresponding restores in TearDown and
avoid global cleanup, and keep existing registrations like
services.AddSingleton<IFrameSessionRegistry, FrameSessionRegistry>(); ensure the
temp directory is still created and disposed locally without using environment
variables.
In `@ImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.cs`:
- Around line 28-56: The test ValidateCredentials_SupportsApacheMd5AndSha1
mutates process-wide Environment variables which can cause flakiness in parallel
runs; fix by either marking the test (or its test class) with
[NonParallelizable] to prevent concurrent execution, or refactor
AdminBasicAuthService to accept an IDictionary<string,string> (or an
abstraction) in its constructor and use the existing LoadUsers overload so the
test can inject a local dictionary of credentials instead of setting Environment
variables; update the test to construct AdminBasicAuthService with the injected
dictionary and remove Environment.SetEnvironmentVariable usage.
In `@ImmichFrame.WebApi/Controllers/AdminAuthController.cs`:
- Around line 36-48: The Login action in AdminAuthController uses
AdminLoginRequest values directly and calls
_adminBasicAuthService.ValidateCredentials without guarding against null/empty
Username or Password and has no rate-limiting; add validation to reject
missing/empty credentials (either annotate AdminLoginRequest properties with
[Required] and validate ModelState in Login, or explicitly check
string.IsNullOrWhiteSpace for request.Username/request.Password and return
BadRequest), then only call _adminBasicAuthService.ValidateCredentials when
credentials are present; also integrate or call an existing rate-limiting
middleware/service (or return TooManyRequests when thresholds are exceeded) to
protect the Login endpoint from brute-force attempts.
In `@ImmichFrame.WebApi/Controllers/AdminFrameSessionsController.cs`:
- Around line 28-45: EnqueueCommand currently treats both "session not found"
and "session stale" as null from _registry.EnqueueCommand; change the registry
method to return a result type (e.g., enum or small DTO like EnqueueResult with
Status {NotFound, Stale, Enqueued} and the AdminCommandDto when enqueued) and
update AdminFrameSessionsController.EnqueueCommand to inspect that result and
return distinct responses (e.g., NotFound() for NotFound, StatusCode(410) or a
specific ProblemDetail for Stale, and Ok(command) for Enqueued); update any
callers/tests that expect the old null behavior accordingly.
In `@ImmichFrame.WebApi/Program.cs`:
- Line 160: The commented-out app.UseHttpsRedirection() in Program.cs is dead
code—either remove the commented line or, if intentionally disabled for a
specific deployment (e.g., reverse proxy or TLS termination), replace it with a
short explanatory comment above the line describing the reason and the expected
deployment scenario; locate the UseHttpsRedirection occurrence in Program.cs and
apply one of these two actions so the intent is explicit.
In `@ImmichFrame.WebApi/Services/AdminBasicAuthService.cs`:
- Around line 107-116: The code path handling "{SHA}" in
AdminBasicAuthService.cs (the block using SHA1.Create() that computes "{SHA}" +
Convert.ToBase64String(digest) and compares via FixedTimeEquals) is only for
htpasswd compatibility and uses weak SHA1; update the source comment or nearby
documentation in the same file to explicitly state that "{SHA}" (SHA1) is weak
and advise administrators to prefer stronger formats like bcrypt ("$2") or
Apache MD5 ("$apr1$") when configuring passwords, mentioning these token strings
so maintainers can find and understand the recommendation quickly.
In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs`:
- Around line 154-159: The code uses Interlocked.Increment(ref _nextCommandId)
while already inside the _sync lock, which is redundant; replace the atomic call
with a simple increment of _nextCommandId (e.g., ++_nextCommandId) when
constructing the AdminCommandDto so the lock protects the counter update,
keeping the rest of the block (creating the AdminCommandDto with CommandId,
CommandType, IssuedAtUtc) unchanged.
- Around line 326-331: The empty catch in FrameSessionRegistry that swallows
exceptions when loading the name-store should log a warning; inject an
ILogger<FrameSessionRegistry> into the FrameSessionRegistry (add a constructor
parameter and a private readonly _logger field) and replace the bare catch with
catch(Exception ex) { _logger?.LogWarning(ex, "Failed to load name-store file,
continuing with empty in-memory cache."); } so administrators get diagnostic
details without changing the current resiliency behavior.
In `@Install_Web.md`:
- Around line 131-134: Update the Install_Web.md paragraph that describes admin
access on http://HOST:8080 and the IMMICHFRAME_AUTH_BASIC_*_USER /
IMMICHFRAME_AUTH_BASIC_*_HASH env pairs to explicitly warn that the /admin
endpoint must only be exposed over HTTPS (or behind a TLS-terminating reverse
proxy) when reachable outside trusted local networks, and advise configuring TLS
(or a reverse proxy like Nginx/Traefik) because the admin login uses session
cookies and basic auth credentials which can be intercepted over plain HTTP.
🪄 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: 6aee8b56-471b-490b-b03a-f1b2cbf61f5e
⛔ Files ignored due to path filters (1)
immichFrame.Web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
Directory.Packages.propsDockerfileImmichFrame.Core.Tests/ImmichFrame.Core.Tests.csprojImmichFrame.Core/Api/AssetResponseDto.csImmichFrame.Core/Helpers/ApiCache.csImmichFrame.Core/Interfaces/IApiCache.csImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.csImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.csImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.Core/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Services/OpenWeatherMapService.csImmichFrame.WebApi.Tests/Controllers/FrameSessionsControllerTests.csImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csprojImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.csImmichFrame.WebApi.Tests/Services/FrameSessionRegistryTests.csImmichFrame.WebApi/App_Data/admin-account.jsonImmichFrame.WebApi/App_Data/frame-session-display-names.jsonImmichFrame.WebApi/Controllers/AdminAuthController.csImmichFrame.WebApi/Controllers/AdminFrameSessionsController.csImmichFrame.WebApi/Controllers/AssetController.csImmichFrame.WebApi/Controllers/CalendarController.csImmichFrame.WebApi/Controllers/FrameSessionsController.csImmichFrame.WebApi/Controllers/WeatherController.csImmichFrame.WebApi/ImmichFrame.WebApi.csprojImmichFrame.WebApi/Models/AdminAuthDtos.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/FrameSessionDtos.csImmichFrame.WebApi/Models/ServerSettings.csImmichFrame.WebApi/Program.csImmichFrame.WebApi/Services/AdminBasicAuthService.csImmichFrame.WebApi/Services/AuthSchemes.csImmichFrame.WebApi/Services/FrameSessionRegistry.csImmichFrame.WebApi/Services/IFrameSessionRegistry.csInstall_Web.mddocker/docker-compose.dev.ymldocker/docker-compose.ymldocker/example.envimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/frameSessionApi.tsimmichFrame.Web/src/lib/stores/persist.store.tsimmichFrame.Web/src/routes/+page.tsimmichFrame.Web/src/routes/admin/+page.svelte
Collaborator
|
This sounds neat. I’ll try to check it out tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 96-105: The heartbeat handler is re-publishing the cached display
name on every tick which can overwrite admin renames; change the logic so
clientNameStore (and similarly clientIdentifierStore) are only
initialized/updated from the URL or localStorage once (or when the URL actually
changes) and do not get included in the regular heartbeat sync unless they have
been explicitly changed locally since last sync; locate uses of clientName,
clientNameStore, clientIdentifier, clientIdentifierStore and the heartbeat
function and: 1) stop writing the stores on every heartbeat, 2) add a
"dirty/changed" flag or compare to the registry value before including
clientName in the payload, and 3) ensure the heartbeat path only persists when a
true local change was made.
In `@immichFrame.Web/src/lib/frameSessionApi.ts`:
- Around line 92-100: The fetch call in putFrameSessionSnapshot currently
returns the raw Response (in function putFrameSessionSnapshot) so HTTP 4xx/5xx
are treated as success; update the implementation to await fetch(...), then pass
the Response to the existing throwIfNotOk(response) helper (used elsewhere in
this file) and return the result or rethrow so callers can catch HTTP errors;
apply the same change to the other similar helper(s) around lines 127-131 (e.g.,
the function handling frame session command ACKs) so all frame API helpers
validate responses with throwIfNotOk instead of returning raw fetch Responses.
In `@ImmichFrame.WebApi/Helpers/ImmichFrameAuthenticationHandler.cs`:
- Around line 44-48: RequiresAuthentication currently treats only null
_authenticationSecret as "auth disabled", so empty or whitespace secrets still
enable auth; update the method RequiresAuthentication to treat blank secrets as
disabled by checking string.IsNullOrWhiteSpace(_authenticationSecret) (or
_authenticationSecret?.Trim().Length > 0) instead of a null check, keeping the
existing endpoint/authorizeAttribute logic (Context.GetEndpoint() and
Metadata.GetMetadata<IAuthorizeData>()) so endpoints with AuthorizeData only
require auth when a non-blank _authenticationSecret is configured.
In `@ImmichFrame.WebApi/Program.cs`:
- Around line 108-119: Middleware ordering issue: call app.UseForwardedHeaders()
before any middleware that relies on request context (notably UseRateLimiter and
authentication/cookie handling) so RemoteIpAddress and SameAsRequest cookie
behavior are correct behind reverse proxies. Locate Program.cs where
AddRateLimiter/UseRateLimiter and authentication are registered (symbols:
AddRateLimiter, UseRateLimiter, UseAuthentication, UseAuthorization, and cookie
SameAsRequest usage) and move or insert a call to UseForwardedHeaders() (with
appropriate ForwardedHeadersOptions configured if needed) prior to
UseRateLimiter() and before authentication middleware so rate-limiting
partitions and cookie Secure/SameAsRequest decisions use the forwarded client IP
and scheme.
In `@ImmichFrame.WebApi/Services/AdminBasicAuthService.cs`:
- Around line 35-53: LoadUsers currently retains multiple entries that can
resolve to the same trimmed username, causing non-deterministic behavior in
ValidateCredentials and GetCredentialVersion; update LoadUsers to trim usernames
and check for duplicates (based on the resulting Username) and reject them by
throwing a clear exception (e.g., InvalidOperationException) mentioning the
duplicate username and the IMMICHFRAME_AUTH_BASIC_* source, so _users never
contains duplicate Username entries and both ValidateCredentials,
GetCredentialVersion, VerifyPassword and ComputeCredentialVersion operate
deterministically.
- Around line 99-104: VerifyPassword currently calls BCrypt.Net.BCrypt.Verify
when hash.StartsWith("$2") but a malformed bcrypt hash can throw
SaltParseException and cause a 500; wrap the BCrypt.Net.BCrypt.Verify call
inside a try-catch that catches SaltParseException (or Exception if
SaltParseException is not referenced) and returns false on any parse error so
malformed hashes are treated as failed authentication, keeping the existing
prefix check and overall fail-closed behavior.
In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs`:
- Around line 68-73: Guard against snapshot.History being null before
enumerating: when assigning session.History use a null-coalescing fallback
(treating FrameSessionSnapshotDto.History as empty) so
Take(_options.MaxHistoryItems).Select(CloneDisplayEvent)... won't throw; locate
the block that reads snapshot.History and ensure you replace the direct
enumeration with a null-safe sequence (e.g., use (snapshot.History ??
Enumerable.Empty<...>()) or equivalent) and preserve the existing
CloneDisplayEvent, Where(x => x != null), Cast<DisplayEventDto>() and ToList()
logic.
🪄 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: f30be890-983d-40b0-a2a9-0de27030cbae
📒 Files selected for processing (16)
.gitignoreImmichFrame.WebApi.Tests/Controllers/FrameSessionsControllerTests.csImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.csImmichFrame.WebApi.Tests/Services/FrameSessionRegistryTests.csImmichFrame.WebApi/Controllers/AdminAuthController.csImmichFrame.WebApi/Controllers/AdminFrameSessionsController.csImmichFrame.WebApi/Controllers/FrameSessionsController.csImmichFrame.WebApi/Helpers/ImmichFrameAuthenticationHandler.csImmichFrame.WebApi/Models/FrameSessionDtos.csImmichFrame.WebApi/Program.csImmichFrame.WebApi/Services/AdminBasicAuthService.csImmichFrame.WebApi/Services/FrameSessionRegistry.csImmichFrame.WebApi/Services/IFrameSessionRegistry.csInstall_Web.mdimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/frameSessionApi.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- ImmichFrame.WebApi/Controllers/FrameSessionsController.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Install_Web.md
- ImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.cs
- ImmichFrame.WebApi.Tests/Controllers/FrameSessionsControllerTests.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
immichFrame.Web/src/lib/frameSessionApi.ts (1)
145-156: Consider addingthrowIfNotOkfor consistency with other API functions.
disconnectFrameSessionreturns the rawResponsewithout status validation, unlike other functions in this module. While the primary use case (unload handler) may tolerate failures, callers expecting consistent error semantics could miss HTTP errors.If fire-and-forget is intentional for this endpoint, consider documenting that in a comment.
💡 Optional: Add error handling for consistency
export async function disconnectFrameSession( clientIdentifier: string, keepalive = false, authSecret?: string | null ) { - return fetch(`/api/frame-sessions/${encodeURIComponent(clientIdentifier)}/disconnect`, { + const response = await fetch(`/api/frame-sessions/${encodeURIComponent(clientIdentifier)}/disconnect`, { method: 'POST', headers: authSecret ? getHeaders(true) : getHeaders(), body: authSecret ? JSON.stringify({ authSecret }) : undefined, keepalive }); + // In keepalive/unload scenarios, errors may not be actionable + if (!keepalive) { + throwIfNotOk(response, `Failed to disconnect frame session: ${response.status}`); + } + return response; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/frameSessionApi.ts` around lines 145 - 156, disconnectFrameSession currently returns the raw fetch Response (in contrast to other API helpers that call throwIfNotOk), so update the function to await the fetch, call throwIfNotOk(response) to surface HTTP errors consistently, then return the response (or void) as appropriate; locate disconnectFrameSession and replace the direct return of fetch(...) with an awaited fetch, call to throwIfNotOk(response) and then return the response, using the same header logic (getHeaders) and body handling already present.ImmichFrame.WebApi/Services/AdminBasicAuthService.cs (1)
174-287: Custom Apache MD5 implementation appears correct but consider documenting the source.The
ApacheMd5Cryptimplementation follows the standard APR1 algorithm. Consider adding a brief comment citing the reference implementation (e.g., Apache httpd'sapr_md5.cor the libccryptspecification) to aid future maintainers in verifying correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Services/AdminBasicAuthService.cs` around lines 174 - 287, Add a short comment at the top of the ApacheMd5Crypt class citing the reference implementation and source (e.g., Apache httpd apr_md5.c and/or POSIX crypt APR1 spec) and include any relevant license/attribution note; mention the primary functions implementing the algorithm (Hash, Md5, ExtractSalt, To64) so maintainers can cross-check behavior against the referenced implementation. Keep the comment concise (1-3 lines) and optionally include a URL to the source repo/file for easy verification.ImmichFrame.WebApi/Program.cs (1)
109-112: Consider documenting proxy trust configuration for production deployments.
ForwardedHeadersOptionsis configured withoutKnownProxiesorKnownNetworks, which means all proxies are trusted. While this works for single-proxy deployments, consider adding a comment or documentation about restricting proxy trust in production environments where IP spoofing is a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Program.cs` around lines 109 - 112, The ForwardedHeadersOptions configuration in Program.cs currently trusts all proxies (options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto) which can allow IP spoofing in multi-proxy or untrusted environments; update the code or add a clear inline comment and docs explaining that production deployments should restrict trusted proxies by setting options.KnownProxies and/or options.KnownNetworks (or explicitly document when it’s safe to leave them empty), and show where to populate those lists (e.g., via configuration values used by Configure<ForwardedHeadersOptions>), so operators know to lock down proxy trust for production.ImmichFrame.WebApi/Services/FrameSessionRegistry.cs (1)
348-367: Consider adding error handling for display name persistence failures.Unlike
LoadPersistedDisplayNames,SavePersistedDisplayNamesUnsafehas no try-catch. An I/O failure (e.g., disk full, permission denied) will propagate up and could disrupt session upserts. Consider logging a warning and continuing rather than failing the operation.🛡️ Proposed fix to add defensive error handling
private void SavePersistedDisplayNamesUnsafe() { var path = _options.DisplayNameStorePath; if (string.IsNullOrWhiteSpace(path)) { return; } + try + { var directory = Path.GetDirectoryName(path); if (!string.IsNullOrWhiteSpace(directory)) { Directory.CreateDirectory(directory); } var json = JsonSerializer.Serialize(_persistedDisplayNames, new JsonSerializerOptions { WriteIndented = true }); File.WriteAllText(path, json); + } + catch (Exception ex) + { + _logger?.LogWarning(ex, "Failed to persist display names to {Path}, in-memory state is still valid.", path); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs` around lines 348 - 367, Wrap the body of SavePersistedDisplayNamesUnsafe in a try-catch so I/O failures don't bubble up and disrupt session upserts: catch Exception (or IOException) around the existing logic that uses _options.DisplayNameStorePath, Directory.CreateDirectory, and File.WriteAllText, log a warning including the path and exception details (use the class logger field, e.g. _logger) and return/continue without rethrowing; keep the existing serialization of _persistedDisplayNames and WriteIndented option but ensure failures are logged and swallowed.
🤖 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.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 668-673: The break after the continue in the 'Shutdown' case is
unreachable; in the switch/case block handling 'Shutdown' (which calls
acknowledgeFrameSessionCommand(...) and shutdownFromAdmin()), remove the stray
`break` that follows `continue` so control flow is clear—keep the await calls
and the `continue` but delete the redundant `break`.
---
Nitpick comments:
In `@immichFrame.Web/src/lib/frameSessionApi.ts`:
- Around line 145-156: disconnectFrameSession currently returns the raw fetch
Response (in contrast to other API helpers that call throwIfNotOk), so update
the function to await the fetch, call throwIfNotOk(response) to surface HTTP
errors consistently, then return the response (or void) as appropriate; locate
disconnectFrameSession and replace the direct return of fetch(...) with an
awaited fetch, call to throwIfNotOk(response) and then return the response,
using the same header logic (getHeaders) and body handling already present.
In `@ImmichFrame.WebApi/Program.cs`:
- Around line 109-112: The ForwardedHeadersOptions configuration in Program.cs
currently trusts all proxies (options.ForwardedHeaders =
ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto) which can
allow IP spoofing in multi-proxy or untrusted environments; update the code or
add a clear inline comment and docs explaining that production deployments
should restrict trusted proxies by setting options.KnownProxies and/or
options.KnownNetworks (or explicitly document when it’s safe to leave them
empty), and show where to populate those lists (e.g., via configuration values
used by Configure<ForwardedHeadersOptions>), so operators know to lock down
proxy trust for production.
In `@ImmichFrame.WebApi/Services/AdminBasicAuthService.cs`:
- Around line 174-287: Add a short comment at the top of the ApacheMd5Crypt
class citing the reference implementation and source (e.g., Apache httpd
apr_md5.c and/or POSIX crypt APR1 spec) and include any relevant
license/attribution note; mention the primary functions implementing the
algorithm (Hash, Md5, ExtractSalt, To64) so maintainers can cross-check behavior
against the referenced implementation. Keep the comment concise (1-3 lines) and
optionally include a URL to the source repo/file for easy verification.
In `@ImmichFrame.WebApi/Services/FrameSessionRegistry.cs`:
- Around line 348-367: Wrap the body of SavePersistedDisplayNamesUnsafe in a
try-catch so I/O failures don't bubble up and disrupt session upserts: catch
Exception (or IOException) around the existing logic that uses
_options.DisplayNameStorePath, Directory.CreateDirectory, and File.WriteAllText,
log a warning including the path and exception details (use the class logger
field, e.g. _logger) and return/continue without rethrowing; keep the existing
serialization of _persistedDisplayNames and WriteIndented option but ensure
failures are logged and swallowed.
🪄 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: de3c818b-f9b9-4265-aead-5cacefdf1af3
📒 Files selected for processing (8)
ImmichFrame.WebApi.Tests/Services/AdminBasicAuthServiceTests.csImmichFrame.WebApi.Tests/Services/FrameSessionRegistryTests.csImmichFrame.WebApi/Helpers/ImmichFrameAuthenticationHandler.csImmichFrame.WebApi/Program.csImmichFrame.WebApi/Services/AdminBasicAuthService.csImmichFrame.WebApi/Services/FrameSessionRegistry.csimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/frameSessionApi.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 133-135: The close handlers currently always call
resumePlayback(), which clears a user's manual pause; change them to only resume
when the overlay itself paused playback: introduce or use a boolean flag (e.g.,
infoPaused or overlayPaused) that you set when the info overlay opens and you
call pausePlayback(), and then in provideClose() and the other close handler
(the close path around lines 602-617) call resumePlayback() only if that flag is
true; finally reset the flag when closing so manual pauses remain respected.
- Around line 634-684: In processPendingCommands, ensure each remote command
runs exactly-once by tracking processed IDs: add a Set (e.g., handledCommandIds)
and skip any command whose command.commandId is in the set; mark a command ID as
handled before executing its action (so it won’t be re-executed if ack fails)
and only remove it when acknowledgeFrameSessionCommand($clientIdentifierStore,
command.commandId) succeeds; for errors during the action, do not remove the ID
so retries won’t re-run the action, and change the Shutdown branch to await
acknowledgeFrameSessionCommand, await shutdownFromAdmin(), and then return
immediately (instead of continue) to stop processing the rest of the batch.
- Around line 222-255: syncFrameSession can overlap and overwrite newer state;
serialize PUTs by tracking a single in-flight request and coalescing one pending
resend. Add an isSyncInFlight boolean and a pendingSync boolean (or similar)
near syncFrameSession; at the top of syncFrameSession return early but set
pendingSync=true when isSyncInFlight is true. When starting the PUT set
isSyncInFlight=true, and in the finally block set isSyncInFlight=false and, if
pendingSync was set, clear it and immediately re-run syncFrameSession() (await
the retry) so only one extra coalesced request runs. Update any call sites
(e.g., startSessionLoops calling syncFrameSession) to keep calling the same
function without changing their semantics.
🪄 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: 6f68e9af-bce6-4e04-aa0d-607ec95fec3b
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 764-767: The startup sequence currently calls startSessionLoops()
before the initial asset bootstrap finishes, which risks race conditions; change
the order so that getNextAssets() is awaited/completed first, then call
syncFrameSession(), and only after that startSessionLoops() (i.e., move
startSessionLoops() to run after the promise returned by getNextAssets()
resolves). Update the calls around startSessionLoops(), getNextAssets(), and
syncFrameSession() to ensure getNextAssets() completes before enabling command
polling or session loops.
🪄 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: f2403575-5949-4297-90e7-f591a004c254
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
Collaborator
|
Thanks for sharing this idea - a remote admin page is an interesting concept worth exploring. That said, you mentioned this was "entirely vibe coded". As a maintainer contributing my free time, I am not in a position to properly audit AI-generated code of this size. I will therefore not be reviewing this PR. |
Contributor
Author
|
That is completely understandable. I hope this is something we can see properly implemented in immichframe sometime in the near future. Thanks Jan. |
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
3 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.
I've added an admin page accessible through http:{ImmichFrameURL}/admin.
The purpose of this page is to provide the ability to manage frames remotely. An admin can view any active session, change the media displayed (previous, play/pause, next), refresh or shutdown the session altogether. The currently displayed media can be viewed along with an history or recently displayed media which when clicked, opens the corresponding media page on the user's immich server to further manage. Frame are initially named by ID (Frame-ID), but users can rename frame. An app-data directory must be mounted as volume to allow for the frame naming to persist through app rebuild and docker container recreation .
The admin page is protected by basic browser authentication and an Htpasswd compliant hash. To enable the admin login page, add at least one matching
IMMICHFRAME_AUTH_BASIC_*_USERandIMMICHFRAME_AUTH_BASIC_*_HASHpair to your environment or.envfile. These env values remain the source of truth for admin users, and the/adminpage signs in against them with a normal session cookie. If these variables aren't declared, immichframe works as usual, but /admin is inaccessible.I understand this is a comically large pull request and honestly, this was entirely vibe coded in a couple days, so I completely understand the reservation in implementing this into the main project branch. Think of this as just an idea of what could be. I do think it would be neat to have the ability to manage frames without having to SSH into servers and tinker with .env files and container recreation. I would love to see this admin page evolve in a manner where the .env file would only contain the required configurations for ImmichFrame to run (ImmichServerUrl + ApiKey + AdminAuth) while all other variables would be configured live within the UI through the admin page.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores