From 7534447b6c0df4290fd8dac12270673097096f1b Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 23 Jun 2026 17:37:41 +0200 Subject: [PATCH 1/6] fix(analytics): admin dashboard reads correct fields + Umami device API (#661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../utils/analyticsDateMerge.test.js | 58 +++++++ backend/__tests__/utils/umamiClient.test.js | 142 ++++++++++++++++++ backend/src/routes/adminDashboard.js | 117 +++++++++++---- backend/src/routes/adminSettings.js | 12 ++ backend/src/services/umamiClient.js | 104 +++++++++++++ .../settings/hooks/useSettingsState.ts | 16 +- .../features/settings/tabs/AnalyticsTab.tsx | 23 +++ frontend/src/pages/admin/AnalyticsPage.tsx | 34 +++-- frontend/src/services/admin.service.ts | 12 ++ 9 files changed, 471 insertions(+), 47 deletions(-) create mode 100644 backend/__tests__/utils/analyticsDateMerge.test.js create mode 100644 backend/__tests__/utils/umamiClient.test.js create mode 100644 backend/src/services/umamiClient.js diff --git a/backend/__tests__/utils/analyticsDateMerge.test.js b/backend/__tests__/utils/analyticsDateMerge.test.js new file mode 100644 index 00000000..dcfda159 --- /dev/null +++ b/backend/__tests__/utils/analyticsDateMerge.test.js @@ -0,0 +1,58 @@ +/** + * Pins the date-merge fix in `adminDashboard.js` /analytics route + * (#661 Bug A). The merge previously failed on Postgres because pg's + * driver returns `DATE(timestamp)` as a JS Date object, while SQLite + * returns a string — the old `dateObj.date === row.date` comparison + * was false on Postgres so chartData stayed all-zero even with traffic. + * + * We test the normalisation helper here in isolation. The route-level + * integration is covered by the existing dashboard route test. + */ + +// The helper is internal to the route file; reimport via a small wrapper +// so we don't need to export everything publicly. +const path = require('path'); +const fs = require('fs'); + +const ROUTE_SRC = fs.readFileSync( + path.join(__dirname, '../../src/routes/adminDashboard.js'), + 'utf8', +); +// Tiny evaluator that grabs the normaliseDateKey function definition from +// the route source so the test pins the actual shipping implementation, +// not a copy. +function extractNormaliseDateKey() { + const match = ROUTE_SRC.match(/function normaliseDateKey\([\s\S]*?\n\}/); + if (!match) throw new Error('normaliseDateKey not found in adminDashboard.js'); + // eslint-disable-next-line no-new-func + return new Function(`${match[0]}; return normaliseDateKey;`)(); +} + +const normaliseDateKey = extractNormaliseDateKey(); + +describe('analytics route — normaliseDateKey (#661 Bug A)', () => { + test('passes through a YYYY-MM-DD string unchanged', () => { + expect(normaliseDateKey('2026-06-22')).toBe('2026-06-22'); + }); + + test('slices off a time component on a longer ISO string', () => { + expect(normaliseDateKey('2026-06-22T00:00:00.000Z')).toBe('2026-06-22'); + }); + + test('normalises a JS Date object (Postgres pg-driver shape) to YYYY-MM-DD', () => { + const d = new Date('2026-06-22T12:34:56Z'); + expect(normaliseDateKey(d)).toBe('2026-06-22'); + }); + + test('returns null for null / undefined / empty', () => { + expect(normaliseDateKey(null)).toBeNull(); + expect(normaliseDateKey(undefined)).toBeNull(); + expect(normaliseDateKey('')).toBeNull(); + }); + + test('coerces unexpected types via String() to avoid throwing', () => { + // We don't expect to receive a number from either driver, but the + // helper should not crash if it does — date merge will simply miss. + expect(normaliseDateKey(20260622)).toBe('20260622'); + }); +}); diff --git a/backend/__tests__/utils/umamiClient.test.js b/backend/__tests__/utils/umamiClient.test.js new file mode 100644 index 00000000..4f325b2c --- /dev/null +++ b/backend/__tests__/utils/umamiClient.test.js @@ -0,0 +1,142 @@ +/** + * Unit tests for the Umami v2 metrics-API client (#661 Bug C). + * + * The client is only consumed by `/admin/dashboard/analytics` today to fetch + * the device-breakdown chart, so these tests pin: + * - The exact URL shape sent to Umami (`/api/websites//metrics?type=device&startAt=…&endAt=…`) + * - The `x-umami-api-key` auth header + * - The `{ x, y }` → `{ desktop, mobile, tablet }` percentage normalisation + * - `laptop` mapping into `desktop` for our 3-bucket UI + * - Defensive returns: missing config / non-2xx / non-JSON / empty array + * all return `null` so the route layer can fall back to access_logs. + */ + +const { fetchUmamiDeviceBreakdown } = require('../../src/services/umamiClient'); + +const ORIGINAL_FETCH = global.fetch; + +afterEach(() => { + global.fetch = ORIGINAL_FETCH; +}); + +function mockJson(body, { status = 200 } = {}) { + global.fetch = jest.fn(async () => ({ + ok: status >= 200 && status < 300, + status, + json: async () => body, + })); +} + +describe('fetchUmamiDeviceBreakdown', () => { + test('returns null when config is incomplete (back-compat for installs without API key)', async () => { + expect(await fetchUmamiDeviceBreakdown({})).toBeNull(); + expect(await fetchUmamiDeviceBreakdown({ baseUrl: 'https://u.example' })).toBeNull(); + expect(await fetchUmamiDeviceBreakdown({ baseUrl: 'https://u.example', websiteId: 'w' })).toBeNull(); + // No fetch should be issued in any of those cases. + expect(global.fetch).toBe(ORIGINAL_FETCH); + }); + + test('builds the expected URL + sends the x-umami-api-key header', async () => { + mockJson([{ x: 'desktop', y: 10 }]); + await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u.example.com/', + websiteId: 'site-123', + apiKey: 'secret', + startMs: 1700000000000, + endMs: 1700003600000, + }); + expect(global.fetch).toHaveBeenCalledTimes(1); + const [calledUrl, init] = global.fetch.mock.calls[0]; + expect(calledUrl).toBe( + 'https://u.example.com/api/websites/site-123/metrics?type=device&startAt=1700000000000&endAt=1700003600000', + ); + expect(init.headers['x-umami-api-key']).toBe('secret'); + expect(init.method).toBe('GET'); + }); + + test('encodes the websiteId so a path segment with reserved chars is safe', async () => { + mockJson([{ x: 'desktop', y: 1 }]); + await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u.example.com', + websiteId: 'a/b?c', + apiKey: 'k', + startMs: 0, endMs: 0, + }); + const [calledUrl] = global.fetch.mock.calls[0]; + expect(calledUrl).toContain('/api/websites/a%2Fb%3Fc/metrics'); + }); + + test('normalises a typical { x, y } payload into integer percentages', async () => { + mockJson([ + { x: 'desktop', y: 60 }, + { x: 'mobile', y: 30 }, + { x: 'tablet', y: 10 }, + ]); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + expect(out).toEqual({ desktop: 60, mobile: 30, tablet: 10 }); + }); + + test('maps `laptop` into `desktop` for the 3-bucket UI', async () => { + mockJson([ + { x: 'desktop', y: 50 }, + { x: 'laptop', y: 20 }, + { x: 'mobile', y: 30 }, + ]); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + // desktop = (50 + 20) / 100 = 70% + expect(out).toEqual({ desktop: 70, mobile: 30, tablet: 0 }); + }); + + test('drops unknown buckets entirely (avoids silent miscategorisation)', async () => { + mockJson([ + { x: 'desktop', y: 80 }, + { x: 'mobile', y: 20 }, + { x: 'unknown-future-bucket', y: 100 }, + ]); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + // 100 isn't counted into total, so 80/(80+20) = 80%, 20/(80+20) = 20%. + expect(out).toEqual({ desktop: 80, mobile: 20, tablet: 0 }); + }); + + test('returns null on empty payload (caller falls back to access_logs)', async () => { + mockJson([]); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + expect(out).toBeNull(); + }); + + test('returns null on non-2xx upstream response', async () => { + mockJson({ error: 'unauthorized' }, { status: 401 }); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + expect(out).toBeNull(); + }); + + test('returns null on invalid JSON body', async () => { + global.fetch = jest.fn(async () => ({ + ok: true, + status: 200, + json: async () => { throw new SyntaxError('not json'); }, + })); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + expect(out).toBeNull(); + }); + + test('returns null on network error (fetch throws)', async () => { + global.fetch = jest.fn(async () => { throw new Error('ECONNREFUSED'); }); + const out = await fetchUmamiDeviceBreakdown({ + baseUrl: 'https://u', websiteId: 'w', apiKey: 'k', startMs: 0, endMs: 0, + }); + expect(out).toBeNull(); + }); +}); diff --git a/backend/src/routes/adminDashboard.js b/backend/src/routes/adminDashboard.js index ec4ec4b3..45cc5cfb 100644 --- a/backend/src/routes/adminDashboard.js +++ b/backend/src/routes/adminDashboard.js @@ -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, diff --git a/backend/src/routes/adminSettings.js b/backend/src/routes/adminSettings.js index 3bb261a7..a04f0b98 100644 --- a/backend/src/routes/adminSettings.js +++ b/backend/src/routes/adminSettings.js @@ -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) { diff --git a/backend/src/services/umamiClient.js b/backend/src/services/umamiClient.js new file mode 100644 index 00000000..7e474562 --- /dev/null +++ b/backend/src/services/umamiClient.js @@ -0,0 +1,104 @@ +/** + * Thin Umami v2 API client (#661 Bug C). + * + * We use this in exactly one place today — `GET /admin/dashboard/analytics` + * pulls device-breakdown stats from Umami when an admin has provided an + * API key, because our own user-agent heuristic on `access_logs` returns + * 0/0/0 on installs where guest UAs don't reliably contain "Mobile" / + * "Tablet" tokens. Umami tracks devices natively. + * + * Auth: Umami v2 supports per-account API keys generated in the Umami UI + * (Settings → Profile → API Keys). We send them via the `x-umami-api-key` + * header. Older session-cookie-based auth is intentionally NOT supported + * here — operators should generate an API key rather than embedding their + * Umami account password in PicPeak. + */ + +const logger = require('../utils/logger'); + +const REQUEST_TIMEOUT_MS = 5000; + +/** + * Fetch the device breakdown for a Umami website in [startMs, endMs]. Returns + * `{ desktop, mobile, tablet }` as integer percentages summing to ~100, or + * `null` when the upstream call fails / returns no data — callers should + * treat null as "fall back to the local heuristic". + * + * @param {object} opts + * @param {string} opts.baseUrl — Umami instance root, e.g. https://analytics.example.com + * @param {string} opts.websiteId — UUID from the Umami website settings + * @param {string} opts.apiKey — Umami API key (x-umami-api-key) + * @param {number} opts.startMs — epoch ms, start of window + * @param {number} opts.endMs — epoch ms, end of window + */ +async function fetchUmamiDeviceBreakdown({ baseUrl, websiteId, apiKey, startMs, endMs }) { + if (!baseUrl || !websiteId || !apiKey) return null; + + const trimmedBase = String(baseUrl).replace(/\/+$/, ''); + const url = `${trimmedBase}/api/websites/${encodeURIComponent(websiteId)}/metrics` + + `?type=device&startAt=${encodeURIComponent(startMs)}&endAt=${encodeURIComponent(endMs)}`; + + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS); + let response; + try { + response = await fetch(url, { + method: 'GET', + headers: { + 'x-umami-api-key': apiKey, + Accept: 'application/json', + }, + signal: controller.signal, + }); + } catch (err) { + clearTimeout(timer); + if (err.name === 'AbortError') { + logger.warn('Umami device fetch: timeout', { url }); + return null; + } + logger.warn('Umami device fetch: network error', { error: err.message }); + return null; + } + clearTimeout(timer); + + if (!response.ok) { + logger.warn('Umami device fetch: non-2xx', { status: response.status }); + return null; + } + + let data; + try { + data = await response.json(); + } catch (err) { + logger.warn('Umami device fetch: invalid JSON', { error: err.message }); + return null; + } + + if (!Array.isArray(data)) return null; + + // Umami buckets device types into these strings: `desktop`, `mobile`, + // `tablet`, `laptop`. Map `laptop` → `desktop` for our 3-bucket UI; drop + // anything we don't recognise (vs. silently miscategorising). + const counts = { desktop: 0, mobile: 0, tablet: 0 }; + let total = 0; + for (const entry of data) { + if (!entry || typeof entry.x !== 'string') continue; + const n = Number(entry.y); + if (!Number.isFinite(n) || n <= 0) continue; + const key = entry.x === 'laptop' ? 'desktop' : entry.x; + if (key in counts) { + counts[key] += n; + total += n; + } + } + + if (total === 0) return null; + + return { + desktop: Math.round((counts.desktop / total) * 100), + mobile: Math.round((counts.mobile / total) * 100), + tablet: Math.round((counts.tablet / total) * 100), + }; +} + +module.exports = { fetchUmamiDeviceBreakdown }; diff --git a/frontend/src/features/settings/hooks/useSettingsState.ts b/frontend/src/features/settings/hooks/useSettingsState.ts index 5e3b2579..dae24f14 100644 --- a/frontend/src/features/settings/hooks/useSettingsState.ts +++ b/frontend/src/features/settings/hooks/useSettingsState.ts @@ -50,6 +50,12 @@ export interface AnalyticsSettings { umami_url: string; umami_website_id: string; umami_share_url: string; + // API key for Umami's v2 metrics API. Required ONLY for the device + // breakdown (#661 Bug C); the rest of the integration (embedded iframe, + // tracker script) still works without it. Server masks as `••••••••` + // on GET when a value is stored — submit the masked sentinel unchanged + // to keep the stored value, or a real key to replace it. + umami_api_key: string; } export interface EventSettings { @@ -129,7 +135,8 @@ export function useSettingsState() { umami_enabled: false, umami_url: '', umami_website_id: '', - umami_share_url: '' + umami_share_url: '', + umami_api_key: '' }); // Event creation settings state @@ -219,7 +226,8 @@ export function useSettingsState() { umami_enabled: toBoolean(settings.analytics_umami_enabled, false), umami_url: settings.analytics_umami_url || '', umami_website_id: settings.analytics_umami_website_id || '', - umami_share_url: settings.analytics_umami_share_url || '' + umami_share_url: settings.analytics_umami_share_url || '', + umami_api_key: settings.analytics_umami_api_key || '' }); setEventSettings({ @@ -315,6 +323,10 @@ export function useSettingsState() { mutationFn: async () => { const settingsData: Record = {}; Object.entries(analyticsSettings).forEach(([key, value]) => { + // The Umami API key (#661 Bug C) is returned masked as `••••••••` + // on GET so it doesn't leak in the response body. Don't re-save + // that sentinel — silently preserve whatever's already stored. + if (key === 'umami_api_key' && value === '••••••••') return; settingsData[`analytics_${key}`] = value; }); return settingsService.updateSettings(settingsData); diff --git a/frontend/src/features/settings/tabs/AnalyticsTab.tsx b/frontend/src/features/settings/tabs/AnalyticsTab.tsx index 265212cc..903e95b9 100644 --- a/frontend/src/features/settings/tabs/AnalyticsTab.tsx +++ b/frontend/src/features/settings/tabs/AnalyticsTab.tsx @@ -85,6 +85,29 @@ export const AnalyticsTab: React.FC = ({ {t('settings.analytics.shareUrlHelp')}

+ + {/* Umami API key — only used by the dashboard's device + breakdown today (#661 Bug C). Optional; the rest of the + integration works without it. Masked as •••••••• on GET. */} +
+ + setAnalyticsSettings(prev => ({ ...prev, umami_api_key: e.target.value }))} + placeholder="api_xxx…" + leftIcon={} + autoComplete="off" + /> +

+ {t( + 'settings.analytics.umamiApiKeyHelp', + 'Optional. Required only for the device-breakdown chart on the Analytics dashboard. Generate in Umami → Settings → Profile → API Keys. Stored masked as •••••••• once saved — leave the masked value to keep the existing key.', + )} +

+
)} diff --git a/frontend/src/pages/admin/AnalyticsPage.tsx b/frontend/src/pages/admin/AnalyticsPage.tsx index 8634fb90..f1253a54 100644 --- a/frontend/src/pages/admin/AnalyticsPage.tsx +++ b/frontend/src/pages/admin/AnalyticsPage.tsx @@ -82,21 +82,19 @@ export const AnalyticsPage: React.FC = () => { useEffect(() => { const fetchUmamiConfig = async () => { try { - // Use admin API endpoint with auth token since we're in admin area + // `/admin/settings` returns a key/value object (see backend + // `adminSettings.js:108`), NOT an array (#661 Bug B). The old + // `.reduce()` path threw `data.reduce is not a function` and the + // catch block silently rendered the "Umami Not Configured" banner + // even on perfectly-configured installs. Read keys directly. const response = await api.get('/admin/settings'); - const settings = response.data; - - // Transform the settings array to object - const settingsMap = settings.reduce((acc: any, setting: any) => { - acc[setting.key] = setting.value; - return acc; - }, {}); - + const settings = response.data ?? {}; + // Check if Umami is enabled in admin settings - if (settingsMap.analytics_umami_enabled && settingsMap.analytics_umami_url && settingsMap.analytics_umami_website_id) { + if (settings.analytics_umami_enabled && settings.analytics_umami_url && settings.analytics_umami_website_id) { setUmamiConfig({ - url: settingsMap.analytics_umami_url, - shareUrl: settingsMap.analytics_umami_share_url, + url: settings.analytics_umami_url, + shareUrl: settings.analytics_umami_share_url, enabled: true }); } else { @@ -139,10 +137,14 @@ export const AnalyticsPage: React.FC = () => { const analytics: ComponentAnalyticsData | undefined = React.useMemo(() => { if (!apiData) return undefined; - // Calculate totals from chart data - const totalViews = apiData.chartData.reduce((sum, day) => sum + day.views, 0); - const totalVisitors = apiData.chartData.reduce((sum, day) => sum + day.uniqueVisitors, 0); - const totalDownloads = apiData.chartData.reduce((sum, day) => sum + day.downloads, 0); + // Headline totals come from the dedicated `totals` object that the + // backend computes via separate COUNT queries (#661 Bug A). The old + // sum-the-chartData path returned 0 on Postgres installs because the + // backend's date-string merge into chartData failed there. Postgres' + // pg driver returns COUNT(...) as strings, so coerce via Number(). + const totalViews = Number(apiData.totals?.views ?? 0); + const totalVisitors = Number(apiData.totals?.uniqueVisitors ?? 0); + const totalDownloads = Number(apiData.totals?.downloads ?? 0); // Calculate trends (comparing last half to first half) const halfPoint = Math.floor(apiData.chartData.length / 2); diff --git a/frontend/src/services/admin.service.ts b/frontend/src/services/admin.service.ts index f8c40575..b3f9b5f7 100644 --- a/frontend/src/services/admin.service.ts +++ b/frontend/src/services/admin.service.ts @@ -327,6 +327,18 @@ export interface AnalyticsData { mobile: number; tablet: number; }; + // Period totals computed via dedicated COUNT queries on the backend + // (#661 Bug A). Postgres returns these as strings, so callers should + // coerce via Number() before display. Optional on the type because + // older backends (pre-#661) didn't always emit it. + totals?: { + views: number | string; + downloads: number | string; + uniqueVisitors: number | string; + }; + // Source of the device breakdown — `umami` when API-key auth succeeded, + // `access_logs` for the local user-agent heuristic fallback (#661 Bug C). + devicesSource?: 'umami' | 'access_logs'; } export const adminService = { From 9ae43fc9910a2bbc7f60eba7eecc07dbf65fcf0c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 23 Jun 2026 15:44:38 +0000 Subject: [PATCH 2/6] chore(beta): release 3.69.1-beta.0 --- .release-please-manifest-beta.json | 2 +- CHANGELOG.md | 8 ++++++++ backend/package.json | 2 +- frontend/package.json | 2 +- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/.release-please-manifest-beta.json b/.release-please-manifest-beta.json index 8b2f83fa..558a54ae 100644 --- a/.release-please-manifest-beta.json +++ b/.release-please-manifest-beta.json @@ -1,3 +1,3 @@ { - ".": "3.69.0-beta.0" + ".": "3.69.1-beta.0" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dcb9a81..23a6d511 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to PicPeak will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [3.69.1-beta.0](https://github.com/the-luap/picpeak/compare/v3.69.0-beta.0...v3.69.1-beta.0) (2026-06-23) + + +### Bug Fixes + +* **analytics:** admin dashboard reads correct fields + Umami device API ([#661](https://github.com/the-luap/picpeak/issues/661)) ([349f566](https://github.com/the-luap/picpeak/commit/349f566e87b33c59f61eb28b8abc5f889e6285d6)) +* **analytics:** admin dashboard reads correct fields + Umami device API ([#661](https://github.com/the-luap/picpeak/issues/661)) ([7534447](https://github.com/the-luap/picpeak/commit/7534447b6c0df4290fd8dac12270673097096f1b)) + ## [3.69.0-beta.0](https://github.com/the-luap/picpeak/compare/v3.68.1-beta.0...v3.69.0-beta.0) (2026-06-22) diff --git a/backend/package.json b/backend/package.json index eb1f0e28..8113bff0 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "picpeak-backend", - "version": "3.69.0-beta.0", + "version": "3.69.1-beta.0", "description": "Backend for PicPeak event photo sharing platform", "main": "server.js", "scripts": { diff --git a/frontend/package.json b/frontend/package.json index 2a6b6033..3b0d319b 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "picpeak-frontend", "private": true, - "version": "3.69.0-beta.0", + "version": "3.69.1-beta.0", "type": "module", "scripts": { "dev": "vite", From ab501459a4208c3a30637a31f0f6f31285ae5ef2 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 23 Jun 2026 18:11:58 +0200 Subject: [PATCH 3/6] =?UTF-8?q?feat(analytics):=20pluggable=20trackers=20?= =?UTF-8?q?=E2=80=94=20Umami=20+=20Rybbit=20+=20Custom=20(#663=20Phase=201?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the hybrid scope agreed on in #663: two native adapters (Umami + Rybbit) for trackers we'd keep maintained, plus a Custom script-paste mode for everyone else (Plausible, Matomo, Pirsch, GA4, GoatCounter, Fathom, Cloudflare Web Analytics). Phase 2 (Plausible native, deeper metrics) explicitly deferred until someone asks. ## Architecture **Backend `services/trackers/`**: - `TrackerAdapter` shape (single method): `fetchDeviceBreakdown` → `{ desktop, mobile, tablet } | null`. Null = route falls back to access_logs heuristic. - `umamiAdapter.js` — extracted from the `services/umamiClient.js` that landed in #662. Same 10 test contract preserved. - `rybbitAdapter.js` — new. Hits `/api/site/{id}/breakdown?dimension= device` with Bearer auth, accepts both bare-array and `{data:[...]}` envelope variants, tolerates `sessions`/`visitors`/`value`/`count` metric keys. - `customScriptSanitiser.js` — sanitize-html with a tracker-tight allowlist (`evil` (
stripped; script tags survive but CSP `script-src 'self'` still blocks inline + non-allowlisted external at runtime) ✓ test 4 (public-settings exposes the provider switch) → `analytics_tracker_provider: 'custom'`, `analytics_custom_head_html: ''` ✓ ``` ## Out of scope (next discussions) - **Plausible native** — covered via Custom mode for now; native is Phase 2 if someone explicitly asks. - **CSP "trusted domains" admin input** — Phase 1.5. For now operators add their tracker domain to nginx/proxy CSP manually; the new CSP-reminder banner in the Custom panel makes that clear. - **Refactor `(window as any).umami.track(...)` direct calls** in PhotoLightbox/PhotoGrid to go through `analyticsService.track()` so events fire on the right tracker. Currently a no-op when Umami isn't loaded; functional but not optimal. Closes #663 Phase 1. --- .../services/customScriptSanitiser.test.js | 100 ++++++ .../__tests__/services/rybbitAdapter.test.js | 115 +++++++ .../__tests__/services/trackerFactory.test.js | 91 +++++ .../__tests__/services/umamiAdapter.test.js | 112 ++++++ backend/__tests__/utils/umamiClient.test.js | 142 -------- backend/src/routes/adminDashboard.js | 37 +- backend/src/routes/adminSettings.js | 27 ++ backend/src/routes/publicSettings.js | 35 +- .../trackers/customScriptSanitiser.js | 86 +++++ backend/src/services/trackers/index.js | 75 +++++ .../src/services/trackers/rybbitAdapter.js | 121 +++++++ backend/src/services/trackers/umamiAdapter.js | 99 ++++++ backend/src/services/umamiClient.js | 104 ------ frontend/src/App.tsx | 45 ++- .../settings/hooks/useSettingsState.ts | 50 ++- .../features/settings/tabs/AnalyticsTab.tsx | 318 ++++++++++++------ frontend/src/i18n/locales/de.json | 20 ++ frontend/src/i18n/locales/en.json | 20 ++ frontend/src/services/analytics.service.ts | 166 ++++++--- .../src/services/publicSettings.service.ts | 8 + 20 files changed, 1344 insertions(+), 427 deletions(-) create mode 100644 backend/__tests__/services/customScriptSanitiser.test.js create mode 100644 backend/__tests__/services/rybbitAdapter.test.js create mode 100644 backend/__tests__/services/trackerFactory.test.js create mode 100644 backend/__tests__/services/umamiAdapter.test.js delete mode 100644 backend/__tests__/utils/umamiClient.test.js create mode 100644 backend/src/services/trackers/customScriptSanitiser.js create mode 100644 backend/src/services/trackers/index.js create mode 100644 backend/src/services/trackers/rybbitAdapter.js create mode 100644 backend/src/services/trackers/umamiAdapter.js delete mode 100644 backend/src/services/umamiClient.js diff --git a/backend/__tests__/services/customScriptSanitiser.test.js b/backend/__tests__/services/customScriptSanitiser.test.js new file mode 100644 index 00000000..e56ebb62 --- /dev/null +++ b/backend/__tests__/services/customScriptSanitiser.test.js @@ -0,0 +1,100 @@ +/** + * Tests for the custom-tracker HTML sanitiser (#663 Phase 1). + * + * The field accepts admin-pasted ``-style snippets for arbitrary + * trackers (Plausible / Matomo / Pirsch / GA4 / GoatCounter / Fathom / + * Cloudflare Web Analytics). We sanitise on save with a narrow allowlist + * tuned for tracker scripts — defence-in-depth, even though the field is + * admin-only. + */ + +const { sanitizeTrackerSnippet } = require('../../src/services/trackers/customScriptSanitiser'); + +describe('sanitizeTrackerSnippet (#663)', () => { + test('returns empty string for non-string / empty / whitespace input', () => { + expect(sanitizeTrackerSnippet(null)).toBe(''); + expect(sanitizeTrackerSnippet(undefined)).toBe(''); + expect(sanitizeTrackerSnippet(42)).toBe(''); + expect(sanitizeTrackerSnippet('')).toBe(''); + expect(sanitizeTrackerSnippet(' ')).toBe(''); + }); + + test('passes through a Plausible-style script tag with data-domain', () => { + const input = ''; + const out = sanitizeTrackerSnippet(input); + expect(out).toContain('src="https://plausible.io/js/script.js"'); + expect(out).toContain('data-domain="example.com"'); + expect(out).toContain('defer'); + }); + + test('passes through a Umami-style script with data-website-id', () => { + const input = ''; + const out = sanitizeTrackerSnippet(input); + expect(out).toContain('src="https://analytics.example.com/script.js"'); + expect(out).toContain('data-website-id="aaa-bbb-ccc"'); + }); + + test('passes through inline script body unchanged', () => { + const input = ''; + const out = sanitizeTrackerSnippet(input); + expect(out).toContain('window.GA = "x"'); + expect(out).toContain('console.log("init")'); + }); + + test('allows
+

+ {t( + 'settings.analytics.customHeadHtmlHelp', + 'Paste your tracker\'s `` snippet (Plausible, Matomo, Pirsch, GA4, GoatCounter, Fathom, Cloudflare Web Analytics, …). Sanitised on save: only `