Conversation
…tes (#177, #178, #179) - Centralize 401 handling in axios interceptor to prevent duplicate error toasts on JWT expiry (#179) - Add nginx cache-control headers, replace legacy register-service-worker with vite-plugin-pwa prompt mode and ReloadPrompt component (#178) - Use job history timestamps for annotation dates instead of file metadata, with file-based fallback for fresh installs (#177)
Setting `Bearer null` when no token caused the API to return 401 on public endpoints, triggering the interceptor redirect for logged-out users. Also clear the default header on 401 logout.
Inside Docker the Vite proxy targets http://traefik:80, but Traefik routes by Host header (localhost|sysndd.dbmr.unibe.ch). Without the override, requests arrived with Host: traefik and got 404.
There was a problem hiding this comment.
Pull request overview
This PR addresses several user-facing reliability issues across the Vue app, PWA update behavior, nginx caching, and the admin annotation dates API by centralizing auth error handling, improving cache/update flows, and deriving annotation “last updated” timestamps from job history.
Changes:
- Centralize Axios configuration + 401 handling, and improve toast message extraction for Axios error objects.
- Switch PWA registration to
promptmode with an in-app reload banner; adjust production nginx cache headers to prevent stale deployments. - Update
/api/admin/annotation_datesto prefer timestamps from job history over file metadata; adjust Vite dev proxy host routing.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/vite.config.ts | Switch PWA to prompt and adjust dev proxy headers for Traefik routing. |
| app/tsconfig.json | Add PWA Vue type declarations. |
| app/src/registerServiceWorker.js | Remove legacy Vue CLI-era service worker registration. |
| app/src/plugins/axios.ts | New centralized Axios setup (defaults + response interceptor). |
| app/src/plugins/axios.js | Remove old Vue plugin-based axios wrapper. |
| app/src/main.ts | Register Axios onto Vue global properties / DI and remove legacy SW import. |
| app/src/composables/useToast.ts | Improve error message extraction for non-string error shapes. |
| app/src/components/ReloadPrompt.vue | Add UI prompt for PWA updates + periodic update checks. |
| app/src/components/AppNavbar.vue | Remove per-component toast on auth check failure (avoid duplicate toasts). |
| app/src/App.vue | Mount the PWA reload prompt globally. |
| app/package.json | Drop register-service-worker; add workbox-window. |
| app/package-lock.json | Lockfile updates for dependency changes. |
| app/docker/nginx/prod.conf | Add cache-control headers for SW/manifest/assets/html. |
| api/endpoints/admin_endpoints.R | Prefer job-history timestamps (fallback to file metadata) for annotation dates. |
Files not reviewed (1)
- app/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| body = | ||
| (respData?.message as string) || | ||
| (respData?.error as string) || | ||
| (msg.message as string) || | ||
| String(message); |
There was a problem hiding this comment.
The message extraction uses type assertions like (respData?.message as string), which can assign non-string truthy values (objects) into body and potentially break toast rendering or show [object Object]. Guard with typeof ... === 'string' (and optionally fall back to JSON.stringify/String) before using these fields.
| body = | |
| (respData?.message as string) || | |
| (respData?.error as string) || | |
| (msg.message as string) || | |
| String(message); | |
| const dataMessage = | |
| respData && typeof (respData as any).message === 'string' | |
| ? ((respData as any).message as string) | |
| : undefined; | |
| const dataError = | |
| respData && typeof (respData as any).error === 'string' | |
| ? ((respData as any).error as string) | |
| : undefined; | |
| const msgMessage = | |
| typeof (msg as any).message === 'string' | |
| ? ((msg as any).message as string) | |
| : undefined; | |
| body = dataMessage ?? dataError ?? msgMessage ?? String(message); |
| const { needRefresh, updateServiceWorker } = useRegisterSW({ | ||
| onRegisteredSW(_swUrl: string, registration: ServiceWorkerRegistration | undefined) { | ||
| if (registration) { | ||
| setInterval(() => { | ||
| registration.update(); | ||
| }, intervalMS); | ||
| } |
There was a problem hiding this comment.
The update polling interval isn’t cleaned up and registration.update()’s returned promise is ignored. Store the interval id and clear it on unmount, and handle/catch update errors (or check navigator.onLine) to avoid unhandled rejections and noisy logs over long-running sessions.
| // Return a never-resolving promise to suppress downstream .catch() toasts | ||
| return new Promise(() => {}); |
There was a problem hiding this comment.
The 401 interceptor returns a never-resolving promise. Any caller awaiting an axios request will hang forever (and any finally/loading cleanup will never run), which can leave the UI stuck and accumulate pending promises during repeated 401s. Prefer letting the promise settle (e.g., reject with a marked/cancelled error that toast logic ignores, or resolve/reject after redirect) so downstream code can clean up deterministically.
| // Return a never-resolving promise to suppress downstream .catch() toasts | |
| return new Promise(() => {}); | |
| // Mark this error as a handled 401 so downstream logic can choose to ignore toasts, | |
| // but still let the promise settle so callers' finally/cleanup logic can run. | |
| (error as any).isHandled401 = true; | |
| return Promise.reject(error); |
| // but the routing rule expects 'localhost', so override it explicitly. | ||
| headers: { Host: 'localhost' }, |
There was a problem hiding this comment.
The proxy forces Host: localhost for all /api requests. This will break when VITE_API_URL points to a non-localhost backend (or when developing against a remote environment), since the upstream will receive an unexpected Host header. Make the Host override conditional (e.g., only when targeting Traefik in Docker / when VITE_MODE=docker) or derive it from the target URL.
| // but the routing rule expects 'localhost', so override it explicitly. | |
| headers: { Host: 'localhost' }, | |
| // but the routing rule expects 'localhost', so override it explicitly | |
| // only when running in the Docker/Traefik environment. | |
| headers: process.env.VITE_MODE === 'docker' ? { Host: 'localhost' } : undefined, |
| # 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", | ||
| ] | ||
| if (nrow(matches) == 0) return(NA) | ||
| # get_job_history returns newest first | ||
| matches$completed_at[1] | ||
| } |
There was a problem hiding this comment.
get_last_successful_run() calls get_job_history(100) every time it’s invoked, so this endpoint queries job history 3x per request. Fetch job history once and reuse it for each operation filter to avoid repeated work and reduce latency.
| # get_job_history returns newest first | ||
| matches$completed_at[1] |
There was a problem hiding this comment.
get_last_successful_run() assumes the first matching row is the most recent successful completion, but get_job_history() is sorted by submitted_at, not completed_at. If jobs overlap or complete out of order, this can report the wrong “Last” date. Filter to completed jobs and select the max by completed_at (or explicitly sort matches by completed_at desc) before returning.
| # get_job_history returns newest first | |
| matches$completed_at[1] | |
| # Explicitly select the latest completion time in case history is not | |
| # ordered by completed_at | |
| latest_idx <- order(matches$completed_at, decreasing = TRUE)[1] | |
| matches$completed_at[latest_idx] |
- 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
fix: address Copilot review comments from PR #180
1 participant
Summary
Fixes three frontend/infrastructure bugs plus two issues discovered during testing:
/Login, and returns a never-resolving promise to suppress all downstream.catch()toasts. Removed per-component toast calls fromAppNavbar. ImproveduseToasterror extraction for Axios error objects.prod.conf(sw.js/manifest never cached, hashed assets immutable 1yr, HTML never cached). Replaced legacyregister-service-workerwithvite-plugin-pwaprompt mode and aReloadPrompt.vuecomponent that shows an update banner with periodic 60-min checks.annotation_datesendpoint now queriesget_job_history()for the most recent successful completion per operation type, falling back to file metadata for fresh installs.Authorizationheader when a token exists — prevents 401s on public endpoints for logged-out users.Host: localhostheader in proxy config so Traefik correctly routes API requests from inside the Docker container.Closes #177, closes #178, closes #179
Test plan
sw.jsgenerated/Login, zero error toasts, localStorage cleared