Skip to content

Feature/base url parameter#552

Open
chuckb7 wants to merge 12 commits intoimmichFrame:mainfrom
chuckb7:feature/baseUrl_parameter
Open

Feature/base url parameter#552
chuckb7 wants to merge 12 commits intoimmichFrame:mainfrom
chuckb7:feature/baseUrl_parameter

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/applies the runtime base path and ensures correct file ownership before launch.
  • Tests

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

    • Documented BaseUrl usage for reverse-proxy deployments.

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds BaseUrl support end-to-end: config/interface/model changes, env-var mapping, runtime Docker entrypoint that substitutes base-path placeholders in static assets, server PathBase + request validation middleware, and frontend SvelteKit base-path wiring and manifest updates.

Changes

Cohort / File(s) Summary
Docker & Deployment
Dockerfile, docker/example.env, docker/Settings.example.json, docker/Settings.example.yml
Add BaseUrl examples; final image creates /app/entrypoint.sh which computes BASE_PATH, replaces __IMMICH_FRAME_BASE__ in runtime wwwroot, sets ownership, symlinks runtime assets, and becomes the container ENTRYPOINT.
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 "/" semantics, validation that BaseUrl starts with / or is empty, and trailing-slash normalization.
Server Settings V1 Adapter
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
Add BaseUrl to legacy ServerSettingsV1 and expose it via Settings and GeneralSettingsV1Adapter to surface BaseUrl for v1 configs.
Configuration Loading & Tests
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs, ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs, ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj
Introduce ApplyEnvironmentVariables + MapDictionaryToConfig (strips surrounding quotes) to map env vars into config shapes; LoadConfig invokes this pass; add tests verifying BaseUrl mapping; switch test assertions from AwesomeAssertions to 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
Configure SvelteKit kit.paths.base = '/__IMMICH_FRAME_BASE__'; convert asset and service-worker paths to relative; add optional baseUrl to client DTO/type; frontend sets/overrides base at runtime; manifest uses __IMMICH_FRAME_BASE__ placeholders.
Docs
docs/docs/getting-started/configuration.md
Document new BaseUrl General setting (default /) with reverse-proxy guidance.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Docker
    participant Entrypoint as "/app/entrypoint.sh"
    participant RuntimeAssets as "wwwroot-runtime"
    participant DotNet as "dotnet ImmichFrame.WebApi.dll"
    participant Config as "ConfigLoader"
    participant Program as "Program (ASP.NET)"
    participant Browser as "Browser / SvelteKit"

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

    DotNet->>Config: LoadConfig()
    Config->>Config: ApplyEnvironmentVariables() -> set BaseUrl
    Config-->>DotNet: return 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: add middleware -> 404 if 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

Possibly related PRs

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' directly corresponds to the main change: adding BaseUrl as a configurable parameter throughout the application stack.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

Tip

You can disable sequence diagrams in the walkthrough.

Disable the reviews.sequence_diagrams setting to disable sequence diagrams in the walkthrough.

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.

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.

brady and others added 4 commits January 26, 2026 10:32
Resolve merge conflict in ServerSettingsV1.cs by adopting main's
_delegate naming convention while preserving BaseUrl property from
the feature branch.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ConfigLoader.cs references v1Adapter.Settings, which was lost when
merging main's _delegate rename. Re-expose as a public property.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@chuckb7
Copy link
Author

chuckb7 commented Mar 18, 2026

I had a bad merge when implementing some of CodeRabbitAI recommendations. I rolled them back to an earlier commit, built my docker image locally, and I've been running it without issue since.

This update is to cherry pick the good changes and roll forward to something that has a chance of getting merged and working.

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

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Dockerfile`:
- Around line 59-66: The entrypoint script only reads the BaseUrl environment
variable so when the application is configured via Settings.json/Settings.yml
(General.BaseUrl) the frontend rewrite uses root and desyncs from API PathBase;
update the /app/entrypoint.sh logic that sets BASE_PATH to first check the
BaseUrl env var and if empty, parse General.BaseUrl from Settings.json and
Settings.yml (e.g., grep/sed/awk for the "General.BaseUrl" key), trim trailing
slashes, and then apply the same replacement that uses BASE_PATH; refer to the
BASE_PATH variable and the entrypoint script block that executes sed
replacements and ensure the new lookup runs before the echo and find/sed
replacement so file-based config is respected.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8a20d705-953d-42e4-b504-a1b3446c8f42

📥 Commits

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

📒 Files selected for processing (11)
  • Dockerfile
  • ImmichFrame.Core/Interfaces/IServerSettings.cs
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • ImmichFrame.WebApi/Models/ClientSettingsDto.cs
  • ImmichFrame.WebApi/Models/ServerSettings.cs
  • docker/Settings.example.json
  • docker/Settings.example.yml
  • docker/example.env
  • docs/docs/getting-started/configuration.md
  • immichFrame.Web/src/lib/immichFrameApi.ts
  • immichFrame.Web/src/routes/+page.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • docker/example.env
  • ImmichFrame.Core/Interfaces/IServerSettings.cs
  • immichFrame.Web/src/routes/+page.ts
  • docker/Settings.example.yml
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • ImmichFrame.WebApi/Models/ServerSettings.cs
  • ImmichFrame.WebApi/Models/ClientSettingsDto.cs

…rl fallback

Extract entrypoint.sh from the Dockerfile RUN echo block for readability,
and add fallback logic to parse General.BaseUrl from Settings.json/yml/yaml
so file-based config is respected during frontend placeholder replacement.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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