From aa77332c0db805eb0a88ed463fa9bee591232f8f Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 6 Aug 2026 09:35:16 -0500 Subject: [PATCH] Let the playlist preview skip, so reviewing item 8 does not cost seven durations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The preview shipped without the skip control #104 asked for, so checking a late item meant watching every item before it in real time — the thing operators do most when ordering a playlist with a client on the phone. The preview is already the real player in device-free mode (an iframe of /player?preview=1), so this drives that instance rather than growing a second playback implementation: the dashboard posts next/prev to the one contentWindow, the player steps its own currentIndex and re-renders through the same path a natural advance uses, and posts back index/total so the modal can say "3 of 7". Nothing here can reach a live screen. A real display is driven over its server socket and holds no window handle this page could address; the message listener is installed only by the preview boot path, previewNavigate refuses outside PREVIEW_MODE, and both ends pin the origin. Stepping is schedule-aware in the direction of travel — falling forward past a dayparted item would make "previous" walk forwards — and a multi-zone playlist reports itself as such, because all zones play at once and a counter there would be a lie. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- frontend/js/i18n/de.js | 4 + frontend/js/i18n/en.js | 4 + frontend/js/i18n/es.js | 4 + frontend/js/i18n/fr.js | 4 + frontend/js/i18n/it.js | 4 + frontend/js/i18n/pt.js | 4 + frontend/js/views/playlists.js | 63 +++++++++++- server/player/index.html | 76 ++++++++++++++ server/test/preview-skip.test.js | 171 +++++++++++++++++++++++++++++++ 9 files changed, 329 insertions(+), 5 deletions(-) create mode 100644 server/test/preview-skip.test.js diff --git a/frontend/js/i18n/de.js b/frontend/js/i18n/de.js index 5bdea0d..8429af2 100644 --- a/frontend/js/i18n/de.js +++ b/frontend/js/i18n/de.js @@ -763,6 +763,10 @@ export default { 'playlist.draft.discard_changes': 'Änderungen verwerfen', 'playlist.draft.publish': 'Veröffentlichen', 'playlist.draft.publishing': 'Wird veröffentlicht...', + 'playlist.preview_prev': 'Zurück', + 'playlist.preview_next': 'Weiter', + 'playlist.preview_position': '{current} von {total}', + 'playlist.preview_zoned': 'Zonen laufen gleichzeitig', 'playlist.toast.created': 'Playlist erstellt', 'playlist.toast.deleted': 'Playlist gelöscht', 'playlist.toast.published': 'Playlist veröffentlicht — Geräte aktualisiert', diff --git a/frontend/js/i18n/en.js b/frontend/js/i18n/en.js index 6a39bce..24a2731 100644 --- a/frontend/js/i18n/en.js +++ b/frontend/js/i18n/en.js @@ -1159,6 +1159,10 @@ export default { 'playlist.zones_count_one': '1 zone', 'playlist.zones_count_other': '{n} zones', 'playlist.layout_ambiguous': 'Items reference zones from more than one layout', + 'playlist.preview_prev': 'Previous', + 'playlist.preview_next': 'Next', + 'playlist.preview_position': '{current} of {total}', + 'playlist.preview_zoned': 'Zones play together', 'playlist.back': 'Back', 'playlist.items_empty': 'This playlist is empty', 'playlist.items_empty_hint': 'Click "Add Content" to add items.', diff --git a/frontend/js/i18n/es.js b/frontend/js/i18n/es.js index e8c8077..9a6da03 100644 --- a/frontend/js/i18n/es.js +++ b/frontend/js/i18n/es.js @@ -793,6 +793,10 @@ export default { 'playlist.draft.discard_changes': 'Descartar cambios', 'playlist.draft.publish': 'Publicar', 'playlist.draft.publishing': 'Publicando...', + 'playlist.preview_prev': 'Anterior', + 'playlist.preview_next': 'Siguiente', + 'playlist.preview_position': '{current} de {total}', + 'playlist.preview_zoned': 'Las zonas se reproducen juntas', 'playlist.toast.created': 'Lista creada', 'playlist.toast.deleted': 'Lista eliminada', 'playlist.toast.published': 'Lista publicada — dispositivos actualizados', diff --git a/frontend/js/i18n/fr.js b/frontend/js/i18n/fr.js index abf9135..438f36c 100644 --- a/frontend/js/i18n/fr.js +++ b/frontend/js/i18n/fr.js @@ -763,6 +763,10 @@ export default { 'playlist.draft.discard_changes': 'Annuler les modifications', 'playlist.draft.publish': 'Publier', 'playlist.draft.publishing': 'Publication...', + 'playlist.preview_prev': 'Précédent', + 'playlist.preview_next': 'Suivant', + 'playlist.preview_position': '{current} sur {total}', + 'playlist.preview_zoned': 'Les zones jouent ensemble', 'playlist.toast.created': 'Liste créée', 'playlist.toast.deleted': 'Liste supprimée', 'playlist.toast.published': 'Liste publiée — appareils mis à jour', diff --git a/frontend/js/i18n/it.js b/frontend/js/i18n/it.js index 7793366..091c2d9 100644 --- a/frontend/js/i18n/it.js +++ b/frontend/js/i18n/it.js @@ -751,6 +751,10 @@ export default { 'playlist.draft.discard_changes': 'Scarta Modifiche', 'playlist.draft.publish': 'Pubblica', 'playlist.draft.publishing': 'Pubblicazione...', + 'playlist.preview_prev': 'Precedente', + 'playlist.preview_next': 'Successivo', + 'playlist.preview_position': '{current} di {total}', + 'playlist.preview_zoned': 'Le zone vengono riprodotte insieme', 'playlist.toast.created': 'Playlist creata', 'playlist.toast.deleted': 'Playlist eliminata', 'playlist.toast.published': 'Playlist pubblicata — dispositivi aggiornati', diff --git a/frontend/js/i18n/pt.js b/frontend/js/i18n/pt.js index bc4a8c9..e7a90d5 100644 --- a/frontend/js/i18n/pt.js +++ b/frontend/js/i18n/pt.js @@ -763,6 +763,10 @@ export default { 'playlist.draft.discard_changes': 'Descartar alterações', 'playlist.draft.publish': 'Publicar', 'playlist.draft.publishing': 'Publicando...', + 'playlist.preview_prev': 'Anterior', + 'playlist.preview_next': 'Próximo', + 'playlist.preview_position': '{current} de {total}', + 'playlist.preview_zoned': 'As zonas tocam juntas', 'playlist.toast.created': 'Playlist criada', 'playlist.toast.deleted': 'Playlist excluída', 'playlist.toast.published': 'Playlist publicada — dispositivos atualizados', diff --git a/frontend/js/views/playlists.js b/frontend/js/views/playlists.js index ec3fc10..96732db 100644 --- a/frontend/js/views/playlists.js +++ b/frontend/js/views/playlists.js @@ -227,28 +227,81 @@ function showPlaylistPreview(playlist) {
- + + +
+
+ +   +
`; document.body.appendChild(overlay); const frame = overlay.querySelector('#pvpFrame'); const btnL = overlay.querySelector('#pvpLandscape'); const btnP = overlay.querySelector('#pvpPortrait'); + const btnPrev = overlay.querySelector('#pvpPrev'); + const btnNext = overlay.querySelector('#pvpNext'); + const position = overlay.querySelector('#pvpPosition'); + + // #239: skip/next. The preview already IS the real player in device-free preview mode, so the + // control is a message to that one iframe rather than a second copy of the playback logic. + // Addressing frame.contentWindow (not a broadcast) and pinning targetOrigin to our own origin is + // what keeps this off any real screen: a live display holds a socket to the server and is not + // reachable from this page at all, and the preview player itself ignores the message unless it + // booted with ?preview=1. + const send = (action) => { + try { frame.contentWindow?.postMessage({ source: 'screentinker-preview', action }, window.location.origin); } catch (e) {} + }; + const onPlayerMessage = (ev) => { + if (ev.origin !== window.location.origin) return; + if (ev.source !== frame.contentWindow) return; // ignore any other frame on the page + const d = ev.data; + if (!d || d.source !== 'screentinker-player' || d.type !== 'preview:state') return; + // A multi-zone playlist plays all zones at once, so there is no single item to step through — + // showing a counter there would be a lie and the buttons would appear dead. + if (d.zoned || !d.total) { + btnPrev.disabled = btnNext.disabled = true; + position.textContent = d.zoned ? t('playlist.preview_zoned') : ''; + return; + } + btnPrev.disabled = btnNext.disabled = false; + position.textContent = t('playlist.preview_position', { current: (d.index >= 0 ? d.index : 0) + 1, total: d.total }); + }; + window.addEventListener('message', onPlayerMessage); + // The player posts its state as soon as it has content, but an orientation reload restarts it — + // ask again on every load so the counter can never be left stale from the previous run. + frame.addEventListener('load', () => send('sync')); + const setOrientation = (o) => { orientation = o; frame.style.aspectRatio = aspect(); + btnPrev.disabled = btnNext.disabled = true; // reloading: no item until the player says so + position.textContent = ''; frame.src = frameSrc(); btnL.className = 'btn btn-sm ' + (o === 'landscape' ? 'btn-primary' : 'btn-secondary'); btnP.className = 'btn btn-sm ' + (o.startsWith('portrait') ? 'btn-primary' : 'btn-secondary'); }; btnL.onclick = () => setOrientation('landscape'); btnP.onclick = () => setOrientation('portrait'); - const close = () => overlay.remove(); + btnPrev.onclick = () => send('prev'); + btnNext.onclick = () => send('next'); + // Listeners are on window/document, so they outlive the overlay unless close() takes them with + // it — a leaked keydown handler would keep firing at a closed preview. + const close = () => { + overlay.remove(); + window.removeEventListener('message', onPlayerMessage); + document.removeEventListener('keydown', onKey); + }; + function onKey(ev) { + if (ev.key === 'Escape') close(); + else if (ev.key === 'ArrowRight') send('next'); + else if (ev.key === 'ArrowLeft') send('prev'); + } overlay.querySelector('#pvpClose').onclick = close; overlay.onclick = (e) => { if (e.target === overlay) close(); }; - document.addEventListener('keydown', function esc(ev) { - if (ev.key === 'Escape') { close(); document.removeEventListener('keydown', esc); } - }); + document.addEventListener('keydown', onKey); } /* diff --git a/server/player/index.html b/server/player/index.html index 3e3031c..b32d3f7 100644 --- a/server/player/index.html +++ b/server/player/index.html @@ -1119,6 +1119,7 @@ // straight to the UNMODIFIED renderer. No socket, no pairing. async function renderPreviewFromUrl(url) { PREVIEW_MODE = true; + installPreviewControlChannel(); // #239: only a preview instance can ever be steered config.serverUrl = window.location.origin; // same-origin -> /uploads + /api/widgets resolve const setup = document.getElementById('setupScreen'); if (setup) setup.style.display = 'none'; @@ -1134,6 +1135,7 @@ } renderPreviewBanner(); handlePlaylistUpdate(payload); + postPreviewState(); // #239: first count, in case the payload landed before the first item did } catch (e) { console.error('preview fetch failed', e); showPreviewError(0); @@ -1166,6 +1168,76 @@ document.body.appendChild(div); } + // ==================== #239 Preview transport control ==================== + // Where a next/previous press lands. Kept as a pure function (no DOM, no globals) because the + // failure it prevents is arithmetic: a negative modulo or an out-of-range index mounts + // `undefined` and blanks the frame, and that is only catchable by testing the maths directly. + // + // `allows` is the per-item schedule gate. Scanning in the DIRECTION OF TRAVEL matters: skipping + // a dayparted item by falling forward would make "previous" walk forwards, which reads as the + // button being broken. Returns -1 when nothing is playable (empty list, or every item outside + // its window) so the caller can idle instead of mounting nothing. + function previewStepIndex(current, delta, length, allows) { + const n = Math.trunc(length); + if (!Number.isFinite(n) || n <= 0) return -1; + const dir = delta < 0 ? -1 : 1; + // A player that has not started yet (currentIndex === -1) or a corrupt index still has to + // produce a valid landing spot rather than propagate NaN into playlist[]. + const from = Number.isFinite(current) ? ((Math.trunc(current) % n) + n) % n : 0; + for (let i = 1; i <= n; i++) { + const idx = (((from + dir * i) % n) + n) % n; + if (!allows || allows(idx)) return idx; + } + return -1; + } + + // Jump the preview by one item. PREVIEW_MODE is the hard gate: a live player never installs the + // message listener below AND would refuse here anyway, so a page that frames the player can + // never steer content on a real screen. + function previewNavigate(delta) { + if (!PREVIEW_MODE) return; + const idx = previewStepIndex(currentIndex, delta, playlist.length, (i) => scheduleAllows(playlist[i])); + if (idx === -1) return; + clearTimeout(scheduleRetryTimer); // we are leaving the idle screen by hand + currentIndex = idx; + isPlaying = true; + playCurrentItem(); // re-renders, which clears the pending advance timer + } + + // Tell the dashboard which item is on screen so it can show "3 of 7". targetOrigin is our own + // origin, so a third-party page that iframes the player learns nothing about the workspace's + // content from this channel. + function postPreviewState() { + if (!PREVIEW_MODE || window.parent === window) return; + const item = playlist[currentIndex]; + // A multi-zone playlist plays every zone at once on independent timers — there is no single + // "current item" to step through, so say so and let the dashboard hide the controls rather + // than offer a button that does nothing visible. + const zoned = !!(layout && layout.zones && layout.zones.length > 1); + try { + window.parent.postMessage({ + source: 'screentinker-player', + type: 'preview:state', + index: currentIndex, + total: playlist.length, + zoned, + name: (item && (item.filename || item.widget_name || item.title)) || null, + }, window.location.origin); + } catch (e) { /* parent went away mid-preview */ } + } + + function installPreviewControlChannel() { + window.addEventListener('message', (ev) => { + if (!PREVIEW_MODE) return; // belt and braces: see previewNavigate + if (ev.origin !== window.location.origin) return; // only our own dashboard may drive us + const d = ev.data; + if (!d || d.source !== 'screentinker-preview') return; + if (d.action === 'next') previewNavigate(1); + else if (d.action === 'prev') previewNavigate(-1); + else if (d.action === 'sync') postPreviewState(); // parent (re)attached and wants the count + }); + } + // #104: the always-visible honest note for webpage widgets. No auto-detection — // an XFO-refused frame is provably indistinguishable client-side from a working // one, so we never guess; we just tell the truth. Preview-only (never on device). @@ -2617,6 +2689,10 @@ renderContent(item); + // #239: the dashboard's "3 of 7" follows the player's own advance, not just operator presses, + // so it stays honest when an item ends on its own duration. + if (PREVIEW_MODE) postPreviewState(); + // Push an immediate sync so followers don't have to wait up to 1s for // the next periodic tick before snapping to the new item. if (wallConfig?.is_leader) emitWallSync(); diff --git a/server/test/preview-skip.test.js b/server/test/preview-skip.test.js new file mode 100644 index 0000000..0eab488 --- /dev/null +++ b/server/test/preview-skip.test.js @@ -0,0 +1,171 @@ +'use strict'; + +// #239: the playlist preview had no way to skip, so reviewing item 8 of a playlist meant sitting +// through items 1–7 in real time. The fix drives the player the preview ALREADY embeds (an iframe +// of /player?preview=1) over postMessage, so there is no second copy of the playback logic. +// +// Two things can go wrong, and neither shows up in a screenshot: +// +// 1. The index maths. A negative modulo or an off-by-one lands on an index that isn't in the +// playlist, and playlist[idx] === undefined mounts nothing — the operator sees a black frame +// and assumes the content is broken. "Previous" is the dangerous direction: JS's % returns a +// negative for a negative left operand. +// 2. The blast radius. A control that reached a REAL display would let an operator skip content +// on a wall in front of customers — far worse than the bug being fixed. So the channel is +// asserted here at the source level: only a player that booted in preview mode may be steered, +// and the dashboard addresses exactly one iframe rather than broadcasting. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const PLAYER = fs.readFileSync(path.join(__dirname, '..', 'player', 'index.html'), 'utf8'); +const DASHBOARD = fs.readFileSync( + path.join(__dirname, '..', '..', 'frontend', 'js', 'views', 'playlists.js'), 'utf8'); + +// Same lift as the other player tests: pull one pure function out of the single-file player and +// run it directly, so the maths is testable without a browser. +function lift(name) { + const start = PLAYER.indexOf(`function ${name}(`); + assert.notEqual(start, -1, `${name}() should exist in the player`); + let depth = 0; + for (let j = PLAYER.indexOf('{', start); j < PLAYER.length; j++) { + if (PLAYER[j] === '{') depth++; + else if (PLAYER[j] === '}' && --depth === 0) { + return new Function(`${PLAYER.slice(start, j + 1)} return ${name};`)(); + } + } + throw new Error('unbalanced braces reading ' + name); +} +const previewStepIndex = lift('previewStepIndex'); + +const NEXT = 1; +const PREV = -1; + +test('next walks the playlist in order', () => { + assert.equal(previewStepIndex(0, NEXT, 7), 1); + assert.equal(previewStepIndex(5, NEXT, 7), 6); +}); + +test('next wraps past the last item back to the first', () => { + assert.equal(previewStepIndex(6, NEXT, 7), 0); +}); + +test('THE BUG previous must not reproduce: stepping back from item 1 wraps to the LAST item', () => { + // (0 - 1) % 7 is -1 in JavaScript, and playlist[-1] is undefined — a black frame. + assert.equal(previewStepIndex(0, PREV, 7), 6); +}); + +test('previous walks backwards', () => { + assert.equal(previewStepIndex(6, PREV, 7), 5); + assert.equal(previewStepIndex(1, PREV, 7), 0); +}); + +test('a one-item playlist lands on that item in either direction', () => { + // Re-rendering the single item is the honest answer: there is nowhere else to go, and returning + // -1 would idle a preview that has perfectly good content. + assert.equal(previewStepIndex(0, NEXT, 1), 0); + assert.equal(previewStepIndex(0, PREV, 1), 0); +}); + +test('an empty playlist has nowhere to go', () => { + assert.equal(previewStepIndex(0, NEXT, 0), -1); + assert.equal(previewStepIndex(-1, PREV, 0), -1); +}); + +test('a player that has not started yet still lands on a real item', () => { + // currentIndex is -1 until the first item mounts; a press during that window must not produce + // -2 or NaN. + assert.equal(previewStepIndex(-1, NEXT, 4), 0); + assert.equal(previewStepIndex(-1, PREV, 4), 2); +}); + +test('a corrupt or out-of-range index is normalised, never propagated', () => { + assert.equal(previewStepIndex(NaN, NEXT, 4), 1); + assert.equal(previewStepIndex(undefined, NEXT, 4), 1); + assert.equal(previewStepIndex(9, NEXT, 4), 2); // 9 % 4 === 1 -> next is 2 + assert.equal(previewStepIndex(-9, NEXT, 4), 0); // -9 normalises to 3 -> wraps to 0 + assert.equal(previewStepIndex(1.7, NEXT, 4), 2); +}); + +test('a non-numeric length is treated as no playlist rather than looping forever', () => { + assert.equal(previewStepIndex(0, NEXT, NaN), -1); + assert.equal(previewStepIndex(0, NEXT, undefined), -1); + assert.equal(previewStepIndex(0, NEXT, -3), -1); +}); + +test('dayparted items are skipped IN THE DIRECTION OF TRAVEL', () => { + // Falling forward past a filtered item would make "previous" walk forwards, which reads to the + // operator as the button being broken. + const off = new Set([1, 2]); + const allows = (i) => !off.has(i); + assert.equal(previewStepIndex(0, NEXT, 5, allows), 3); + assert.equal(previewStepIndex(3, PREV, 5, allows), 0); + assert.equal(previewStepIndex(0, PREV, 5, allows), 4); +}); + +test('every item outside its schedule window means nowhere to go, not item 0', () => { + // The caller idles on -1. Returning an index here would mount content the schedule says must not + // be on screen — the preview would then be lying about what the display will show. + assert.equal(previewStepIndex(2, NEXT, 5, () => false), -1); + assert.equal(previewStepIndex(2, PREV, 5, () => false), -1); +}); + +test('a step lands on an allowed item even when it has to wrap the whole list', () => { + const allows = (i) => i === 0; + assert.equal(previewStepIndex(0, NEXT, 6, allows), 0); + assert.equal(previewStepIndex(4, PREV, 6, allows), 0); +}); + +// ---- Blast radius: the control must never reach a live display ---- + +test('only a preview instance can be steered', () => { + // previewNavigate is the single entry point for a skip, and PREVIEW_MODE is set exactly once, + // in the ?preview=1 boot path (renderPreviewFromUrl) — a paired display never sets it. + const nav = PLAYER.slice(PLAYER.indexOf('function previewNavigate(')); + assert.match(nav.slice(0, 200), /if \(!PREVIEW_MODE\) return;/, + 'previewNavigate must refuse to act outside preview mode'); + assert.equal((PLAYER.match(/PREVIEW_MODE = true;/g) || []).length, 1, + 'preview mode should have exactly one assignment — the preview boot path'); +}); + +test('the message listener exists only in preview mode and only for our own origin', () => { + // A live player never installs the listener at all, so a page that iframes a real display cannot + // even attempt a skip; the origin check stops a third-party framer of the preview itself. + const calls = PLAYER.match(/installPreviewControlChannel\(\)/g) || []; + assert.equal(calls.length, 2, 'expected one definition call site plus the preview boot call'); + const boot = PLAYER.slice(PLAYER.indexOf('PREVIEW_MODE = true;'), + PLAYER.indexOf('PREVIEW_MODE = true;') + 200); + assert.match(boot, /installPreviewControlChannel\(\)/, + 'the channel must be installed by the preview boot, not at load time'); + const channel = PLAYER.slice(PLAYER.indexOf('function installPreviewControlChannel(')); + assert.match(channel.slice(0, 800), /ev\.origin !== window\.location\.origin/, + 'cross-origin messages must be rejected'); + assert.match(channel.slice(0, 800), /d\.source !== 'screentinker-preview'/, + 'unrelated postMessage traffic (extensions, embeds) must be ignored'); +}); + +test('preview state is posted to our origin only — never "*"', () => { + // "*" would hand the workspace's playlist contents to any page that framed the player. + const post = PLAYER.slice(PLAYER.indexOf('function postPreviewState(')); + assert.match(post.slice(0, 900), /window\.location\.origin\)/); + assert.doesNotMatch(post.slice(0, 900), /postMessage\([^)]*'\*'/); +}); + +test('the dashboard addresses the preview iframe, not a broadcast', () => { + // Broadcasting would still not reach a display (they hold a server socket, not a window handle), + // but addressing one contentWindow keeps the intent unambiguous and pins the target origin. + assert.match(DASHBOARD, /frame\.contentWindow\?\.postMessage\(\{ source: 'screentinker-preview', action \}, window\.location\.origin\)/); + assert.match(DASHBOARD, /ev\.source !== frame\.contentWindow/); + assert.match(DASHBOARD, /ev\.origin !== window\.location\.origin/); +}); + +test('the preview modal cleans up its window-level listeners on close', () => { + // They outlive the overlay otherwise, and a leaked keydown handler keeps posting arrow-key skips + // at a preview the operator already closed. + const modal = DASHBOARD.slice(DASHBOARD.indexOf('function showPlaylistPreview('), + DASHBOARD.indexOf('function layoutMockup(')); + assert.match(modal, /window\.removeEventListener\('message', onPlayerMessage\)/); + assert.match(modal, /document\.removeEventListener\('keydown', onKey\)/); +});