feat: update ready keeps showing even after updating

Task #61 completed by ClaudeQueue

ClaudeQueue

frontend/app.js

@@ -2992,17 +2992,34 @@ function showUpdateBanner() {
     { label: 'Later', onClick: closeModal },
     { label: 'Refresh UI', primary: true, onClick: async () => {
       closeModal();
-      await hardReloadUI();
+      await applyUpdate();
     }},
   ]);
 }
 
+// Tracks the current SW registration so applyUpdate() can reach the
+// waiting worker without re-querying getRegistration().
+let _swReg = null;
+
+// Applies a pending SW update in place (see sw-update.js for the full
+// rationale — this used to call hardReloadUI(), which caused the "Update
+// ready" banner to reappear right after being applied).
+async function applyUpdate() {
+  const reg = _swReg || (('serviceWorker' in navigator) ? await navigator.serviceWorker.getRegistration() : null);
+  await window.SwUpdate.applyUpdate({
+    reg,
+    container: navigator.serviceWorker,
+    reload: () => window.location.reload(),
+  });
+}
+
 async function registerServiceWorker() {
   if (!WEB || !('serviceWorker' in navigator)) return;
   try {
     // updateViaCache:'none' prevents the browser from serving a stale
     // cached copy of sw.js — the server always returns it fresh (no-store).
     const reg = await navigator.serviceWorker.register('/sw.js', { updateViaCache: 'none' });
+    _swReg = reg;
 
     // If a new SW is already waiting (e.g. user refreshed after an update),
     // show the dialog right away.

frontend/index.html

@@ -307,6 +307,7 @@
     <script src="fingerprint.js"></script>
     <script src="opfs.js"></script>
     <script src="async-guard.js"></script>
+    <script src="sw-update.js"></script>
     <script src="app.js"></script>
   </body>
 </html>

frontend/sw-update.js

@@ -0,0 +1,70 @@
+/* ============================================================================
+ * sw-update — applies a waiting service-worker update in place.
+ *
+ * Framework-free and dependency-free on purpose (same pattern as
+ * async-guard.js):
+ *   • Loads as a plain <script> under CSP `script-src 'self'` (browser
+ *     global `window.SwUpdate`).
+ *   • `require`-able by `node --test` (CommonJS `module.exports`).
+ *
+ * Bug this fixes (Task #61): "Update ready" kept reappearing right after the
+ * user clicked "Reload now" / "Refresh UI". The old flow called
+ * hardReloadUI(), which unregisters the service worker and wipes every cache
+ * before navigating — forcing a brand-new install on the next load. That
+ * fresh install briefly has `reg.waiting` truthy again (a normal but
+ * transient SW lifecycle state), which registerServiceWorker() misread as a
+ * genuinely new update and re-showed the banner immediately.
+ *
+ * Fix: activate the *already-installed* waiting worker in place —
+ * postMessage SKIP_WAITING to it, wait for it to actually take control
+ * (`controllerchange`), and only then reload. The reloaded page is served by
+ * the new SW from its very first request, and no fresh install/registration
+ * cycle happens, so the banner has nothing to spuriously re-trigger on.
+ * ========================================================================== */
+(function (root) {
+  'use strict';
+
+  /**
+   * Applies a pending SW update: messages the waiting worker to skipWaiting(),
+   * waits for controllerchange, then reloads exactly once.
+   *
+   * @param {object} opts
+   * @param {ServiceWorkerRegistration|null} opts.reg  the current registration
+   * @param {ServiceWorkerContainer} opts.container     navigator.serviceWorker
+   * @param {() => void} opts.reload                    called at most once
+   * @param {(fn: () => void, ms: number) => any} [opts.setTimeout] injectable for tests
+   * @returns {Promise<void>}
+   */
+  async function applyUpdate({ reg, container, reload, setTimeout: setTimeoutFn }) {
+    const waiting = reg && reg.waiting;
+    const scheduleTimeout = setTimeoutFn || (typeof setTimeout !== 'undefined' ? setTimeout : null);
+
+    if (!waiting) {
+      // Nothing to activate (e.g. banner was shown from a broadcast message
+      // rather than an actual waiting worker) — just reload.
+      reload();
+      return;
+    }
+
+    let reloaded = false;
+    const reloadOnce = () => {
+      if (reloaded) return;
+      reloaded = true;
+      reload();
+    };
+
+    container.addEventListener('controllerchange', reloadOnce, { once: true });
+    // Safety net in case controllerchange never fires (e.g. no controller yet).
+    if (scheduleTimeout) scheduleTimeout(reloadOnce, 3000);
+
+    waiting.postMessage({ type: 'SKIP_WAITING' });
+  }
+
+  const SwUpdate = { applyUpdate };
+
+  if (typeof module !== 'undefined' && module.exports) {
+    module.exports = SwUpdate;
+  } else {
+    root.SwUpdate = SwUpdate;
+  }
+})(typeof globalThis !== 'undefined' ? globalThis : this);

frontend/sw-update.test.js

@@ -0,0 +1,130 @@
+'use strict';
+
+/**
+ * Unit tests for sw-update.js (Task #61).
+ *
+ * Bug: "Update ready / Reload now" reappeared immediately after the user
+ * clicked "Reload now". Root cause — the old flow unregistered the service
+ * worker and wiped every cache before navigating, forcing a brand-new
+ * install on the next load; that fresh install's registration briefly has
+ * `reg.waiting` truthy again (a normal but transient SW lifecycle state),
+ * which was misread as a new pending update and re-showed the banner.
+ *
+ * These tests drive applyUpdate() directly against mocked registration /
+ * container / reload objects — no real service worker or browser needed.
+ */
+
+const { test } = require('node:test');
+const assert = require('node:assert');
+const { applyUpdate } = require('./sw-update');
+
+// A minimal fake ServiceWorkerContainer supporting addEventListener/once.
+function fakeContainer() {
+  const listeners = {};
+  return {
+    addEventListener(type, fn, opts) {
+      (listeners[type] = listeners[type] || []).push({ fn, once: !!(opts && opts.once) });
+    },
+    fireControllerChange() {
+      const fns = (listeners.controllerchange || []).slice();
+      for (const { fn, once } of fns) {
+        fn();
+        if (once) {
+          listeners.controllerchange = listeners.controllerchange.filter((l) => l.fn !== fn);
+        }
+      }
+    },
+  };
+}
+
+test('messages the waiting worker to skipWaiting and reloads only after controllerchange', async () => {
+  const messages = [];
+  const waiting = { postMessage: (m) => messages.push(m) };
+  const container = fakeContainer();
+  let reloadCount = 0;
+
+  const done = applyUpdate({
+    reg: { waiting },
+    container,
+    reload: () => { reloadCount++; },
+    setTimeout: () => {}, // no-op — we drive controllerchange manually
+  });
+
+  // SKIP_WAITING should be sent immediately, before any reload.
+  await Promise.resolve();
+  assert.deepStrictEqual(messages, [{ type: 'SKIP_WAITING' }]);
+  assert.strictEqual(reloadCount, 0, 'must not reload before the new SW has taken control');
+
+  container.fireControllerChange();
+  await done;
+  assert.strictEqual(reloadCount, 1, 'reloads exactly once after controllerchange');
+});
+
+test('reloads only once even if controllerchange fires more than once (no reload loop)', async () => {
+  const waiting = { postMessage: () => {} };
+  const container = fakeContainer();
+  let reloadCount = 0;
+
+  const done = applyUpdate({
+    reg: { waiting },
+    container,
+    reload: () => { reloadCount++; },
+    setTimeout: () => {},
+  });
+
+  container.fireControllerChange();
+  container.fireControllerChange(); // simulate a spurious second event
+  await done;
+
+  assert.strictEqual(reloadCount, 1, 'reload must be idempotent — no loop');
+});
+
+test('falls back to a plain reload when there is no waiting worker', async () => {
+  const container = fakeContainer();
+  let reloadCount = 0;
+
+  await applyUpdate({
+    reg: { waiting: null },
+    container,
+    reload: () => { reloadCount++; },
+    setTimeout: () => { throw new Error('timeout should not be scheduled without a waiting worker'); },
+  });
+
+  assert.strictEqual(reloadCount, 1);
+});
+
+test('falls back to a plain reload when there is no registration at all', async () => {
+  const container = fakeContainer();
+  let reloadCount = 0;
+
+  await applyUpdate({
+    reg: null,
+    container,
+    reload: () => { reloadCount++; },
+  });
+
+  assert.strictEqual(reloadCount, 1);
+});
+
+test('the timeout safety net reloads once if controllerchange never fires', async () => {
+  const waiting = { postMessage: () => {} };
+  const container = fakeContainer();
+  let reloadCount = 0;
+  let scheduledFn = null;
+
+  await applyUpdate({
+    reg: { waiting },
+    container,
+    reload: () => { reloadCount++; },
+    setTimeout: (fn) => { scheduledFn = fn; }, // capture instead of real timer
+  });
+
+  assert.strictEqual(reloadCount, 0, 'not reloaded yet — timeout not fired');
+  scheduledFn(); // simulate the timeout elapsing
+  assert.strictEqual(reloadCount, 1);
+
+  // A late controllerchange after the timeout already reloaded must not
+  // trigger a second reload.
+  container.fireControllerChange();
+  assert.strictEqual(reloadCount, 1, 'no double reload once the timeout fallback has fired');
+});

frontend/sw.js

@@ -9,21 +9,24 @@
  *   Everything else            → network, fallback to cache
  *
  * Auto-update flow:
- *   1. New SW installs alongside the old one.
- *   2. activate: if an older *versioned shell cache* is found (i.e. this
+ *   1. New SW installs alongside the old one and waits (skipWaiting() is NOT
+ *      called automatically — see the install handler below).
+ *   2. Client (app.js registerServiceWorker()) notices the waiting worker
+ *      and shows the "Update ready" banner.
+ *   3. User clicks "Reload now" → client (applyUpdate() in sw-update.js)
+ *      posts SKIP_WAITING to the waiting worker and waits for it to take
+ *      control (controllerchange) before reloading — it does NOT unregister
+ *      or wipe caches, so the reload doesn't force a brand-new install.
+ *   4. activate: if an older *versioned shell cache* is found (i.e. this
  *      activation is genuinely replacing a previous deploy, not just the
  *      first-ever install of a freshly (re)registered worker), broadcast
- *      SW_UPDATE_AVAILABLE to all clients.
- *   3. Client shows "Update ready" banner.
- *   4. User clicks "Refresh UI" → client (hardReloadUI in app.js) wipes every
- *      cache, unregisters the SW, and hard-navigates to fetch everything
- *      fresh from the network.
+ *      SW_UPDATE_AVAILABLE to all clients as a secondary/fallback signal.
  * ========================================================================== */
 
 // BUILD_TAG is injected by the server at request time (GET /sw.js).
 // It changes on every deploy/restart so the cache is busted automatically
 // without any manual version bump.
-const VERSION = typeof __BUILD_TAG__ !== 'undefined' ? __BUILD_TAG__ : 'v1.0.3';
+const VERSION = typeof __BUILD_TAG__ !== 'undefined' ? __BUILD_TAG__ : 'v1.0.4';
 const CACHE   = 'ytplayer-' + VERSION;
 
 // Prefix shared by every versioned app-shell cache (ytplayer-<VERSION>).
@@ -41,6 +44,7 @@ const SHELL = [
   '/index.html',
   '/styles.css',
   '/async-guard.js',
+  '/sw-update.js',
   '/fingerprint.js',
   '/opfs.js',
   '/app.js',

frontend/sw.test.js

@@ -76,7 +76,7 @@ function loadSw(initialCacheKeys) {
 }
 
 test('activate does NOT broadcast on a fresh install (only the current-version cache exists)', async () => {
-  const sw = loadSw(['ytplayer-v1.0.3']); // default VERSION fallback in sw.js is 'v1.0.3'
+  const sw = loadSw(['ytplayer-v1.0.4']); // default VERSION fallback in sw.js is 'v1.0.4'
   await sw.triggerActivate();
 
   assert.deepStrictEqual(sw.clientMessages, [], 'no update banner should be triggered on first-ever install');
@@ -84,7 +84,7 @@ test('activate does NOT broadcast on a fresh install (only the current-version c
 });
 
 test('activate DOES broadcast when an older versioned shell cache is present (genuine update)', async () => {
-  const sw = loadSw(['ytplayer-v1.0.3', 'ytplayer-v1.0.2']);
+  const sw = loadSw(['ytplayer-v1.0.4', 'ytplayer-v1.0.2']);
   await sw.triggerActivate();
 
   assert.strictEqual(sw.clientMessages.length, 1, 'a genuine version bump should notify clients once');
@@ -93,25 +93,25 @@ test('activate DOES broadcast when an older versioned shell cache is present (ge
 });
 
 test('activate never deletes the utility caches (thumbs/fonts), broadcast or not', async () => {
-  const sw = loadSw(['ytplayer-v1.0.3', 'ytplayer-thumbs', 'ytplayer-fonts']);
+  const sw = loadSw(['ytplayer-v1.0.4', 'ytplayer-thumbs', 'ytplayer-fonts']);
   await sw.triggerActivate();
 
   assert.deepStrictEqual(sw.clientMessages, [], 'utility caches alone are not a version bump');
   assert.deepStrictEqual(sw.deleted, [], 'utility caches must survive activate');
   assert.deepStrictEqual(
     sw.cacheStoreRemaining().sort(),
-    ['ytplayer-fonts', 'ytplayer-thumbs', 'ytplayer-v1.0.3'].sort()
+    ['ytplayer-fonts', 'ytplayer-thumbs', 'ytplayer-v1.0.4'].sort()
   );
 });
 
 test('activate reports an update and still preserves utility caches together', async () => {
-  const sw = loadSw(['ytplayer-v1.0.3', 'ytplayer-v1.0.2', 'ytplayer-thumbs', 'ytplayer-fonts']);
+  const sw = loadSw(['ytplayer-v1.0.4', 'ytplayer-v1.0.2', 'ytplayer-thumbs', 'ytplayer-fonts']);
   await sw.triggerActivate();
 
   assert.strictEqual(sw.clientMessages.length, 1);
   assert.deepStrictEqual(sw.deleted, ['ytplayer-v1.0.2']);
   assert.deepStrictEqual(
     sw.cacheStoreRemaining().sort(),
-    ['ytplayer-fonts', 'ytplayer-thumbs', 'ytplayer-v1.0.3'].sort()
+    ['ytplayer-fonts', 'ytplayer-thumbs', 'ytplayer-v1.0.4'].sort()
   );
 });