Skip to content

Conversation

@chuckb7
Copy link

@chuckb7 chuckb7 commented Jan 7, 2026

Added parameter BaseUrl to example.env and documentation
Added entrypoint.sh script to update files prior to running ImmichFrame .net
Return 404 for any requests that don't match BaseUrl

This is linked to Issue #546

This was done vibe coding with AI as this is not my area of expertise. I have tested basic functionality / failure scenarios.

Summary by CodeRabbit

  • New Features

    • Added BaseUrl setting so the app can run under a custom subpath; server enforces and client initializes it.
  • Bug Fixes / UI

    • Asset URLs, manifest and service worker paths now respect the configured base path for reliable loading.
  • Chores

    • Container startup now computes and applies the runtime base path for subpath deployments.
  • Tests

    • Added tests validating environment-driven BaseUrl mapping.
  • Documentation

    • Documented BaseUrl usage for reverse-proxy deployments.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Adds BaseUrl support across config, env mapping, Docker runtime asset substitution via an entrypoint, server PathBase middleware, and frontend SvelteKit base-path wiring and asset placeholders.

Changes

Cohort / File(s) Summary
Docker & Deployment
Dockerfile, docker/example.env, docker/Settings.example.json, docker/Settings.example.yml
Add BaseUrl examples; Dockerfile now creates /app/entrypoint.sh, copies wwwroot to /app/wwwroot-runtime, replaces __IMMICH_FRAME_BASE__ placeholders at container start, symlinks runtime assets, adjusts ownership, and sets ENTRYPOINT ["/app/entrypoint.sh"].
Config Interfaces & Models
ImmichFrame.Core/Interfaces/IServerSettings.cs, ImmichFrame.WebApi/Models/ServerSettings.cs, ImmichFrame.WebApi/Models/ClientSettingsDto.cs
Add nullable BaseUrl to IGeneralSettings, GeneralSettings, and ClientSettingsDto; default "/" handling and validation/normalization added.
Server Settings V1 Adapter
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
Add BaseUrl to ServerSettingsV1; expose via Settings and GeneralSettingsV1Adapter so legacy v1 config exposes BaseUrl.
Configuration Loading & Tests
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs, ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs, ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj
Add ApplyEnvironmentVariables(IServerSettings) and MapDictionaryToConfig<T> (strips surrounding quotes, maps env vars into config shapes); LoadConfig invokes this pass; add tests for env-var mapping; replace AwesomeAssertions with FluentAssertions.
Web API Runtime
ImmichFrame.WebApi/Program.cs
Load .env in dev; read IGeneralSettings.BaseUrl, trim trailing slash; when set and not / call app.UsePathBase(baseUrl), add middleware to 404 requests not matching base, ensure UseRouting() when base set; make Program a public partial.
Frontend & Assets
immichFrame.Web/svelte.config.js, immichFrame.Web/src/app.html, immichFrame.Web/src/routes/+page.ts, immichFrame.Web/src/lib/immichFrameApi.ts, immichFrame.Web/static/manifest.webmanifest
Set kit.paths.base = '/__IMMICH_FRAME_BASE__'; switch asset and service-worker paths to relative; add optional baseUrl to client type; client normalizes/sets baseUrl at runtime; manifest uses __IMMICH_FRAME_BASE__ placeholders.
Docs
docs/docs/getting-started/configuration.md
Document new BaseUrl General setting (default /) and reverse-proxy usage notes.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Docker
    participant Entrypoint as /app/entrypoint.sh
    participant RuntimeAssets as wwwroot-runtime
    participant DotNet as ImmichFrame.WebApi.dll
    participant Config as ConfigLoader
    participant Program as Program.cs
    participant Browser as SvelteKit/Browser

    Docker->>Entrypoint: container starts
    Entrypoint->>Entrypoint: compute BASE_PATH from env or Settings.json/yml
    Entrypoint->>RuntimeAssets: copy /app/wwwroot -> /app/wwwroot-runtime
    Entrypoint->>RuntimeAssets: replace "__IMMICH_FRAME_BASE__" placeholders
    Entrypoint->>Entrypoint: symlink runtime -> /app/wwwroot
    Entrypoint->>DotNet: exec dotnet ImmichFrame.WebApi.dll

    DotNet->>Config: LoadConfig()
    Config->>Config: ApplyEnvironmentVariables() -> set BaseUrl
    Config-->>DotNet: config with BaseUrl

    DotNet->>Program: bootstrap app
    Program->>Program: read BaseUrl from IGeneralSettings
    alt BaseUrl set and not "/"
        Program->>Program: app.UsePathBase(baseUrl)
        Program->>Program: middleware -> 404 if request path doesn't start with base
        Program->>Program: UseRouting()
    end
    Program->>Program: register endpoints

    Browser->>Program: request /[BaseUrl]/...
    Program->>Program: validate path -> route or 404
    Browser->>Browser: fetch config -> setBaseUrl(config.baseUrl) -> resolve assets/routes
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Suggested labels

enhancement

Suggested reviewers

  • JW-CH
  • 3rob3

Poem

🐰 I hopped through configs, placeholders, and art,
Replaced absolute paths with a relative heart,
Docker stitches base, .NET steers the way,
Svelte and browser follow the rewritten bay,
A happy rabbit cheers: routes aligned today! 🌱

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feature/base url parameter' is directly related to the main changeset, which adds BaseUrl configuration support throughout the codebase including settings, environment variables, Docker configuration, and web app integration.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eec82dd and 502f771.

📒 Files selected for processing (1)
  • immichFrame.Web/src/routes/+page.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • immichFrame.Web/src/routes/+page.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @Dockerfile:
- Around line 58-66: The entrypoint script currently does an irreversible sed
replacement of the /__IMMICH_FRAME_BASE__ placeholder (created in the RUN echo
block that writes /app/entrypoint.sh), which makes the operation non-idempotent
when BaseUrl changes; modify the script written by that RUN to instead copy
/app/wwwroot to a writable runtime working directory (e.g.,
/app/wwwroot-runtime) on each container start and run the sed substitution
against that copy, or implement a reversible marker scheme (e.g., wrap the base
path with a distinct marker like __IMMICH_BASE_MARKER__ that the script can
detect and replace back), ensuring you update the entrypoint logic that sets
BASE_PATH from BaseUrl and the find/sed command to operate on the working copy
or marker so subsequent restarts with different BaseUrl values correctly update
files.

In @ImmichFrame.Core/Interfaces/IServerSettings.cs:
- Line 65: The property BaseUrl is declared non-nullable in IServerSettings but
is consumed with a nullable-conditional operator; change the interface
declaration to allow null by updating IServerSettings to declare BaseUrl as
string? BaseUrl { get; } so it matches usage (and leave GeneralSettings and
ServerSettingsV1 defaults of "/" as-is); this aligns types and avoids the
mismatch at the call site (var baseUrl = settings.BaseUrl?.TrimEnd('/')).

In @ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs:
- Line 9: Add a PackageReference for FluentAssertions to the test project: open
ImmichFrame.WebApi.Tests.csproj and inside the <ItemGroup> that contains the
other <PackageReference> entries (where AwesomeAssertions is referenced) add a
new <PackageReference Include="FluentAssertions" /> element so the using
FluentAssertions; import in ConfigLoaderTest.cs resolves; save and restore
packages (dotnet restore) if needed.
🧹 Nitpick comments (6)
immichFrame.Web/src/routes/+page.ts (1)

8-15: Consider normalizing trailing slash handling.

There's an inconsistency in how trailing slashes are handled:

  • Line 8 explicitly appends "/" to the base path
  • Line 14 uses config.baseUrl as-is without ensuring a trailing slash

If config.baseUrl comes from the server without a trailing slash (e.g., "/subpath"), the behavior will differ from the initial setup. Consider normalizing this by either:

  1. Always ensuring a trailing slash: setBaseUrl(config.baseUrl.endsWith('/') ? config.baseUrl : config.baseUrl + '/')
  2. Or documenting that config.baseUrl must include a trailing slash
♻️ Proposed fix to normalize trailing slash
  setBaseUrl(base + "/");

  const configRequest = await api.getConfig({ clientIdentifier: "" });

  const config = configRequest.data;
  if (config.baseUrl) {
-    setBaseUrl(config.baseUrl);
+    const normalizedUrl = config.baseUrl.endsWith('/') ? config.baseUrl : config.baseUrl + '/';
+    setBaseUrl(normalizedUrl);
  }
docs/docs/getting-started/configuration.md (1)

101-102: Consider adding an example value for clarity.

The documentation clearly describes the BaseUrl option. To help users understand reverse proxy scenarios better, consider adding an example with a non-root path in the comment.

📝 Suggested documentation enhancement
-  # The base URL the app is hosted on. Useful when using a reverse proxy.
+  # The base URL the app is hosted on. Useful when using a reverse proxy.
+  # Example: For https://example.com/immichframe, set this to '/immichframe'
  BaseUrl: '/'  # string
docker/example.env (1)

13-13: Clarify the default behavior in documentation.

The BaseUrl option is shown as commented, which is appropriate for an example file. However, consider adding a comment explaining:

  • The default value is '/' if not specified
  • When users should uncomment and modify this setting (e.g., when deploying behind a reverse proxy at a subpath)
📝 Suggested documentation improvement
+# BaseUrl: Set the base path for reverse proxy deployments (default: /)
+# Example: BaseUrl=/immichframe for hosting at https://example.com/immichframe
 # BaseUrl=/
ImmichFrame.WebApi/Models/ServerSettings.cs (1)

65-65: Consider adding BaseUrl validation.

The BaseUrl property is correctly added with a sensible default. However, consider adding validation in the Validate() method (line 75) to ensure:

  • BaseUrl starts with '/'
  • BaseUrl contains only valid URL path characters
  • Consistent trailing slash handling

This would catch configuration errors early and provide clear feedback to users.

♻️ Optional validation enhancement
 public void Validate() 
 {
+    if (!string.IsNullOrEmpty(BaseUrl) && !BaseUrl.StartsWith('/'))
+    {
+        throw new InvalidOperationException("BaseUrl must start with '/' or be empty.");
+    }
+    
+    // Normalize trailing slash for consistency
+    if (!string.IsNullOrEmpty(BaseUrl) && BaseUrl != "/" && BaseUrl.EndsWith('/'))
+    {
+        BaseUrl = BaseUrl.TrimEnd('/');
+    }
 }
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)

74-76: Unused variable adapter.

The ServerSettingsV1Adapter instance is created but never used in this test. Consider removing it if not needed.

Proposed fix
     public void TestApplyEnvironmentVariables_V1()
     {
         var v1 = new ServerSettingsV1 { BaseUrl = "/" };
-        var adapter = new ServerSettingsV1Adapter(v1);

         var env = new Dictionary<string, string> { { "BaseUrl", "'/new-path'" } };
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)

115-122: Consider edge case with nested quotes.

The sequential if statements (not else if) will strip both single and double quotes if nested. For example, '"value"' becomes value after both checks. If this is intentional for shell escaping scenarios, consider adding a brief comment to clarify.

Add clarifying comment
                 var value = env[key]?.ToString() ?? string.Empty;
-                // Clean up quotes if present
+                // Clean up quotes if present (handles nested quotes from shell escaping)
                 if (value.StartsWith("'") && value.EndsWith("'"))
                     value = value.Substring(1, value.Length - 2);
                 if (value.StartsWith("\"") && value.EndsWith("\""))
                     value = value.Substring(1, value.Length - 2);
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4472213 and c2fff34.

📒 Files selected for processing (17)
  • Dockerfile
  • ImmichFrame.Core/Interfaces/IServerSettings.cs
  • ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs
  • ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • ImmichFrame.WebApi/Models/ClientSettingsDto.cs
  • ImmichFrame.WebApi/Models/ServerSettings.cs
  • ImmichFrame.WebApi/Program.cs
  • docker/Settings.example.json
  • docker/Settings.example.yml
  • docker/example.env
  • docs/docs/getting-started/configuration.md
  • immichFrame.Web/src/app.html
  • immichFrame.Web/src/lib/immichFrameApi.ts
  • immichFrame.Web/src/routes/+page.ts
  • immichFrame.Web/static/manifest.webmanifest
  • immichFrame.Web/svelte.config.js
🧰 Additional context used
🧬 Code graph analysis (9)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
ImmichFrame.Core/Api/ImmichApi.cs (3)
  • ImmichApi (3-11)
  • ImmichApi (5-10)
  • ImmichFrame (1-12)
immichFrame.Web/src/lib/immichFrameApi.ts (1)
immichFrame.Web/src/lib/index.ts (2)
  • baseUrl (15-17)
  • defaults (13-13)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
  • MapDictionaryToConfig (105-125)
immichFrame.Web/src/routes/+page.ts (2)
immichFrame.Web/src/lib/index.ts (1)
  • setBaseUrl (15-17)
immichFrame.Web/svelte.config.js (1)
  • config (6-27)
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (2)
ImmichFrame.Core/Api/ImmichApi.cs (3)
  • ImmichApi (3-11)
  • ImmichApi (5-10)
  • ImmichFrame (1-12)
ImmichFrame.WebApi/Controllers/ConfigController.cs (2)
  • ApiController (7-33)
  • ImmichFrame (5-34)
ImmichFrame.WebApi/Program.cs (2)
ImmichFrame.WebApi/Controllers/ConfigController.cs (1)
  • ApiController (7-33)
ImmichFrame.Core/Api/ImmichApi.cs (2)
  • ImmichApi (3-11)
  • ImmichFrame (1-12)
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
ImmichFrame.Core/Api/ImmichApi.cs (3)
  • ImmichApi (5-10)
  • ImmichApi (3-11)
  • ImmichFrame (1-12)
immichFrame.Web/src/app.html (1)
immichFrame.Web/static/pwa-service-worker.js (2)
  • cacheName (14-18)
  • event (9-22)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (2)
  • IServerSettings (23-29)
  • IServerSettings (30-89)
ImmichFrame.WebApi/Models/ServerSettings.cs (2)
  • GeneralSettings (38-76)
  • ValidateAndInitialize (95-110)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
  • ValidateAndInitialize (27-27)
🔇 Additional comments (14)
docker/Settings.example.json (1)

37-38: LGTM!

The BaseUrl configuration addition is properly formatted with correct JSON syntax and a sensible default value of "/".

immichFrame.Web/src/routes/+page.ts (1)

3-4: LGTM!

The imports are appropriate for configuring the base URL from both SvelteKit's base path and the runtime configuration.

immichFrame.Web/src/lib/immichFrameApi.ts (1)

215-215: LGTM! Server-side definition verified.

The addition of the optional baseUrl property to ClientSettingsDto is correct. The server-side ClientSettingsDto in ImmichFrame.WebApi/Models/ClientSettingsDto.cs includes the public string BaseUrl { get; set; } property, and it is properly mapped in the FromGeneralSettings() method.

docker/Settings.example.yml (1)

36-36: LGTM!

The BaseUrl configuration is properly added with a sensible default value of '/' for root-level deployment.

immichFrame.Web/svelte.config.js (1)

12-14: LGTM!

The SvelteKit base path configuration correctly uses the same placeholder as the manifest, ensuring consistent routing and asset resolution across the frontend.

immichFrame.Web/static/manifest.webmanifest (1)

4-4: Implementation already handles placeholder replacement correctly.

The entrypoint script in the Dockerfile (line 58-66) properly replaces __IMMICH_FRAME_BASE__ at runtime using the BaseUrl environment variable. It processes all relevant file types (html, js, json, webmanifest, css) and falls back to an empty string if BaseUrl is not provided. However, consider:

  1. Document the BaseUrl environment variable in Install_Web.md—it's not listed in the example configurations
  2. Add error handling to the entrypoint script: sed failures silently continue; consider explicit logging if replacement fails
immichFrame.Web/src/app.html (1)

6-6: LGTM - Relative paths align with base path strategy.

The switch from absolute to relative paths for favicon.png, manifest.webmanifest, and pwa-service-worker.js correctly supports the configurable base URL feature. When SvelteKit's kit.paths.base is set, these relative references will resolve correctly under the configured path.

Also applies to: 12-14, 25-25

ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)

33-33: LGTM - BaseUrl property correctly added and mapped.

The new BaseUrl property follows the established pattern for other settings, and the mapping from IGeneralSettings is consistent with existing properties.

Also applies to: 64-64

ImmichFrame.WebApi/Program.cs (2)

11-19: LGTM - Development environment .env loading.

The conditional loading of .env file only in development mode is appropriate and helps with local development workflow.


93-113: Base URL middleware implementation is correct.

The UsePathBase combined with the 404 guard middleware correctly handles requests that don't match the configured base URL. The middleware order (custom validation before UseRouting, followed by UseStaticFiles and endpoint mapping) is appropriate for a SPA application and ensures:

  • Requests without the base URL are rejected immediately
  • Valid requests proceed through static file serving and endpoint routing
  • The fallback to index.html works as expected

The PathBase.HasValue check is technically redundant since an empty PathBase won't equal a non-empty baseUrl, but it provides clearer intent and doesn't affect correctness.

ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)

90-104: LGTM - Environment variable application to config.

The method correctly handles both ServerSettings (V2) and ServerSettingsV1Adapter configurations, ensuring environment variables can override settings loaded from files.

ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)

55-55: LGTM - BaseUrl property added with sensible default.

The default value of "/" is appropriate, ensuring existing configurations without explicit BaseUrl continue to work at the root path.


61-66: LGTM - Settings property exposure for environment variable mapping.

Exposing Settings as a public property is necessary for ConfigLoader.MapDictionaryToConfig to apply environment variable overrides. The adapter pattern is correctly maintained.


96-131: LGTM - Adapter mappings consistently updated.

All property mappings correctly reference Settings.* and the new BaseUrl property is properly exposed through GeneralSettingsV1Adapter.

public bool ImageFill { get; }
public string Layout { get; }
public string Language { get; }
public string BaseUrl { get; }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, find the exact interface and its location
find . -name "IServerSettings.cs" -o -name "IGeneralSettings.cs" | head -20

Repository: immichFrame/ImmichFrame

Length of output: 115


🏁 Script executed:

# Search for class implementations
rg "class.*:.*IServerSettings|class.*:.*IGeneralSettings" --type cs -A 3

Repository: immichFrame/ImmichFrame

Length of output: 1653


🏁 Script executed:

# Search for BaseUrl property implementations to see how they're initialized
rg "BaseUrl" --type cs -B 2 -A 2

Repository: immichFrame/ImmichFrame

Length of output: 6305


🏁 Script executed:

# Get the full interface file to see all properties and context
cat -n ImmichFrame.Core/Interfaces/IServerSettings.cs

Repository: immichFrame/ImmichFrame

Length of output: 3058


🏁 Script executed:

# Check the IGeneralSettings interface
rg "interface IGeneralSettings" --type cs -A 30

Repository: immichFrame/ImmichFrame

Length of output: 2888


Address type safety mismatch: BaseUrl is declared non-nullable but consumed as nullable.

The interface declares BaseUrl as non-nullable string, and all implementations (GeneralSettings, ServerSettingsV1) provide a default value of "/". However, in Program.cs (line with var baseUrl = settings.BaseUrl?.TrimEnd('/')), the code uses the nullable-conditional operator ?. on this property, which is only necessary for nullable types.

Either change the interface to public string? BaseUrl { get; } to match the actual usage, or remove the ?. operator from Program.cs since the property is guaranteed to have a value.

🤖 Prompt for AI Agents
In @ImmichFrame.Core/Interfaces/IServerSettings.cs at line 65, The property
BaseUrl is declared non-nullable in IServerSettings but is consumed with a
nullable-conditional operator; change the interface declaration to allow null by
updating IServerSettings to declare BaseUrl as string? BaseUrl { get; } so it
matches usage (and leave GeneralSettings and ServerSettingsV1 defaults of "/"
as-is); this aligns types and avoids the mismatch at the call site (var baseUrl
= settings.BaseUrl?.TrimEnd('/')).

brady added 2 commits January 7, 2026 11:52
…7/ImmichFrame into feature/baseUrl_parameter

# Conflicts:
#	Dockerfile
#	ImmichFrame.Core/Interfaces/IServerSettings.cs
#	ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
#	ImmichFrame.WebApi/Models/ClientSettingsDto.cs
#	ImmichFrame.WebApi/Models/ServerSettings.cs
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)

32-32: Clarify: Is the Language property change related to BaseUrl feature?

The addition of = string.Empty initialization to the Language property appears unrelated to the BaseUrl feature. Can you clarify if this is:

  • An intentional improvement included in this PR?
  • A separate fix that should be tracked separately?

While this change doesn't introduce issues, it's generally better to keep unrelated changes in separate commits for cleaner history.

ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj (1)

10-11: Remove the AwesomeAssertions package reference.

Both AwesomeAssertions and FluentAssertions are present in the project file, but AwesomeAssertions is no longer used in any tests. The codebase has fully migrated to FluentAssertions (e.g., .Should().BeEquivalentTo() in ConfigLoaderTest.cs). Removing this unused dependency reduces package bloat.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c2fff34 and 685a079.

📒 Files selected for processing (6)
  • Dockerfile
  • ImmichFrame.Core/Interfaces/IServerSettings.cs
  • ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • ImmichFrame.WebApi/Models/ClientSettingsDto.cs
  • ImmichFrame.WebApi/Models/ServerSettings.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • Dockerfile
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
  • IConfigSettable (6-9)
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)
ImmichFrame.WebApi/Controllers/ConfigController.cs (1)
  • ApiController (7-33)
🔇 Additional comments (5)
ImmichFrame.WebApi/Models/ServerSettings.cs (1)

65-65: LGTM! BaseUrl property is well-integrated.

The BaseUrl property addition is consistent with the IGeneralSettings interface and has an appropriate default value of "/".

ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (2)

55-55: LGTM! V1 settings model properly extended.

The BaseUrl property addition to ServerSettingsV1 maintains consistency with the V2 settings model and uses the same default value.


130-130: LGTM! Adapter correctly delegates BaseUrl.

The GeneralSettingsV1Adapter properly exposes the BaseUrl property through delegation to the underlying V1 settings object.

ImmichFrame.Core/Interfaces/IServerSettings.cs (1)

65-65: LGTM! Interface properly defines BaseUrl as nullable.

The BaseUrl property is correctly defined as nullable (string?), which aligns with how it's consumed in Program.cs with the null-conditional operator (?.). This addresses the type safety concern mentioned in previous reviews.

ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)

33-33: LGTM! BaseUrl properly added to DTO and mapping.

The BaseUrl property is correctly defined and mapped from IGeneralSettings, maintaining consistency with the rest of the PR changes.

Also applies to: 64-64

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @immichFrame.Web/src/routes/+page.ts:
- Line 8: The current call setBaseUrl(base + "/") can produce a double trailing
slash if base already ends with '/', so change the code around the setBaseUrl
call to normalize base first (e.g., trim any trailing slashes from the base
value and then append a single '/'), ensuring you operate on the same variable
referenced as base before calling setBaseUrl; for example, compute a
normalizedBase by removing trailing slashes from base (handle empty/null safely)
and then call setBaseUrl(normalizedBase + "/").
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 685a079 and eec82dd.

📒 Files selected for processing (6)
  • ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • ImmichFrame.WebApi/Models/ServerSettings.cs
  • docker/example.env
  • docs/docs/getting-started/configuration.md
  • immichFrame.Web/src/routes/+page.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • docker/example.env
  • docs/docs/getting-started/configuration.md
  • ImmichFrame.WebApi/Models/ServerSettings.cs
🧰 Additional context used
🧬 Code graph analysis (1)
immichFrame.Web/src/routes/+page.ts (2)
immichFrame.Web/src/lib/index.ts (1)
  • setBaseUrl (15-17)
immichFrame.Web/svelte.config.js (1)
  • config (6-27)
🔇 Additional comments (3)
ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj (1)

10-10: Good library choice; migration is complete.

Switching to FluentAssertions is a solid improvement—it's the industry-standard assertion library for .NET with excellent readability and comprehensive APIs. The migration from AwesomeAssertions is complete with no remaining usages found, and test code is correctly using FluentAssertions fluent syntax.

Note: FluentAssertions lacks an explicit Version attribute, consistent with all other packages in the test project. If version pinning is desired for build reproducibility, consider adding versions across the dependency set.

immichFrame.Web/src/routes/+page.ts (2)

3-4: LGTM! Imports are correctly added for base URL support.

The imports of setBaseUrl and base are appropriate for initializing and configuring the application's base URL from both build-time and runtime sources.


13-16: Base URL normalization and dual initialization are correctly implemented.

The dual setBaseUrl calls follow a well-designed pattern:

  1. Line 8 initializes the API base URL from SvelteKit's base path (set in svelte.config.js as /__IMMICH_FRAME_BASE__)
  2. The Dockerfile replaces this placeholder with the actual base path at runtime using sed
  3. Lines 13-16 optionally override with server-provided config.baseUrl if available

The trailing slash normalization is correct, and the design properly allows both default behavior and runtime override. Asset loading and API routing will work correctly since SvelteKit's static adapter handles assets from the configured base path, while API calls use the base URL set by setBaseUrl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant