From 9796969e801e95d6a0f944ab33cb59b6e2260f7f Mon Sep 17 00:00:00 2001
From: Bernt Popp <bernt.popp@gmail.com>
Date: Mon, 9 Feb 2026 14:36:25 +0100
Subject: [PATCH] fix: address Copilot review comments from PR #180

- useToast: replace unsafe `as string` casts with typeof guards, skip
  __handled401 errors to prevent duplicate toasts
- ReloadPrompt: clean up setInterval on unmount, add network/installing
  guards and try/catch per vite-plugin-pwa best practices
- axios: replace never-resolving promise with marked rejection so
  .finally() cleanup blocks still run
- vite proxy: only set Host:localhost header when using default Traefik
  target, skip when VITE_API_URL overrides it
- annotation_dates: fetch job history once instead of 3x, sort by
  completed_at desc instead of submitted_at
---
 api/endpoints/admin_endpoints.R     | 38 +++++++++++++++++------------
 app/src/components/ReloadPrompt.vue | 27 +++++++++++++++++---
 app/src/composables/useToast.ts     | 35 ++++++++++++++++++++------
 app/src/plugins/axios.ts            |  7 ++++--
 app/vite.config.ts                  |  7 +++---
 5 files changed, 83 insertions(+), 31 deletions(-)

diff --git a/api/endpoints/admin_endpoints.R b/api/endpoints/admin_endpoints.R
index dc43600e..19ca4b65 100644
--- a/api/endpoints/admin_endpoints.R
+++ b/api/endpoints/admin_endpoints.R
@@ -686,22 +686,28 @@ function() {
 function() {
   data_dir <- "data/"
 
+  # Fetch job history once for all lookups
+  history <- tryCatch(get_job_history(100), error = function(e) {
+    data.frame(
+      operation = character(0),
+      status = character(0),
+      completed_at = character(0),
+      stringsAsFactors = FALSE
+    )
+  })
+
   # Helper: get most recent completed_at for matching operations
-  get_last_successful_run <- function(operations) {
-    history <- tryCatch(get_job_history(100), error = function(e) {
-      data.frame(
-        operation = character(0),
-        status = character(0),
-        completed_at = character(0)
-      )
-    })
-    if (nrow(history) == 0) return(NA)
-    matches <- history[
-      history$operation %in% operations & history$status == "completed",
+  get_last_successful_run <- function(operations, hist) {
+    if (nrow(hist) == 0) return(NA)
+    matches <- hist[
+      hist$operation %in% operations & hist$status == "completed",
     ]
     if (nrow(matches) == 0) return(NA)
-    # get_job_history returns newest first
-    matches$completed_at[1]
+    # Sort by completed_at descending to get most recently finished job
+    valid <- matches[!is.na(matches$completed_at), ]
+    if (nrow(valid) == 0) return(NA)
+    sorted <- valid[order(valid$completed_at, decreasing = TRUE), ]
+    sorted$completed_at[1]
   }
 
   # Helper to get most recent file date matching a pattern
@@ -724,11 +730,11 @@ function() {
   }
 
   # Prefer job history timestamps; fall back to file metadata
-  omim_job <- get_last_successful_run("omim_update")
+  omim_job <- get_last_successful_run("omim_update", history)
   ontology_job <- get_last_successful_run(
-    c("ontology_update", "force_apply_ontology")
+    c("ontology_update", "force_apply_ontology"), history
   )
-  hgnc_job <- get_last_successful_run("hgnc_update")
+  hgnc_job <- get_last_successful_run("hgnc_update", history)
 
   null_coalesce <- function(a, b) if (!is.na(a)) a else b
 
diff --git a/app/src/components/ReloadPrompt.vue b/app/src/components/ReloadPrompt.vue
index badc9dc0..eca3a1aa 100644
--- a/app/src/components/ReloadPrompt.vue
+++ b/app/src/components/ReloadPrompt.vue
@@ -9,15 +9,30 @@
 </template>
 
 <script setup lang="ts">
+import { onUnmounted } from 'vue';
 import { useRegisterSW } from 'virtual:pwa-register/vue';
 
 const intervalMS = 60 * 60 * 1000; // Check for updates every 60 minutes
+let intervalId: ReturnType<typeof setInterval> | undefined;
 
 const { needRefresh, updateServiceWorker } = useRegisterSW({
-  onRegisteredSW(_swUrl: string, registration: ServiceWorkerRegistration | undefined) {
+  onRegisteredSW(swUrl: string, registration: ServiceWorkerRegistration | undefined) {
     if (registration) {
-      setInterval(() => {
-        registration.update();
+      intervalId = setInterval(async () => {
+        if (registration.installing) return;
+        if ('connection' in navigator && !navigator.onLine) return;
+
+        try {
+          const resp = await fetch(swUrl, {
+            cache: 'no-store',
+            headers: { 'cache-control': 'no-cache' },
+          });
+          if (resp?.status === 200) {
+            await registration.update();
+          }
+        } catch {
+          // Network error — skip this cycle
+        }
       }, intervalMS);
     }
   },
@@ -26,6 +41,12 @@ const { needRefresh, updateServiceWorker } = useRegisterSW({
   },
 });
 
+onUnmounted(() => {
+  if (intervalId !== undefined) {
+    clearInterval(intervalId);
+  }
+});
+
 function close() {
   needRefresh.value = false;
 }
diff --git a/app/src/composables/useToast.ts b/app/src/composables/useToast.ts
index 534d1a38..d8938453 100644
--- a/app/src/composables/useToast.ts
+++ b/app/src/composables/useToast.ts
@@ -40,19 +40,40 @@ export default function useToast(): ToastMethods {
     autoHide: boolean = true,
     autoHideDelay: number = 3000
   ): void => {
+    // Skip errors already handled by the 401 interceptor (redirect to login)
+    if (
+      typeof message === 'object' &&
+      message !== null &&
+      '__handled401' in message &&
+      (message as Record<string, unknown>).__handled401 === true
+    ) {
+      return;
+    }
+
     // Extract meaningful message from various error shapes (Axios errors, Error objects, strings)
     let body: string;
     if (typeof message === 'string') {
       body = message;
     } else if (typeof message === 'object' && message !== null) {
       const msg = message as Record<string, unknown>;
-      const resp = msg.response as Record<string, unknown> | undefined;
-      const respData = resp?.data as Record<string, unknown> | undefined;
-      body =
-        (respData?.message as string) ||
-        (respData?.error as string) ||
-        (msg.message as string) ||
-        String(message);
+      const resp =
+        typeof msg.response === 'object' && msg.response !== null
+          ? (msg.response as Record<string, unknown>)
+          : undefined;
+      const respData =
+        resp !== undefined && typeof resp.data === 'object' && resp.data !== null
+          ? (resp.data as Record<string, unknown>)
+          : undefined;
+
+      if (respData !== undefined && typeof respData.message === 'string') {
+        body = respData.message;
+      } else if (respData !== undefined && typeof respData.error === 'string') {
+        body = respData.error;
+      } else if (typeof msg.message === 'string') {
+        body = msg.message;
+      } else {
+        body = String(message);
+      }
     } else {
       body = String(message);
     }
diff --git a/app/src/plugins/axios.ts b/app/src/plugins/axios.ts
index 3c005810..06b79f39 100644
--- a/app/src/plugins/axios.ts
+++ b/app/src/plugins/axios.ts
@@ -37,8 +37,11 @@ axios.interceptors.response.use(
         isLoggingOut = false;
       }, 2000);
 
-      // Return a never-resolving promise to suppress downstream .catch() toasts
-      return new Promise(() => {});
+      // Mark as handled so downstream .catch() can skip toast display,
+      // while still rejecting so .finally() cleanup runs properly
+      const handled = new Error('Redirecting to login');
+      (handled as Error & { __handled401: boolean }).__handled401 = true;
+      return Promise.reject(handled);
     }
 
     return Promise.reject(error);
diff --git a/app/vite.config.ts b/app/vite.config.ts
index 2555536e..2485df15 100644
--- a/app/vite.config.ts
+++ b/app/vite.config.ts
@@ -132,9 +132,10 @@ export default defineConfig({
       '/api': {
         target: process.env.VITE_API_URL || 'http://traefik:80',
         changeOrigin: true,
-        // Traefik routes by Host header; inside Docker the target is 'traefik'
-        // but the routing rule expects 'localhost', so override it explicitly.
-        headers: { Host: 'localhost' },
+        // Inside Docker the proxy target is 'traefik' but Traefik routes by Host
+        // header expecting 'localhost'. Only override when using the default target;
+        // when VITE_API_URL is set the caller controls the target host directly.
+        ...(process.env.VITE_API_URL ? {} : { headers: { Host: 'localhost' } }),
       },
     },
   },