From 9034478c282c3e9b48a93da6ea382b81edaf7552 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 22:11:40 -0500 Subject: [PATCH 1/3] Android: a YouTube item must end on its duration, and clearing a playlist must apply at once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A screen kept showing a YouTube video after its playlist was reassigned, and kept showing it after "no playlist" was selected. Restarting the app showed the new content immediately, which ruled out the network, the download and the server payload. Two faults met: 1. Nothing ever ended a YouTube item. playCurrentItem armed an advance only for images and widgets; video/youtube is neither, and it is played by loading an embed into a WebView, which reports no completion. playYoutube even took the item's durationSec and never read it. So any playlist containing a YouTube item stopped rotating at that item permanently — broader than what was reported. The web and Tizen players both already time YouTube off its duration; Android was the only player that did not, so this brings it back in line. 2. #157 defers a playlist change when the item on screen is dropped from the new list, applying it at the next natural advance. With no advance ever coming, the change was stranded. An EMPTY new list went down the same path, so "no playlist" — the one action that should always take effect immediately — was deferred too. Fixed all three layers: video/youtube now ends on a timer (ItemTiming), an empty list is never deferred (PendingSwap), and a deferral gets a 60s deadline so no future item type that ends on a callback can strand a swap again. Local and remote video stay off the timer path, where STATE_ENDED drives them, so clips are not cut short. The deferral rule and the timing rule are pure seams, tested without a device: 126 Android JVM tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- .../player/player/PlaylistController.kt | 57 ++++++++++- .../player/player/PlaylistSelection.kt | 59 +++++++++++ .../player/player/PendingSwapTest.kt | 98 +++++++++++++++++++ 3 files changed, 210 insertions(+), 4 deletions(-) create mode 100644 android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt index d1b2e42..d82c7c2 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt +++ b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt @@ -111,7 +111,7 @@ class PlaylistController( val item = currentItem ?: return val delay = FollowerExit.resumeDelayMs( isRunning = isRunning, - isImageOrWidget = item.mimeType.startsWith("image/") || item.isWidget, + isImageOrWidget = endsOnTimer(item), slotMs = slotMs(item), elapsedMs = System.currentTimeMillis() - itemStartedAt ) ?: return @@ -220,8 +220,17 @@ class PlaylistController( // In solo playback, don't interrupt it: keep it up, stash the new list, and rotate out on the // next natural advance (video end / image duration). Excludes wallFollower + group-sync, whose // advance is driven by their tick, not next() — deferring there would strand the swap. - if (isRunning && !wallFollower && hasContentOnScreen && currentlyPlayingId != null && - newItems.none { it.contentId == currentlyPlayingId }) { + // An EMPTY new list is never a deferral candidate. Clearing a screen's playlist is an + // explicit "stop showing that" from an operator, not an item rotating out — deferring it + // meant selecting "no playlist" left the old content up indefinitely, which is the opposite + // of what was asked for and looked like the setting had done nothing. + if (PendingSwap.shouldDefer( + isRunning = isRunning, + wallFollower = wallFollower, + hasContentOnScreen = hasContentOnScreen, + currentlyPlayingId = currentlyPlayingId, + newContentIds = newItems.map { it.contentId }, + )) { var succ: String? = null if (items.isNotEmpty()) { for (k in 1..items.size) { @@ -232,11 +241,13 @@ class PlaylistController( pendingItems = newItems pendingSuccessorId = succ Log.i("PlaylistController", "Current item removed but still live — deferring rotation-out (successor=$succ)") + armPendingSwapDeadline() return } // A non-deferred structural update supersedes any pending swap. pendingItems = null pendingSuccessorId = null + cancelPendingSwapDeadline() items.clear() items.addAll(newItems) @@ -330,6 +341,7 @@ class PlaylistController( isRunning = false cancelAdvance() cancelRetry() + cancelPendingSwapDeadline() // else a stopped controller can still fire next() hasContentOnScreen = false pendingItems = null pendingSuccessorId = null @@ -343,6 +355,7 @@ class PlaylistController( // Swap in the stashed list now and continue at the preserved successor (or first playable). pendingItems?.let { p -> pendingItems = null + cancelPendingSwapDeadline() val succ = pendingSuccessorId; pendingSuccessorId = null items.clear(); items.addAll(p) if (items.isEmpty()) { currentIndex = -1; cancelAdvance(); onPlaylistEmpty(); return } @@ -373,6 +386,42 @@ class PlaylistController( next() } + /** + * Items whose turn ends on a TIMER rather than a completion callback. + * + * video/youtube belongs here and did not: it is played by loading an embed into a WebView, which + * fires no completion event, so nothing ever advanced past it. playYoutube() even takes a + * durationSec and never reads it. A playlist containing a YouTube item simply stopped there. + * + * That also stranded #157's deferred swap, which waits for "the next natural advance": assigning + * a different playlist while a YouTube item was on screen deferred forever, so the change looked + * like it had been ignored. Reported as "I assigned Playlist 2 and it kept showing the video". + * + * Local and remote non-YouTube video stay off this list — ExoPlayer reports STATE_ENDED and + * onVideoComplete drives those, and a timer would cut a clip short. + */ + private var pendingSwapRunnable: Runnable? = null + + /** Apply a deferred swap even if no advance arrives — see PENDING_SWAP_DEADLINE_MS. */ + private fun armPendingSwapDeadline() { + cancelPendingSwapDeadline() + pendingSwapRunnable = Runnable { + if (pendingItems != null) { + Log.w("PlaylistController", "Deferred playlist swap never got an advance — applying it now") + next() + } + } + handler.postDelayed(pendingSwapRunnable!!, PendingSwap.DEADLINE_MS) + } + + private fun cancelPendingSwapDeadline() { + pendingSwapRunnable?.let { handler.removeCallbacks(it) } + pendingSwapRunnable = null + } + + private fun endsOnTimer(item: PlaylistItem): Boolean = + ItemTiming.endsOnTimer(item.mimeType, item.isWidget) + private fun playCurrentItem() { cancelAdvance() cancelRetry() @@ -395,7 +444,7 @@ class PlaylistController( // For images and widgets, auto-advance after duration. For videos, wait // for the completion callback. Wall followers never auto-advance — the // leader's wall:sync index drives every switch. - if (!wallFollower && (item.mimeType.startsWith("image/") || item.isWidget)) { + if (!wallFollower && endsOnTimer(item)) { // slotMs() floors a zero/negative duration to 10s (the max(1, duration||10) // contract shared with the web/Tizen players). A raw durationSec*1000 here let a // solo fullscreen widget with duration_sec=0 schedule a 0ms advance -> self-loop. diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt index 483464c..0936f0b 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt +++ b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt @@ -76,3 +76,62 @@ object PlaybackResume { return savedIndex } } + +/** + * #157's deferral: when a playlist update drops the item that is CURRENTLY on screen, we let that + * item finish its turn instead of yanking it, and apply the new list at the next natural advance. + * + * The rule needs two guards it did not have, both found from a customer report where a playlist + * change appeared to be ignored entirely: + * + * 1. An EMPTY new list is not a rotation. Clearing a screen's playlist is an operator saying "stop + * showing that", so it must take effect now. Deferring it left the old content up forever. + * 2. Deferring assumes an advance is coming. A YouTube item never advanced (see endsOnTimer), so + * the pending swap was stranded permanently — the caller must pair this with a deadline. + * + * Pure so the rule can be checked without a device or a WebView. + */ +object PendingSwap { + /** + * How long a deferred swap may wait for "the next natural advance" before it is applied anyway. + * The deferral assumes an advance is coming; YouTube proved it might not be, and any future item + * type that ends on a callback could do the same. Must comfortably clear an ordinary dwell so it + * never pre-empts a normal rotation, while still being short enough that an operator watching + * the screen sees their change land. + */ + const val DEADLINE_MS = 60_000L + + /** + * Whether a playlist update should wait for the current item to finish. + * False means apply it immediately. + */ + fun shouldDefer( + isRunning: Boolean, + wallFollower: Boolean, + hasContentOnScreen: Boolean, + currentlyPlayingId: String?, + newContentIds: List, + ): Boolean { + if (!isRunning || wallFollower || !hasContentOnScreen) return false + if (currentlyPlayingId == null) return false + if (newContentIds.isEmpty()) return false // guard 1: an explicit stop + return !newContentIds.contains(currentlyPlayingId) + } +} + +/** + * Which items end on a TIMER versus a completion callback. + * + * video/youtube was in neither camp and so ended on nothing at all: it is played by loading an embed + * into a WebView, which reports no completion, and no advance was ever armed for it. The item's + * configured duration was passed to the player and dropped on the floor. A playlist containing a + * YouTube item simply stopped there for good, and any pending playlist change stopped with it. + * + * Local and remote video deliberately stay OFF the timer path — the player reports STATE_ENDED for + * those and a timer would cut a clip short at its configured duration. + */ +object ItemTiming { + fun endsOnTimer(mimeType: String, isWidget: Boolean): Boolean = + mimeType.startsWith("image/") || isWidget || mimeType == "video/youtube" +} + diff --git a/android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt b/android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt new file mode 100644 index 0000000..a82b2f5 --- /dev/null +++ b/android/app/src/test/java/com/remotedisplay/player/player/PendingSwapTest.kt @@ -0,0 +1,98 @@ +package com.remotedisplay.player.player + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * A customer assigned a different playlist to a screen and the screen kept showing the old content. + * Then they selected "no playlist" — still the old content. Restarting the app showed the new + * content instantly, which ruled out downloads, the network and the server payload. + * + * Two faults met. #157's deferral holds a playlist change until the current item finishes its turn, + * and the item on screen was a YouTube video, which never finished: nothing armed an advance for it, + * so the pending change waited for an event that could not arrive. And "no playlist" went down the + * same deferral path, so the one action that should always take effect immediately did not. + * + * Invariants pinned here: + * - an empty new list is applied at once, never deferred + * - a real rotation still defers, because #157's reason for existing has not changed + * - an item that ends on a timer is recognised as such, YouTube included + */ +class PendingSwapTest { + + private val LIVE = "content-on-screen" + + private fun defer( + newIds: List, + current: String? = LIVE, + isRunning: Boolean = true, + wallFollower: Boolean = false, + hasContent: Boolean = true, + ) = PendingSwap.shouldDefer(isRunning, wallFollower, hasContent, current, newIds) + + @Test fun THE_BUG_selecting_no_playlist_must_not_be_deferred() { + // The decisive observation from the report: "I selected No playlist ... it still showed the + // same video." An empty list is an operator saying stop, not an item rotating out. + assertFalse(defer(newIds = emptyList())) + } + + @Test fun a_genuine_rotation_still_defers_157_must_not_regress() { + // The current item is gone from the new list but other items remain: let it finish. + assertTrue(defer(newIds = listOf("other-a", "other-b"))) + } + + @Test fun a_playlist_that_still_contains_the_live_item_never_defers() { + assertFalse(defer(newIds = listOf(LIVE, "other-a"))) + } + + @Test fun nothing_on_screen_yet_means_apply_immediately() { + // A first load has nothing to protect, so there is nothing to wait for. + assertFalse(defer(newIds = listOf("other-a"), hasContent = false)) + assertFalse(defer(newIds = listOf("other-a"), current = null)) + } + + @Test fun a_stopped_controller_does_not_defer() { + // Otherwise a swap is parked on an instance that will never advance again. + assertFalse(defer(newIds = listOf("other-a"), isRunning = false)) + } + + @Test fun a_wall_follower_does_not_defer_it_obeys_the_leader() { + assertFalse(defer(newIds = listOf("other-a"), wallFollower = true)) + } + + @Test fun the_deferral_deadline_is_long_enough_for_a_normal_item_and_short_enough_to_notice() { + // The deadline is the backstop for "no advance ever arrives". It must clear a typical dwell + // comfortably (or it would cut ordinary items short) while still resolving fast enough that + // an operator watching the screen sees their change land. + val deadline = PendingSwap.DEADLINE_MS + assertTrue("deadline must exceed a common 30s dwell", deadline > 30_000L) + assertTrue("an operator should not wait minutes", deadline <= 120_000L) + } +} + +/** + * The other half of the same report. A YouTube item ended on nothing: no timer was armed for it and + * a WebView embed reports no completion, so it held the screen forever and stranded whatever + * playlist change was waiting behind it. + */ +class ItemTimingTest { + + @Test fun THE_BUG_a_youtube_item_must_end_on_a_timer() { + // Nothing else can end it — a WebView embed fires no completion event. + assertTrue(ItemTiming.endsOnTimer("video/youtube", isWidget = false)) + } + + @Test fun images_and_widgets_are_timed_as_they_always_were() { + assertTrue(ItemTiming.endsOnTimer("image/jpeg", isWidget = false)) + assertTrue(ItemTiming.endsOnTimer("image/png", isWidget = false)) + assertTrue(ItemTiming.endsOnTimer("text/html", isWidget = true)) + } + + @Test fun real_video_must_NOT_be_timed_or_clips_get_cut_short() { + // These end on STATE_ENDED. Arming a timer would truncate a clip at its configured duration, + // which is the regression to avoid while fixing the YouTube case. + assertFalse(ItemTiming.endsOnTimer("video/mp4", isWidget = false)) + assertFalse(ItemTiming.endsOnTimer("video/webm", isWidget = false)) + } +} From 5297f091af9c65b5d5391821d2b6fa598048132d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 18:29:18 -0500 Subject: [PATCH 2/3] Let a display's playlist actually be cleared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "No playlist" was an option you could select that did nothing. The picker offered it, and the change handler opened with `if (!newPlaylistId) return; // Don't allow deselecting for now` — so choosing it sent no request, changed nothing, and said nothing. The guard was honest about why: there was no way to do it. PUT /devices/:id has never read playlist_id (200, ignored), and POST /playlists/:id/assign can only ever set one. Reported on #234 as "I also selected No playlist ... it still showed the same video". It did, and my first explanation blamed the playlist-swap deferral. The deferral would have stranded it too — that is fixed separately and tested — but on this path nothing was ever sent, so the deferral never got the chance. DELETE /api/devices/:id/playlist, device-scoped rather than playlist-scoped because there is no playlist to authorize against when clearing. Ownership goes through checkDeviceOwnership like every other device mutation, so a viewer and a stranger are refused. Clearing an already-clear display is a no-op success, since it lives in a dropdown someone can pick twice. The now-empty playlist is pushed to the device so the screen stops, rather than leaving the old content up until something else happens to refresh it. Validated on an Android 12 emulator against the reporter's shape: cleared while a YouTube item was on screen, zero plays afterwards, device row cleared. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- frontend/js/api.js | 1 + frontend/js/views/device-detail.js | 11 ++- server/routes/devices.js | 33 +++++++ server/test/clear-device-playlist.test.js | 107 ++++++++++++++++++++++ 4 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 server/test/clear-device-playlist.test.js diff --git a/frontend/js/api.js b/frontend/js/api.js index 9ec7996..9fb8b82 100644 --- a/frontend/js/api.js +++ b/frontend/js/api.js @@ -193,6 +193,7 @@ export const api = { getItemSchedules: (id, itemId) => request(`/playlists/${id}/items/${itemId}/schedules`), setItemSchedules: (id, itemId, blocks) => request(`/playlists/${id}/items/${itemId}/schedules`, { method: 'PUT', body: JSON.stringify({ blocks }) }), assignPlaylistToDevice: (playlistId, device_id) => request(`/playlists/${playlistId}/assign`, { method: 'POST', body: JSON.stringify({ device_id }) }), + clearDevicePlaylist: (device_id) => request(`/devices/${device_id}/playlist`, { method: 'DELETE' }), publishPlaylist: (id) => request(`/playlists/${id}/publish`, { method: 'POST' }), discardPlaylistDraft: (id) => request(`/playlists/${id}/discard`, { method: 'POST' }), diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index 07df81b..53b6159 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -1039,10 +1039,15 @@ function setupActions(device) { playlistPicker.addEventListener('change', async () => { const newPlaylistId = playlistPicker.value; - if (!newPlaylistId) return; // Don't allow deselecting for now try { - await api.assignPlaylistToDevice(newPlaylistId, device.id); - device.playlist_id = newPlaylistId; + // Empty value is the "No playlist" option. It used to be discarded right here, so the + // option was offered, selecting it did nothing, and nothing said so (#234). + if (newPlaylistId) { + await api.assignPlaylistToDevice(newPlaylistId, device.id); + } else { + await api.clearDevicePlaylist(device.id); + } + device.playlist_id = newPlaylistId || null; const assignments = await api.getAssignments(device.id); const pc = document.getElementById('playlistContainer'); pc.innerHTML = renderPlaylist(assignments); diff --git a/server/routes/devices.js b/server/routes/devices.js index 30e56b5..e098d59 100644 --- a/server/routes/devices.js +++ b/server/routes/devices.js @@ -215,6 +215,39 @@ router.get('/:id/preview-payload', (req, res) => { }); // Update device +// Clear a device's playlist — the "No playlist" option in the dashboard picker. +// +// There was no way to do this. PUT /devices/:id ignores playlist_id (it always has), and +// POST /playlists/:id/assign can only ever SET one, so the picker carried a guard that +// silently discarded the selection: `if (!newPlaylistId) return; // Don't allow deselecting`. +// The option was offered, selecting it did nothing, and no error said so — reported on #234 +// as "I selected No playlist and it still showed the same video". It did. +// +// Device-scoped rather than playlist-scoped because there is no playlist to authorize +// against when clearing; ownership is checked the same way every other device mutation +// checks it. Clearing an already-clear device is a no-op success, so the button is safe to +// press twice. +router.delete('/:id/playlist', (req, res) => { + const device = checkDeviceOwnership(req, res); + if (!device) return; + + db.prepare('UPDATE devices SET playlist_id = NULL, updated_at = ? WHERE id = ?') + .run(Math.floor(Date.now() / 1000), req.params.id); + + // Push the now-empty playlist so the screen stops, rather than leaving the old content up + // until something else happens to update it. + try { + const io = req.app.get('io'); + if (io) { + const { buildPlaylistPayload } = require('../ws/deviceSocket'); + const commandQueue = require('../lib/command-queue'); + commandQueue.queueOrEmitPlaylistUpdate(io.of('/device'), req.params.id, buildPlaylistPayload); + } + } catch (e) { /* silent — the DB is the source of truth, the push is best-effort */ } + + res.json({ success: true }); +}); + router.put('/:id', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; diff --git a/server/test/clear-device-playlist.test.js b/server/test/clear-device-playlist.test.js new file mode 100644 index 0000000..b03b82b --- /dev/null +++ b/server/test/clear-device-playlist.test.js @@ -0,0 +1,107 @@ +'use strict'; + +// "No playlist" was an option you could select that did nothing. +// +// The dashboard picker offered ``, and its change handler +// opened with `if (!newPlaylistId) return; // Don't allow deselecting for now` — so choosing it +// sent no request, changed nothing, and raised no error. The guard was honest about why: there was +// no way to do it. PUT /devices/:id has never read playlist_id (it returns 200 and ignores it), and +// POST /playlists/:id/assign can only set one. +// +// Reported on #234 as "I also selected No playlist ... it still showed the same video". It did. +// +// The invariant: clearing a display's playlist actually clears it, and only the people allowed to +// change that display can do it. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'st-clearpl-')); +process.env.DATA_DIR = tmp; +process.env.JWT_SECRET = 'test-secret-clear-playlist'; + +const express = require('express'); +const { db } = require('../db/database'); +const { requireAuth, generateToken } = require('../middleware/auth'); + +// devices -> workspaces -> organizations -> users, FK-enforced, so seed the whole chain. +function seed(suffix) { + const u = 'u-' + suffix, o = 'o-' + suffix, ws = 'ws-' + suffix; + const dev = 'd-' + suffix, pl = 'p-' + suffix; + db.prepare("INSERT OR IGNORE INTO users (id, email, password_hash, role) VALUES (?, ?, 'x', 'user')") + .run(u, suffix + '@test.local'); + db.prepare('INSERT OR IGNORE INTO organizations (id, name, owner_user_id) VALUES (?, ?, ?)').run(o, 'org ' + suffix, u); + db.prepare('INSERT OR IGNORE INTO workspaces (id, organization_id, name) VALUES (?, ?, ?)').run(ws, o, 'ws ' + suffix); + // accessContext resolves through the MEMBERSHIP tables, not organizations.owner_user_id — + // seeding only the owner column gets a legitimate owner a 403 and looks like an authz bug. + db.prepare("INSERT OR IGNORE INTO organization_members (organization_id, user_id, role) VALUES (?, ?, 'org_owner')").run(o, u); + db.prepare("INSERT INTO playlists (id, name, workspace_id, user_id) VALUES (?, 'PL', ?, ?)").run(pl, ws, u); + db.prepare(`INSERT INTO devices (id, name, workspace_id, user_id, playlist_id, created_at, updated_at) + VALUES (?, 'Screen', ?, ?, ?, strftime('%s','now'), strftime('%s','now'))`).run(dev, ws, u, pl); + return { u, ws, dev, pl }; +} + +const mine = seed('mine'); +const theirs = seed('theirs'); + +const app = express(); +app.use(express.json()); +app.use('/api/devices', requireAuth, require('../routes/devices')); +const server = app.listen(0); + +const userRow = (id) => db.prepare('SELECT id, email, role FROM users WHERE id = ?').get(id); +const tokenFor = (u, ws) => generateToken(userRow(u), ws); + +async function del(deviceId, token) { + await new Promise(r => (server.listening ? r() : server.once('listening', r))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/devices/${deviceId}/playlist`, { + method: 'DELETE', + headers: token ? { Authorization: `Bearer ${token}` } : {}, + }); + return res.status; +} + +const playlistOf = (id) => db.prepare('SELECT playlist_id FROM devices WHERE id = ?').get(id).playlist_id; + +test('THE BUG: PUT /devices/:id silently ignores playlist_id, so it could not clear one', async () => { + // Pinned so nobody "fixes" the picker by pointing it back at PUT and re-creating the silence. + const src = fs.readFileSync(path.join(__dirname, '..', 'routes', 'devices.js'), 'utf8'); + const put = src.slice(src.indexOf("router.put('/:id'")); + const body = put.slice(0, put.indexOf('\nrouter.')); + assert.ok(!/playlist_id\s*[,=]/.test(body), 'PUT now touches playlist_id — update this test and the picker'); +}); + +test('THE FIX: clearing a playlist actually clears it', async () => { + assert.equal(playlistOf(mine.dev), mine.pl, 'precondition: a playlist is assigned'); + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null, 'the display must end up with no playlist'); +}); + +test('clearing an already-clear display is a harmless no-op', async () => { + // The button is in a dropdown a person can pick twice; it must not 404 or 500 on the second go. + assert.equal(await del(mine.dev, tokenFor(mine.u, mine.ws)), 200); + assert.equal(playlistOf(mine.dev), null); +}); + +test('someone else\'s display cannot be cleared', async () => { + const before = playlistOf(theirs.dev); + const status = await del(theirs.dev, tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); + assert.equal(playlistOf(theirs.dev), before, 'a refused call must not have changed anything'); +}); + +test('an unauthenticated caller cannot clear a playlist', async () => { + const before = playlistOf(theirs.dev); + assert.equal(await del(theirs.dev), 401); + assert.equal(playlistOf(theirs.dev), before); +}); + +test('a display that does not exist is refused, not invented', async () => { + const status = await del('no-such-device', tokenFor(mine.u, mine.ws)); + assert.ok(status === 403 || status === 404, `expected refusal, got ${status}`); +}); + +test.after(() => { server.close(); try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} }); From 301c76c3f7c2668846682b794cf60d66de079095 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 18:35:31 -0500 Subject: [PATCH 3/3] Let a display opt in to pre-release builds, so a test build is not reverted under the tester MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handing someone a test build was a trap. A prerelease sorts BELOW its own release — 1.9.25-fix234d is semver-older than 1.9.25 — so a sideloaded display asked "anything newer?", was correctly told yes, and updated itself straight back off the build we had asked someone to test. Same versionCode, so Android installed it without complaint. Silent, and within minutes. That is what happened on #234: the reporter installed the fix, tested for an evening, and reported nothing had changed. They were right. Their tablet was running the old code again by then, and I had told them it was fixed without ever checking what the device reported. Adds a per-display opt-in (devices.ota_beta, default 0, checkbox next to the OTA toggle). When set, the display keeps a prerelease of the CURRENT core instead of being pulled back to its release. Deliberately narrow in one direction and deliberately wide in the other: - Narrow: it only holds a prerelease of the core already installed. A plain release, a -patchN build, an upgrade to a newer core, and a display ahead of the server all behave exactly as before, and the flag defaults off so a fleet that never sets it is unaffected. - Wide: an opted-in display is exempted from the superseded-prerelease guard. That guard would otherwise pin a tester on an old test build permanently — an older-core prerelease is never offered anything, so they would have to notice and sideload their way out. Writing the test is what surfaced that; opting in must never mean never updating again. 9 tests covering both directions, including that shipping a newer release pulls a beta display back onto the release line. 845 server tests green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- frontend/js/i18n/en.js | 2 + frontend/js/views/device-detail.js | 5 ++ server/db/database.js | 6 +++ server/lib/ota-breaker.js | 18 ++++++- server/routes/devices.js | 7 ++- server/server.js | 9 +++- server/test/ota-beta-channel.test.js | 79 ++++++++++++++++++++++++++++ 7 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 server/test/ota-beta-channel.test.js diff --git a/frontend/js/i18n/en.js b/frontend/js/i18n/en.js index 9238bc4..d338fa9 100644 --- a/frontend/js/i18n/en.js +++ b/frontend/js/i18n/en.js @@ -524,6 +524,8 @@ export default { 'device.debug.toggle': 'Debug logging (live)', 'device.debug.hint': 'Streams player/zone logs from this device in real time. Turns off on its own when the device reconnects.', 'device.ota.toggle': 'Self-update (OTA)', + 'device.ota.beta': 'Accept pre-release builds', + 'device.ota.beta_hint': 'Keeps this display on a test build instead of updating it back to the current release. Only affects pre-releases of the version already installed — once a newer release ships, this display updates to it normally.', 'device.ota.hint': 'When off, this device is never offered an update — an MDM or operator owns its updates instead. Turn OFF for MDM-managed panels (e.g. Pivot/MAXHUB) so the app never shows a self-install dialog.', 'device.reboot_schedule.label': 'Nightly reboot', 'device.reboot_schedule.hint': 'Reboot this panel once a day at this device-local time (leave blank for off). A clean nightly reboot clears memory leaks and re-syncs the clock. Silent on device-owner panels; a no-op on panels that can\'t self-reboot.', diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index 53b6159..fad4b94 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -446,6 +446,10 @@ async function loadDevice(deviceId, activeTab = null) { ${t('device.ota.toggle')}
${t('device.ota.hint')}
+ +
${t('device.ota.beta_hint')}
@@ -977,6 +981,7 @@ function setupActions(device) { orientation: document.getElementById('deviceOrientation').value, default_content_id: document.getElementById('deviceDefaultContent').value || null, ota_enabled: document.getElementById('otaToggle')?.checked ? 1 : 0, + ota_beta: document.getElementById('otaBetaToggle')?.checked ? 1 : 0, reboot_schedule: document.getElementById('rebootSchedule')?.value || null, }); showToast(t('device.toast.settings_saved'), 'success'); diff --git a/server/db/database.js b/server/db/database.js index d49470c..e12bc4a 100644 --- a/server/db/database.js +++ b/server/db/database.js @@ -299,6 +299,12 @@ const migrations = [ // device an update (an MDM/operator owns its updates). Default 1 (self-update on). // UPDATE devices SET ota_enabled = 0 WHERE id = ''; (1 to re-enable) "ALTER TABLE devices ADD COLUMN ota_enabled INTEGER NOT NULL DEFAULT 1", + // Opt a single display into pre-release builds. Without this, handing someone a test build is a + // trap: a prerelease sorts BELOW its own release (1.9.25-fix234d < 1.9.25), so the next OTA check + // correctly "upgrades" the device straight back off the build you asked them to test — silently, + // within minutes. It cost a reporter on #234 an evening of testing code that had already been + // replaced under them. Set this and the display keeps a same-core prerelease. + "ALTER TABLE devices ADD COLUMN ota_beta INTEGER NOT NULL DEFAULT 0", // #161: privilege tier reported by the player (0 unprivileged / 1 device-admin / 2 owner-or- // delegated-install) + whether a foreign device owner (MDM) manages it. Drives dashboard gating // of Tier-2 controls (reboot/kiosk/time) — shown only for owned panels. diff --git a/server/lib/ota-breaker.js b/server/lib/ota-breaker.js index b359be0..0c072fd 100644 --- a/server/lib/ota-breaker.js +++ b/server/lib/ota-breaker.js @@ -68,7 +68,7 @@ function isReleased(p) { return p.pre === null || /^patch\d+$/i.test(p.pre); } // decide(clientVersion, latestVersion, deviceId?, now?) -> // { update_available, reason, retry_after_seconds?, log? } -function decide(clientVersion, latestVersion, deviceId = null, now = Date.now()) { +function decide(clientVersion, latestVersion, deviceId = null, now = Date.now(), betaChannel = false) { // ---- PHANTOM / unrecognized guard (immediate, version-based, no rate state) ---- if (!clientVersion) return { update_available: false, reason: 'no-version' }; const pc = parseVer(clientVersion), pl = parseVer(latestVersion); @@ -76,10 +76,24 @@ function decide(clientVersion, latestVersion, deviceId = null, now = Date.now()) const full = cmpParsed(pc, pl); if (full === 0) return { update_available: false, reason: 'up-to-date' }; if (full > 0) return { update_available: false, reason: 'client-newer' }; // never offer a downgrade - if (!isReleased(pc) && coreCmp(pc, pl) < 0) { // GENUINE superseded old-core prerelease (e.g. 1.9.1-beta4) — a -patchN release is NOT one, so it still gets offered + // betaChannel is exempt: this guard would otherwise strand the very displays we hand test + // builds to. A tester on 1.9.25-fix234d has an older core than a released 1.9.26, so without + // the exemption they are told "superseded" forever and never rejoin the release line — the + // opposite of what opting in should mean. Opting in must be reversible by shipping a release. + if (!betaChannel && !isReleased(pc) && coreCmp(pc, pl) < 0) { // GENUINE superseded old-core prerelease (e.g. 1.9.1-beta4) — a -patchN release is NOT one, so it still gets offered return { update_available: false, reason: 'superseded-prerelease', log: logOnce(clientVersion, `[ota] superseded prerelease '${clientVersion}' (older core than latest=${latestVersion}) — no offer`) }; } + // A display opted into pre-release builds keeps a prerelease of the CURRENT core. Semver puts + // 1.9.25-fix234d below 1.9.25, so without this the only "upgrade" on offer is dropping the very + // build we asked this display to run — which is how a test build silently reverts. Scoped to the + // same core on purpose: an older-core prerelease is genuinely stale and still gets offered, and + // once 1.9.26 ships a 1.9.25-anything device is behind and updates normally. So opting in cannot + // strand a display on an abandoned branch. + if (betaChannel && !isReleased(pc) && coreCmp(pc, pl) === 0) { + return { update_available: false, reason: 'beta-channel' }; + } + // ---- offerable (recent real older version) -> RATE breaker, keyed per device / per version ---- const key = deviceId ? 'd:' + deviceId : 'v:' + clientVersion; let b = state.get(key); diff --git a/server/routes/devices.js b/server/routes/devices.js index e098d59..faf455a 100644 --- a/server/routes/devices.js +++ b/server/routes/devices.js @@ -252,7 +252,7 @@ router.put('/:id', (req, res) => { const device = checkDeviceOwnership(req, res); if (!device) return; - const { name, notes, timezone, orientation, default_content_id, layout_id, ota_enabled, reboot_schedule } = req.body; + const { name, notes, timezone, orientation, default_content_id, layout_id, ota_enabled, ota_beta, reboot_schedule } = req.body; // #150: validate orientation against the known enum (previously accepted any string, which // let a bad value reach the player -> unknown rotation falls back to landscape silently). if (orientation !== undefined && !deviceSettings.ORIENTATIONS.has(orientation)) { @@ -282,6 +282,11 @@ router.put('/:id', (req, res) => { if (ota_enabled !== undefined) { updates.push('ota_enabled = ?'); values.push(ota_enabled ? 1 : 0); } + if (ota_beta !== undefined) { + // Per-display pre-release opt-in (#234 follow-up). Stops a test build being reverted by the + // next OTA check, which is what a prerelease version sorting below its own release causes. + updates.push('ota_beta = ?'); values.push(ota_beta ? 1 : 0); + } // #12 scheduled reboot: device-local "HH:MM" (null/'' clears -> off). Reset the // once-per-day guard on any change so a newly-set time can still fire later today. if (reboot_schedule !== undefined) { diff --git a/server/server.js b/server/server.js index bfa2590..e7c4c7d 100644 --- a/server/server.js +++ b/server/server.js @@ -704,6 +704,7 @@ app.get('/api/update/check', (req, res) => { const currentVersion = req.query.version; const deviceId = req.query.device_id || null; // #144: optional; beta4+ clients send it for per-device keying const latestVersion = VERSION; + let betaChannel = false; // per-display pre-release opt-in, set from the device row below // #155/#161: self-update kill switch, enforced SERVER-SIDE so it covers EVERY client // version (not just ones with the client-side stand-down). If OTA is off globally @@ -715,8 +716,12 @@ app.get('/api/update/check', (req, res) => { let otaDeviceOff = false; if (deviceId) { try { - const row = require('./db/database').db.prepare('SELECT ota_enabled FROM devices WHERE id = ?').get(deviceId); + const row = require('./db/database').db.prepare('SELECT ota_enabled, ota_beta FROM devices WHERE id = ?').get(deviceId); otaDeviceOff = !!row && row.ota_enabled === 0; + // #234 follow-up: per-display pre-release opt-in, read from the same row rather than a + // second query. Without it, handing someone a test build is a trap — a prerelease sorts + // BELOW its own release, so the next check "upgrades" the display straight back off it. + betaChannel = !!row && row.ota_beta === 1; } catch (_) { /* device unknown / pre-migration — treat as enabled */ } } if (otaGloballyOff || otaDeviceOff) { @@ -731,7 +736,7 @@ app.get('/api/update/check', (req, res) => { // #144: circuit-breaker + phantom-version guard. Keys per device_id when present, else // per reported version (NOT IP — SNAT). Rate-trips a looping client in seconds. - const verdict = otaBreaker.decide(currentVersion, latestVersion, deviceId); + const verdict = otaBreaker.decide(currentVersion, latestVersion, deviceId, Date.now(), betaChannel); // #146 Item C: EARLY-RETURN before any filesystem work when we won't serve // (rate-backoff, up-to-date, phantom, client-newer, …). A looping client that gets diff --git a/server/test/ota-beta-channel.test.js b/server/test/ota-beta-channel.test.js new file mode 100644 index 0000000..572d8c0 --- /dev/null +++ b/server/test/ota-beta-channel.test.js @@ -0,0 +1,79 @@ +'use strict'; + +// Handing someone a test build was a trap. +// +// A prerelease sorts BELOW its own release: 1.9.25-fix234d < 1.9.25. So a display sideloaded with a +// test build asked the server "anything newer?", was correctly told yes — the released 1.9.25 — and +// updated itself straight back off the build we had asked someone to test. Same versionCode, so +// Android installed it without complaint. Silent, within minutes. +// +// It happened on #234: the reporter installed the fix, tested for an evening, and reported that +// nothing had changed. They were right — their tablet was running the old code again by then. +// +// The opt-in is per display and deliberately narrow: it holds a prerelease of the CURRENT core +// only. An older-core prerelease is genuinely stale and must still be offered an update, and once a +// newer release ships the display must rejoin it — otherwise "beta" quietly becomes "abandoned on a +// branch nobody maintains". + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const breaker = require('../lib/ota-breaker'); + +// decide(client, latest, deviceId, now, betaChannel) +const ask = (client, latest, beta) => breaker.decide(client, latest, null, Date.now(), beta); + +test('THE BUG: without the opt-in, a test build is offered its own release and reverts', () => { + const v = ask('1.9.25-fix234d', '1.9.25', false); + assert.equal(v.update_available, true, 'this is the revert that cost a reporter an evening'); + assert.equal(v.reason, 'offer'); +}); + +test('THE FIX: an opted-in display keeps a prerelease of the current release', () => { + const v = ask('1.9.25-fix234d', '1.9.25', true); + assert.equal(v.update_available, false); + assert.equal(v.reason, 'beta-channel'); +}); + +test('opting in does NOT strand a display once a newer release ships', () => { + // The whole risk of a beta flag is that it becomes permanent. 1.9.26 is a real newer core, so an + // opted-in display on any 1.9.25 build must take it. + const v = ask('1.9.25-fix234d', '1.9.26', true); + assert.equal(v.update_available, true, 'a beta display must rejoin the next real release'); +}); + +test('the superseded-prerelease guard is untouched for displays that did NOT opt in', () => { + // #144's phantom protection: a device reporting an ancient beta is not chased with offers. + const v = ask('1.9.1-beta4', '1.9.25', false); + assert.equal(v.update_available, false); + assert.equal(v.reason, 'superseded-prerelease'); +}); + +test('but an opted-in display on an old prerelease IS offered the current release', () => { + // This is the escape hatch, and it is the difference between "beta" and "abandoned". Without + // it the superseded guard pins a tester on an old test build permanently — they would have to + // notice and sideload their way out, which is exactly the trap the opt-in exists to remove. + const v = ask('1.9.1-beta4', '1.9.25', true); + assert.equal(v.update_available, true, 'opting in must never mean never updating again'); +}); + +test('the opt-in changes nothing for a display on a plain release', () => { + assert.equal(ask('1.9.25', '1.9.25', true).reason, 'up-to-date'); + assert.equal(ask('1.9.24', '1.9.25', true).update_available, true, 'a real upgrade is unaffected'); + assert.equal(ask('1.9.24', '1.9.25', false).update_available, true); +}); + +test('a display ahead of the server is never downgraded, opted in or not', () => { + assert.equal(ask('1.9.26', '1.9.25', true).reason, 'client-newer'); + assert.equal(ask('1.9.26', '1.9.25', false).reason, 'client-newer'); +}); + +test('a -patchN build is a release, not a prerelease, so beta does not pin it', () => { + // isReleased() treats patchN as released; it must keep being offered real updates. + const v = ask('1.9.2-patch3', '1.9.25', true); + assert.equal(v.update_available, true, 'a patch release must not be mistaken for a beta build'); +}); + +test('the flag defaults to off, so nothing changes for a fleet that never sets it', () => { + const withDefault = breaker.decide('1.9.25-fix234d', '1.9.25', null, Date.now()); + assert.equal(withDefault.update_available, true, 'default must match pre-existing behaviour'); +});