fix(portal): address CodeRabbit review findings

.coderabbit.yaml

@@ -0,0 +1,4 @@
+reviews:
+  path_filters:
+    - "!server/wwwroot/js/app.js"
+    - "!server/wwwroot/js/app.css"

server/ClientApp/src/components/Dashboard.tsx

@@ -30,21 +30,6 @@ interface HealthInfo {
   models:                       Record<string, ModelStatus>;
 }
 
-interface AllModelSettings {
-  ocr:       ModelEntry;
-  translate: ModelEntry;
-  inpaint:   ModelEntry;
-  bubble:    ModelEntry;
-  preferred_translation_engine: string;
-}
-
-interface ModelEntry {
-  repo:    string;
-  dir:     string;
-  enabled: boolean;
-  files:   string;
-}
-
 // ── Sub-components ───────────────────────────────────────────────────────────
 
 function StatusBadge(props: { ok: boolean | null; label: string }) {
@@ -98,6 +83,7 @@ export function Dashboard() {
   onMount(fetchHealth);
 
   function fetchHealth() {
+    setErr(null);
     fetch("/health")
       .then((r) => {
         if (!r.ok) throw new Error(`HTTP ${r.status}`);
@@ -126,23 +112,20 @@ export function Dashboard() {
   async function changeEngine(engine: string) {
     const h = health();
     if (!h || saving()) return;
+    setErr(null);
     setSaving(true);
     try {
-      // Fetch current settings to avoid overwriting other fields
-      const settingsRes = await fetch("/api/settings");
-      if (!settingsRes.ok) throw new Error("Failed to load settings");
-      const settings = (await settingsRes.json()) as AllModelSettings;
-      settings.preferred_translation_engine = engine;
-
-      const putRes = await fetch("/api/settings", {
-        method:  "PUT",
+      // Narrow PATCH endpoint updates only preferred_translation_engine,
+      // avoiding a read-modify-write cycle that would persist env-derived values to disk.
+      const res = await fetch("/api/settings/engine", {
+        method:  "PATCH",
         headers: { "Content-Type": "application/json" },
-        body:    JSON.stringify(settings),
+        body:    JSON.stringify({ engine }),
       });
-      if (!putRes.ok) throw new Error("Failed to save settings");
+      if (!res.ok) throw new Error(`Failed to save settings (HTTP ${res.status})`);
       setSavedEngine(engine);
     } catch (e) {
-      console.error("Failed to update translation engine:", e);
+      setErr(String(e));
     } finally {
       setSaving(false);
     }

server/ClientApp/src/pages/JobsListPage.tsx

@@ -55,49 +55,52 @@ function JobCard(props: {
       : jobOriginalUrl(props.job.id);
 
   return (
-    <button
-      class="group flex flex-col overflow-hidden rounded-2xl bg-white text-left shadow-sm ring-1 ring-slate-200 transition hover:shadow-md hover:ring-slate-300 focus-visible:outline focus-visible:outline-2 focus-visible:outline-violet-500"
-      onClick={props.onClick}
-    >
-      {/* Thumbnail */}
-      <div class="relative aspect-3/4 w-full overflow-hidden bg-slate-100">
-        <img
-          src={thumbSrc()}
-          alt={props.job.title}
-          class="h-full w-full object-cover transition-transform duration-200 group-hover:scale-105"
-          loading="lazy"
-        />
-        <div class="absolute right-2 top-2">
-          <StatusBadge status={props.job.status} />
-        </div>
-        {/* Delete icon — shown on hover. Must be a div, not button, because it's nested inside the card button. */}
-        <div
-          role="button"
-          tabIndex={0}
-          class="absolute left-2 top-2 flex h-6 w-6 cursor-pointer items-center justify-center rounded-md bg-white/80 text-slate-500 opacity-0 shadow-sm ring-1 ring-slate-200 transition-opacity hover:bg-red-50 hover:text-red-600 group-hover:opacity-100"
-          title="Delete job"
-          aria-label="Delete job"
-          onClick={props.onDelete}
-          onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") props.onDelete(e as unknown as MouseEvent); }}
-        >
-          <Trash2 class="h-3.5 w-3.5" />
+    // Outer div acts as the group anchor; contains two sibling native buttons so
+    // neither is nested inside the other (avoids invalid interactive-in-interactive HTML).
+    <div class="group relative flex flex-col overflow-hidden rounded-2xl bg-white shadow-sm ring-1 ring-slate-200 transition hover:shadow-md hover:ring-slate-300">
+      {/* Delete button — sibling, not nested inside the nav button */}
+      <button
+        class="absolute left-2 top-2 z-10 flex h-6 w-6 items-center justify-center rounded-md bg-white/80 text-slate-500 shadow-sm ring-1 ring-slate-200 hover:bg-red-50 hover:text-red-600"
+        title="Delete job"
+        aria-label="Delete job"
+        onClick={props.onDelete}
+      >
+        <Trash2 class="h-3.5 w-3.5" />
+      </button>
+
+      {/* Nav button covers thumbnail + info */}
+      <button
+        class="flex w-full flex-col text-left focus-visible:outline-2 focus-visible:outline-violet-500"
+        onClick={props.onClick}
+      >
+        {/* Thumbnail */}
+        <div class="relative aspect-3/4 w-full overflow-hidden bg-slate-100">
+          <img
+            src={thumbSrc()}
+            alt={props.job.title}
+            class="h-full w-full object-cover transition-transform duration-200 group-hover:scale-105"
+            loading="lazy"
+          />
+          <div class="absolute right-2 top-2">
+            <StatusBadge status={props.job.status} />
+          </div>
         </div>
-      </div>
 
-      {/* Info */}
-      <div class="flex flex-col gap-1 p-3">
-        <p class="truncate text-sm font-medium text-slate-800">{props.job.title}</p>
-        <div class="flex items-center gap-3 text-xs text-slate-500">
-          <span class="flex items-center gap-1">
-            <Layers class="h-3 w-3" />
-            {props.job.bubbleCount} bubbles
-          </span>
-          <span class="ml-auto">
-            {new Date(props.job.createdAt).toLocaleDateString()}
-          </span>
+        {/* Info */}
+        <div class="flex flex-col gap-1 p-3">
+          <p class="truncate text-sm font-medium text-slate-800">{props.job.title}</p>
+          <div class="flex items-center gap-3 text-xs text-slate-500">
+            <span class="flex items-center gap-1">
+              <Layers class="h-3 w-3" />
+              {props.job.bubbleCount} bubbles
+            </span>
+            <span class="ml-auto">
+              {new Date(props.job.createdAt).toLocaleDateString()}
+            </span>
+          </div>
         </div>
-      </div>
-    </button>
+      </button>
+    </div>
   );
 }
 
@@ -131,9 +134,11 @@ export function JobsListPage() {
   // Delete confirm state
   const [pendingDeleteId, setPendingDeleteId] = createSignal<string | null>(null);
   const [isDeleting, setIsDeleting] = createSignal(false);
+  const [deleteError, setDeleteError] = createSignal<string | null>(null);
 
   function requestDelete(id: string, e: MouseEvent): void {
     e.stopPropagation();
+    setDeleteError(null);
     setPendingDeleteId(id);
   }
 
@@ -145,6 +150,9 @@ export function JobsListPage() {
       await deleteJob(id);
       setPendingDeleteId(null);
       refetch();
+    } catch (err) {
+      setPendingDeleteId(null);
+      setDeleteError(err instanceof Error ? err.message : "Failed to delete job");
     } finally {
       setIsDeleting(false);
     }
@@ -180,6 +188,22 @@ export function JobsListPage() {
         </select>
       </div>
 
+      {/* Delete error */}
+      <Show when={deleteError()}>
+        {(msg) => (
+          <div role="alert" class="mb-4 flex items-center gap-2 rounded-xl bg-red-50 px-4 py-3 text-sm text-red-600 ring-1 ring-red-200">
+            <span class="flex-1">{msg()}</span>
+            <button
+              onClick={() => setDeleteError(null)}
+              class="shrink-0 rounded p-0.5 hover:bg-red-100"
+              aria-label="Dismiss error"
+            >
+              ✕
+            </button>
+          </div>
+        )}
+      </Show>
+
       {/* States */}
       <Show when={jobs.loading}>
         <div class="grid grid-cols-2 gap-4 sm:grid-cols-3 lg:grid-cols-4 xl:grid-cols-5">

server/src/ModelSettingsStore.cs

@@ -119,6 +119,38 @@ public async Task<AllModelSettings> UpdateAsync(AllModelSettings updated)
         finally { _writeLock.Release(); }
     }
 
+    /// <summary>
+    /// Atomically updates only the preferred translation engine and persists to disk.
+    /// Clones the current settings inside the write lock to avoid a read-modify-write race.
+    /// </summary>
+    public async Task<AllModelSettings> UpdateEngineAsync(string engine)
+    {
+        if (string.IsNullOrWhiteSpace(engine))
+            throw new ArgumentException("Engine is required.", nameof(engine));
+        engine = engine.Trim();
+
+        await _writeLock.WaitAsync();
+        try
+        {
+            var current = _current;
+            var updated = new AllModelSettings
+            {
+                Ocr       = current.Ocr,
+                Translate = current.Translate,
+                Inpaint   = current.Inpaint,
+                Bubble    = current.Bubble,
+                PreferredTranslationEngine = engine,
+            };
+            var dir = Path.GetDirectoryName(_filePath);
+            if (!string.IsNullOrEmpty(dir)) Directory.CreateDirectory(dir);
+            await File.WriteAllTextAsync(_filePath, JsonSerializer.Serialize(updated, JsonOpts));
+            _current = updated;
+            _logger.LogInformation("[ModelSettings] Persisted to {Path}", _filePath);
+            return _current;
+        }
+        finally { _writeLock.Release(); }
+    }
+
     // ── Private helpers ───────────────────────────────────────────────────────
 
     /// <summary>Build settings from env vars, using AppConfig-derived paths as dir defaults.</summary>
@@ -193,7 +225,15 @@ private static AllModelSettings Merge(AllModelSettings env, AllModelSettings fil
             Translate = MergeEntry(env.Translate,  file.Translate,  "TRANSLATE_MODEL_REPO", "TRANSLATE_MODELS_DIR", "TRANSLATE_MODEL_ENABLED", "TRANSLATE_MODEL_FILES"),
             Inpaint   = MergeEntry(env.Inpaint,    file.Inpaint,    "INPAINT_MODEL_REPO",   "INPAINT_MODELS_DIR",   "INPAINT_MODEL_ENABLED",   "INPAINT_MODEL_FILES"),
             Bubble    = MergeEntry(env.Bubble,     file.Bubble,     "BUBBLE_MODEL_REPO",    "BUBBLE_MODELS_DIR",    "BUBBLE_MODEL_ENABLED",    "BUBBLE_MODEL_FILES"),
-            PreferredTranslationEngine = file.PreferredTranslationEngine,
+            // Env wins; fall back to file (only if non-empty), then the env-derived default.
+            // IsNullOrWhiteSpace guards against whitespace-only env vars being treated as set.
+            PreferredTranslationEngine =
+                Environment.GetEnvironmentVariable("PREFERRED_TRANSLATION_ENGINE") is { } rawPe
+                    && !string.IsNullOrWhiteSpace(rawPe)
+                    ? rawPe.Trim()
+                    : string.IsNullOrWhiteSpace(file.PreferredTranslationEngine)
+                        ? env.PreferredTranslationEngine
+                        : file.PreferredTranslationEngine,
         };
     }
 

server/src/Routes/SettingsRoutes.cs

@@ -1,5 +1,8 @@
 namespace WebOcrServer;
 
+/// <summary>Narrow patch body for updating only the preferred translation engine.</summary>
+public record EngineUpdateRequest(string Engine);
+
 public static class SettingsRoutes
 {
     public static void MapSettingsRoutes(this WebApplication app)
@@ -18,5 +21,17 @@ public static void MapSettingsRoutes(this WebApplication app)
             var saved = await store.UpdateAsync(updated);
             return Results.Ok(saved);
         });
+
+        // PATCH /api/settings/engine — updates only preferred_translation_engine without
+        // touching other settings, avoiding the read-modify-write footgun on env-derived values.
+        app.MapMethods("/api/settings/engine", ["PATCH"], async (
+            EngineUpdateRequest req, ModelSettingsStore store) =>
+        {
+            if (string.IsNullOrWhiteSpace(req.Engine))
+                return Results.BadRequest(new { error = "engine is required" });
+
+            var saved = await store.UpdateEngineAsync(req.Engine);
+            return Results.Ok(new { preferred_translation_engine = saved.PreferredTranslationEngine });
+        });
     }
 }

server/src/Routes/TranslateRoutes.cs

@@ -11,16 +11,26 @@ public static void MapTranslateRoutes(this WebApplication app)
             BootState              boot,
             InferenceQueue         queue,
             ModelSettingsStore     modelSettings,
+            AppConfig              config,
             IServiceScopeFactory   scopeFactory,
             ILogger<TranslateService> logger) =>
         {
-            if (!boot.TranslateReady)
-                return Results.Json(new { error = "Translate model not ready" }, statusCode: 503);
-
             if (string.IsNullOrWhiteSpace(req.Text))
                 return Results.BadRequest(new { error = "text is required" });
 
-            var engine = req.TranslateEngine ?? modelSettings.Current.PreferredTranslationEngine;
+            var rawEngine = string.IsNullOrWhiteSpace(req.TranslateEngine)
+                ? modelSettings.Current.PreferredTranslationEngine
+                : req.TranslateEngine;
+
+            // Resolve "auto": prefer DeepL when configured, fall back to local.
+            var engine = rawEngine.Equals("auto", StringComparison.OrdinalIgnoreCase)
+                ? (config.DeeplAvailable ? "deepl" : "local")
+                : rawEngine;
+
+            // Only require the local model when the chosen engine is not an external API (DeepL).
+            var needsLocal = !engine.Equals("deepl", StringComparison.OrdinalIgnoreCase);
+            if (needsLocal && !boot.TranslateReady)
+                return Results.Json(new { error = "Translate model not ready" }, statusCode: 503);
 
             var tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
             await queue.Writer.WriteAsync(new TranslateJob(req.Text, engine, tcs));