From b82b90d2f5542114c4e7f5e5ab78d42c3ceb6e20 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 2 Jun 2026 23:20:14 +1000 Subject: [PATCH] =?UTF-8?q?fix(sacred-valley):=20review=20polish=20?= =?UTF-8?q?=E2=80=94=20render-gen=20guard,=20auth-boundary=20tests,=20PNG?= =?UTF-8?q?=20sig,=20dedup=20note?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses final-review findings: I1 render-generation guard prevents a double-mount /timer leak on rapid re-navigation; I2 adds anonymous-rejection tests for the owner-only POST /speedtest/run and /health/check; M1 CSS comment; M2 cron↔worker dedup note; M4 full 8-byte PNG signature check; M5 card-contract unit test for all 7 cards. Co-Authored-By: Claude Opus 4.8 --- lib/cron/index.js | 4 ++++ lib/health/icons.js | 3 ++- public/style.css | 2 +- public/views/sacred_valley.js | 5 +++++ tests/api/health.test.js | 2 ++ tests/api/speedtest.test.js | 2 ++ tests/frontend/card_contract.test.js | 31 ++++++++++++++++++++++++++++ 7 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 tests/frontend/card_contract.test.js diff --git a/lib/cron/index.js b/lib/cron/index.js index 5334e0c..b9b7383 100644 --- a/lib/cron/index.js +++ b/lib/cron/index.js @@ -23,6 +23,10 @@ export function startCron() { catch (e) { log.error({ err: e }, 'cron speedtest failed'); } }); + // Health checks every minute. NOTE: this runs checkAll() inline; the same + // probe+upsert logic is also exposed on-demand via the `health.check` pg-boss + // worker (lib/jobs/workers/health_check.js, triggered by POST /api/health/check). + // Keep the two in sync — both rely on lib/health/checker.js as the source of truth. cron.schedule('*/1 * * * *', async () => { try { const results = await checkAll(load()); diff --git a/lib/health/icons.js b/lib/health/icons.js index 3022247..be4697d 100644 --- a/lib/health/icons.js +++ b/lib/health/icons.js @@ -16,7 +16,8 @@ async function defaultFetcher(slug) { return Buffer.from(await res.arrayBuffer()); } -function isPng(buf) { return buf && buf.length > 8 && buf[0] === 0x89 && buf[1] === 0x50 && buf[2] === 0x4e && buf[3] === 0x47; } +const PNG_SIG = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; +function isPng(buf) { return buf && buf.length > 8 && PNG_SIG.every((b, i) => buf[i] === b); } // Returns a Buffer (cached or freshly fetched) or null if upstream has no icon. export async function getIcon(slug) { diff --git a/public/style.css b/public/style.css index 4e5dd9b..1e2c8e2 100644 --- a/public/style.css +++ b/public/style.css @@ -175,7 +175,7 @@ ul.plain li:last-child { border-bottom: none; } --hue-dross: #ff4f2e; --hue-yerin: #c45a4a; --hue-orthos: #6fa86a; */ } #sv-cards { display: grid; grid-template-columns: repeat(6, 1fr); gap: 16px; align-items: start; } -.sv-card { grid-column: span 2; } /* m default */ +.sv-card { grid-column: span 2; } /* s / fallback (factory always sets data-size) */ .sv-card[data-size="s"] { grid-column: span 2; } .sv-card[data-size="m"] { grid-column: span 3; } .sv-card[data-size="l"] { grid-column: span 6; } diff --git a/public/views/sacred_valley.js b/public/views/sacred_valley.js index a86b2d0..68d009f 100644 --- a/public/views/sacred_valley.js +++ b/public/views/sacred_valley.js @@ -14,8 +14,10 @@ import speedtest from './cards/speedtest.js'; const CARD_MODULES = [clock, weather, hostPerf, jobs, inbox, search, speedtest]; // grows in later tasks let active = []; // mounted cards needing stop() +let renderGen = 0; // guards against overlapping async renders export async function render(main) { + const myGen = ++renderGen; active.forEach(c => c.stop && c.stop()); active = []; stopHealthBand(); mount(main, el('h1', { class: 'view-h1' }, 'Sacred Valley'), @@ -26,6 +28,9 @@ export async function render(main) { let layout = { card_order: [], hidden: [], sizes: {} }; try { layout = await api.get('/api/dashboard/layout'); } catch { /* defaults */ } + // A newer render() started while we awaited — bail before mounting timers/DOM + // so we don't double-mount cards or leak intervals onto the live grid. + if (myGen !== renderGen) return; const grid = document.getElementById('sv-cards'); const ordered = orderCards(CARD_MODULES, layout); diff --git a/tests/api/health.test.js b/tests/api/health.test.js index 3aaae7b..98ecb1f 100644 --- a/tests/api/health.test.js +++ b/tests/api/health.test.js @@ -10,6 +10,8 @@ beforeAll(async () => { }); describe('health api', () => { it('401 without auth', async () => expect((await request(app).get('/api/health/services')).status).toBe(401)); + it('POST /check rejects anonymous (owner-only mutation)', async () => + expect((await request(app).post('/api/health/check')).status).toBe(401)); it('returns groups with counts + merged cached status', async () => { const res = await request(app).get('/api/health/services').set(ownerHeaders); expect(res.status).toBe(200); diff --git a/tests/api/speedtest.test.js b/tests/api/speedtest.test.js index 7f372fc..159ff76 100644 --- a/tests/api/speedtest.test.js +++ b/tests/api/speedtest.test.js @@ -7,6 +7,8 @@ let app, ownerHeaders; beforeAll(async () => { ({ app, ownerHeaders } = await setup()); await repo.record({ down_mbps: 50, up_mbps: 10, ping_ms: 12 }); }); describe('speedtest api', () => { it('401 without auth', async () => expect((await request(app).get('/api/speedtest/history')).status).toBe(401)); + it('POST /run rejects anonymous (auth boundary before enqueue)', async () => + expect((await request(app).post('/api/speedtest/run')).status).toBe(401)); it('history returns rows', async () => { const res = await request(app).get('/api/speedtest/history').set(ownerHeaders); expect(res.status).toBe(200); diff --git a/tests/frontend/card_contract.test.js b/tests/frontend/card_contract.test.js new file mode 100644 index 0000000..3bd3cc0 --- /dev/null +++ b/tests/frontend/card_contract.test.js @@ -0,0 +1,31 @@ +import { describe, it, expect } from 'vitest'; +import clock from '../../public/views/cards/clock.js'; +import weather from '../../public/views/cards/weather.js'; +import hostPerf from '../../public/views/cards/host_perf.js'; +import jobs from '../../public/views/cards/jobs.js'; +import inbox from '../../public/views/cards/inbox.js'; +import search from '../../public/views/cards/search.js'; +import speedtest from '../../public/views/cards/speedtest.js'; + +// Every data card must satisfy the uniform contract that sacred_valley.js relies +// on: { id, title, size: s|m|l, mount(), start(), stop() }. Importing the modules +// is safe in node — their browser globals (document/localStorage/fetch) are only +// touched inside mount()/load(), which this test never calls. +const cards = { clock, weather, hostPerf, jobs, inbox, search, speedtest }; + +describe('card contract', () => { + for (const [name, c] of Object.entries(cards)) { + it(`${name} implements the card contract`, () => { + expect(typeof c.id).toBe('string'); + expect(typeof c.title).toBe('string'); + expect(['s', 'm', 'l']).toContain(c.size); + expect(typeof c.mount).toBe('function'); + expect(typeof c.start).toBe('function'); + expect(typeof c.stop).toBe('function'); + }); + } + it('all card ids are unique', () => { + const ids = Object.values(cards).map(c => c.id); + expect(new Set(ids).size).toBe(ids.length); + }); +});