fix(analytics): admin dashboard reads correct fields + Umami device API (#661)

Reporter @alexvaltchev hit three independent bugs on the Analytics Dashboard. All three fixed in one PR; pluggable-tracker support (Rybbit, Plausible, etc.) left for a separate discussion. ## Bug A — Summary cards showed 0 Two layers, both fixed. **Frontend** (`AnalyticsPage.tsx:142-149`): the cards summed `chartData[].views/uniqueVisitors/downloads`. The backend now (and already) emits a dedicated `totals` object computed via separate COUNT queries, which is what the cards should read. Postgres returns counts as strings, so coerce via `Number()`. **Backend** (`adminDashboard.js:268-282`): the chartData merge used `dateObj.date === row.date`. On Postgres, pg's driver auto-converts `DATE(timestamp)` to a JS Date object — the string-equality match failed silently and `chartData` stayed all-zero on every Postgres install with traffic. Added a `normaliseDateKey()` helper that returns YYYY-MM-DD regardless of driver shape, plus `Number()` coercion on the counts. SQLite path unchanged. ## Bug B — "Umami Not Configured" banner despite valid config `AnalyticsPage.tsx:90` did `settings.reduce(...)` on the `/admin/settings` response. That endpoint returns a key/value **object** (verified at `adminSettings.js:108-149`), not an array, so `.reduce` threw `data.reduce is not a function` and the catch silently rendered the "Not Configured" banner even on perfectly-configured installs. Read the umami keys directly off the response object. ## Bug C — Device breakdown 0/0/0 Two-pronged fix. **Primary path — Umami device API** (`services/umamiClient.js`, wired into `adminDashboard.js`). When the admin provides an Umami v2 API key (new setting `analytics_umami_api_key`), the backend fetches the per-period device breakdown from Umami's `/api/websites/:id/metrics?type=device` endpoint. Umami tracks devices natively — far more accurate than our coarse user-agent heuristic. The new `devicesSource` field in the response lets the UI hint at where the numbers came from. **Fallback hardening — local heuristic** (`adminDashboard.js:296-320`). The existing access_logs `LIKE '%Mobile%' / '%Tablet%'` query stays in place as a fallback for installs without Umami. Hardened with: `whereNotNull('user_agent')` skips rows we never captured a UA on, `Number()` coercion on COUNT results (pg returns strings), and a guard against divide-by-zero when access_logs is empty. ## API key handling Mirrors the existing recaptcha-secret pattern: stored plaintext in `app_settings`, masked as `••••••••` on every GET via the existing `adminSettings.js` GET handlers, and the frontend save mutation silently drops the masked sentinel so re-saving without typing a new key preserves the stored value. ## End-to-end smoke (dockerised backend with my fixes applied) ``` chartData total views: 27 ← previously 0 (date merge broken on PG) totals: {'views': '27', 'downloads': '3', 'uniqueVisitors': '1'} devices: {'desktop': 100, 'mobile': 0, 'tablet': 0} ← was 0/0/0 devicesSource: access_logs ← falls back correctly analytics_umami_api_key (GET /settings/analytics): •••••••• ``` ## Tests **Backend** (15 new cases): - `umamiClient.test.js` (10): missing-config → null, URL shape + `x-umami-api-key` header, websiteId URL-encoding, `{x,y}` → percentages, `laptop` → `desktop` mapping, unknown buckets dropped, empty payload → null, non-2xx → null, invalid JSON → null, network error → null. - `analyticsDateMerge.test.js` (5): YYYY-MM-DD string pass-through, ISO timestamp slice, JS Date (pg shape) → YYYY-MM-DD, null/empty → null, coercion for unexpected types. **Frontend**: full 84-case vitest suite still green (no analytics unit tests existed before; not adding any here — the changes are narrow and the unit-level confidence comes from the type system + the backend smoke above). Closes #661 (bugs A + B + C). Rybbit / pluggable tracker support is the next conversation per the issue author's follow-up.
2026-06-23 17:37:41 +02:00
parent a1f68bc081
commit 7534447b6c
9 changed files with 471 additions and 47 deletions
@@ -4,8 +4,26 @@ const { adminAuth } = require('../middleware/auth');
 const { requirePermission } = require('../middleware/permissions');
 const { sanitizeDays, addDateRangeCondition } = require('../utils/sqlSecurity');
 const { formatBoolean } = require('../utils/dbCompat');
+const { getAppSetting } = require('../utils/appSettings');
+const { fetchUmamiDeviceBreakdown } = require('../services/umamiClient');
+const logger = require('../utils/logger');
 const router = express.Router();

+/**
+ * Normalise a value coming back from `DATE(timestamp)` into a YYYY-MM-DD
+ * string. SQLite returns this column as a string already; Postgres' pg
+ * driver auto-converts it to a JavaScript Date object, which broke the
+ * old `dateObj.date === row.date` merge below — every Postgres install saw
+ * an all-zero `chartData[]` even with real traffic (#661 Bug A). Always
+ * normalise before comparing.
+ */
+function normaliseDateKey(value) {
+  if (!value) return null;
+  if (value instanceof Date) return value.toISOString().slice(0, 10);
+  // Strings might arrive with time component, slice defensively.
+  return String(value).slice(0, 10);
+}
+
 // Get dashboard statistics
 router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req, res) => {
  try {
@@ -265,20 +283,25 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (
      .where('timestamp', '>=', startDateStr)
      .groupByRaw('DATE(timestamp)');

-    // Merge data into dates array
+    // Merge data into dates array. row.date is normalised because Postgres
+    // returns DATE() as a JS Date while SQLite returns a string (#661 Bug A).
+    // Counts come back as strings on Postgres too, so coerce via Number.
    viewsData.forEach(row => {
-      const dateObj = dates.find(d => d.date === row.date);
-      if (dateObj) dateObj.views = row.count;
+      const key = normaliseDateKey(row.date);
+      const dateObj = dates.find(d => d.date === key);
+      if (dateObj) dateObj.views = Number(row.count) || 0;
    });

    downloadsData.forEach(row => {
-      const dateObj = dates.find(d => d.date === row.date);
-      if (dateObj) dateObj.downloads = row.count;
+      const key = normaliseDateKey(row.date);
+      const dateObj = dates.find(d => d.date === key);
+      if (dateObj) dateObj.downloads = Number(row.count) || 0;
    });

    visitorsData.forEach(row => {
-      const dateObj = dates.find(d => d.date === row.date);
-      if (dateObj) dateObj.uniqueVisitors = row.count;
+      const key = normaliseDateKey(row.date);
+      const dateObj = dates.find(d => d.date === key);
+      if (dateObj) dateObj.uniqueVisitors = Number(row.count) || 0;
    });

    // Get top galleries by views with additional metrics
@@ -293,31 +316,66 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (
      .orderBy('views', 'desc')
      .limit(5);

-    // Get device breakdown (simplified - based on user agent)
-    const deviceData = await db('access_logs')
-      .select(
-        db.raw(`
-          CASE 
-            WHEN user_agent LIKE '%Mobile%' THEN 'mobile'
-            WHEN user_agent LIKE '%Tablet%' OR user_agent LIKE '%iPad%' THEN 'tablet'
-            ELSE 'desktop'
-          END as device_type
-        `),
-        db.raw('COUNT(*) as count')
-      )
-      .where('timestamp', '>=', startDateStr)
-      .groupBy('device_type');
+    // Device breakdown — prefer Umami's metrics API when configured (#661
+    // Bug C). The local access_logs heuristic below produces 0% on installs
+    // where guest user agents don't reliably contain "Mobile" / "Tablet"
+    // tokens; Umami tracks devices natively. Falls back to access_logs when
+    // Umami is unconfigured, unreachable, or rate-limited.
+    let devices = { desktop: 0, mobile: 0, tablet: 0 };
+    let devicesSource = 'access_logs';

-    const totalDevices = deviceData.reduce((sum, d) => sum + d.count, 0);
-    const devices = {
-      desktop: 0,
-      mobile: 0,
-      tablet: 0
+    const umamiConfig = {
+      enabled: (await getAppSetting('analytics_umami_enabled', false)) === true,
+      baseUrl: await getAppSetting('analytics_umami_url', null),
+      websiteId: await getAppSetting('analytics_umami_website_id', null),
+      apiKey: await getAppSetting('analytics_umami_api_key', null),
    };
+    if (umamiConfig.enabled && umamiConfig.baseUrl && umamiConfig.websiteId && umamiConfig.apiKey) {
+      try {
+        const umamiDevices = await fetchUmamiDeviceBreakdown({
+          baseUrl: umamiConfig.baseUrl,
+          websiteId: umamiConfig.websiteId,
+          apiKey: umamiConfig.apiKey,
+          startMs: startDate.getTime(),
+          endMs: Date.now(),
+        });
+        if (umamiDevices) {
+          devices = umamiDevices;
+          devicesSource = 'umami';
+        }
+      } catch (err) {
+        logger.warn('Analytics: Umami device-breakdown fetch failed; falling back to access_logs', {
+          error: err.message,
+        });
+      }
+    }

-    deviceData.forEach(d => {
-      devices[d.device_type] = Math.round((d.count / totalDevices) * 100);
-    });
+    if (devicesSource === 'access_logs') {
+      // Local heuristic on access_logs user_agent. Coarse — `LIKE` doesn't
+      // cover every UA shape (some Android browsers, embedded webviews, etc.)
+      // — and counts come back as strings on Postgres, hence Number() below.
+      const deviceData = await db('access_logs')
+        .select(
+          db.raw(`
+            CASE
+              WHEN user_agent LIKE '%Mobile%' THEN 'mobile'
+              WHEN user_agent LIKE '%Tablet%' OR user_agent LIKE '%iPad%' THEN 'tablet'
+              ELSE 'desktop'
+            END as device_type
+          `),
+          db.raw('COUNT(*) as count')
+        )
+        .where('timestamp', '>=', startDateStr)
+        .whereNotNull('user_agent')
+        .groupBy('device_type');
+
+      const totalDevices = deviceData.reduce((sum, d) => sum + (Number(d.count) || 0), 0);
+      if (totalDevices > 0) {
+        deviceData.forEach(d => {
+          devices[d.device_type] = Math.round(((Number(d.count) || 0) / totalDevices) * 100);
+        });
+      }
+    }

    // Calculate totals for the period (matching /stats logic)
    const totalViews = await db('access_logs')
@@ -341,6 +399,7 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (
      chartData: dates,
      topGalleries,
      devices,
+      devicesSource,
      totals: {
        views: totalViews?.count || 0,
        downloads: totalDownloadsCount?.count || 0,
@@ -145,6 +145,12 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res)
    if (settingsObject.security_recaptcha_secret_key) {
      settingsObject.security_recaptcha_secret_key = '••••••••';
    }
+    // Umami v2 API key (#661 Bug C) — read-write secret that authenticates
+    // outbound calls to the operator's Umami instance for the device
+    // breakdown. Masked on GET, same pattern as the recaptcha secret.
+    if (settingsObject.analytics_umami_api_key) {
+      settingsObject.analytics_umami_api_key = '••••••••';
+    }

    res.json(settingsObject);
  } catch (error) {
@@ -390,6 +396,12 @@ router.get('/:type', adminAuth, requirePermission('settings.view'), async (req,
    if (settingsObject.security_recaptcha_secret_key) {
      settingsObject.security_recaptcha_secret_key = '••••••••';
    }
+    // Umami v2 API key (#661 Bug C) — read-write secret that authenticates
+    // outbound calls to the operator's Umami instance for the device
+    // breakdown. Masked on GET, same pattern as the recaptcha secret.
+    if (settingsObject.analytics_umami_api_key) {
+      settingsObject.analytics_umami_api_key = '••••••••';
+    }

    res.json(settingsObject);
  } catch (error) {