From 3aef97f08a1b1399385cf032eeb78306f48952c3 Mon Sep 17 00:00:00 2001
From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com>
Date: Sat, 20 Jun 2026 09:32:30 -0400
Subject: [PATCH] feat(edit): clean up superseded thumbnail on edit (orphan
 removal)

Previously editing a video deleted the old hosted video + its .torrent when the
URL changed, but intentionally left the old thumbnail orphaned in the bucket.
Now that the Edit modal can replace a thumbnail (and we have a clear "thumbnail
changed" signal), also delete the superseded thumbnail: build an orphan list with
the old video ({url}) and/or old thumbnail ({thumbnail}) and pass both to
deleteVideoStorage. Each guarded on old & new being non-empty and different, so we
never delete an object the new note still references. Best-effort; never blocks
the edit.

Tests (collectVideoStorageKeys): a {url} orphan yields the video key + sibling
.torrent; a {thumbnail} orphan yields the thumbnail key; both collect together;
objects under a different publicBaseUrl are never collected (bucket-switch safety).

Also documents the pre-existing uploadModal-reset.test.mjs flake in KNOWN_ISSUES
(reproduced on 1b11cb1b; not from this work).

Build + full lint green; full unit suite green except the documented pre-existing
flake.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 KNOWN_ISSUES.md                         |  5 +++
 js/ui/editModalController.js            | 27 ++++++++----
 tests/edit-orphan-cleanup-keys.test.mjs | 56 +++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 7 deletions(-)
 create mode 100644 tests/edit-orphan-cleanup-keys.test.mjs

diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md
index 87ccdbc5..6d39ee99 100644
--- a/KNOWN_ISSUES.md
+++ b/KNOWN_ISSUES.md
@@ -33,3 +33,8 @@
 - **Status:** Active (pre-existing — reproduced on pre-refactor commit `1b11cb1b`)
 - **Description:** "plays directly from HTML link (CDN mode)" fails on `--project=e2e-firefox` with `#modalVideo` `src` empty instead of the test's archive.org URL. The Chromium variant and the WebTorrent-fallback variant (`:63`) both pass. Firefox-headless-specific (external URL reachability/codec), not an app regression.
 - **Impact:** One Firefox-only e2e failure; CDN playback works on Chromium and in manual testing.
+
+### `uploadModal-reset.test.mjs` ("UploadModal Reset Logic") hangs/cancels
+- **Status:** Active (pre-existing — reproduced on pre-refactor commit `1b11cb1b`)
+- **Description:** The jsdom-based UploadModal reset suite intermittently fails with `cancelledByParent` / "Promise resolution is still pending but the event loop has already resolved" (~12s hang). It passes or fails depending on full-suite test ordering/timing (an async-hang flake, likely a torrent-metadata/upload promise not settling under the mocked services). Not a regression from the MediaUploader extraction — it fails identically on `1b11cb1b`.
+- **Impact:** One unit suite can report `not ok 1`; the runner still exits 0. Other tests unaffected.
diff --git a/js/ui/editModalController.js b/js/ui/editModalController.js
index 51e2a0f1..815891b2 100644
--- a/js/ui/editModalController.js
+++ b/js/ui/editModalController.js
@@ -92,18 +92,31 @@ export default class EditModalController {
         pubkey,
       });
 
-      // Best-effort: if the hosted video URL was genuinely replaced, the old
-      // R2/S3 object (and its .torrent) is now orphaned — remove it. We require
-      // both URLs to be non-empty and different so we never delete an object
-      // that's still referenced (e.g. via a magnet web-seed when the URL was
-      // merely cleared). Thumbnails are intentionally left alone here to avoid
-      // deleting one the new note still points at. Never blocks the edit.
+      // Best-effort: when the hosted video URL and/or thumbnail were genuinely
+      // replaced, the old R2/S3 objects are now orphaned — remove them. For each,
+      // we require both old and new values to be non-empty and different so we
+      // never delete an object the new note still references (e.g. the URL was
+      // merely cleared, or the thumbnail was left unchanged). The video entry
+      // also takes its sibling .torrent. Never blocks the edit.
       try {
+        const orphans = [];
+
         const oldUrl = detail.video?.url || "";
         const newUrl = updatedData?.url || "";
         if (updatedData?.urlEdited && oldUrl && newUrl && oldUrl !== newUrl) {
+          // collectVideoStorageKeys derives the video key + its .torrent.
+          orphans.push({ url: oldUrl });
+        }
+
+        const oldThumb = detail.video?.thumbnail || "";
+        const newThumb = updatedData?.thumbnail || "";
+        if (oldThumb && newThumb && oldThumb !== newThumb) {
+          orphans.push({ thumbnail: oldThumb });
+        }
+
+        if (orphans.length) {
           const cleanup = await r2Service.deleteVideoStorage({
-            videos: [{ url: oldUrl }],
+            videos: orphans,
             pubkey,
           });
           if (cleanup?.deleted?.length) {
diff --git a/tests/edit-orphan-cleanup-keys.test.mjs b/tests/edit-orphan-cleanup-keys.test.mjs
new file mode 100644
index 00000000..f0da4163
--- /dev/null
+++ b/tests/edit-orphan-cleanup-keys.test.mjs
@@ -0,0 +1,56 @@
+// On edit, superseded R2/S3 objects are removed by passing orphan descriptors to
+// deleteVideoStorage, which derives the bucket keys via collectVideoStorageKeys.
+// This guards the descriptors the edit-cleanup builds map to the right keys:
+//   - a replaced video entry -> the video key AND its sibling .torrent
+//   - a replaced thumbnail entry -> the thumbnail key
+// and that only objects under the configured publicBaseUrl are ever collected.
+
+import "./test-helpers/setup-localstorage.mjs";
+import assert from "node:assert/strict";
+import test from "node:test";
+import { collectVideoStorageKeys } from "../js/utils/storagePointer.js";
+
+const BASE = "https://pub-abc.r2.dev";
+
+test("a replaced video entry yields the video key + its .torrent", () => {
+  const keys = collectVideoStorageKeys({
+    videos: [{ url: `${BASE}/u/npub1x/deadbeef/my-clip.mp4` }],
+    publicBaseUrl: BASE,
+  });
+  assert.deepEqual(
+    keys.sort(),
+    ["u/npub1x/deadbeef/my-clip.mp4", "u/npub1x/deadbeef/my-clip.torrent"].sort(),
+  );
+});
+
+test("a replaced thumbnail entry yields the thumbnail key", () => {
+  const keys = collectVideoStorageKeys({
+    videos: [{ thumbnail: `${BASE}/npub1x/thumbnails/123-old.png` }],
+    publicBaseUrl: BASE,
+  });
+  assert.deepEqual(keys, ["npub1x/thumbnails/123-old.png"]);
+});
+
+test("both video and thumbnail orphans collect together", () => {
+  const keys = collectVideoStorageKeys({
+    videos: [
+      { url: `${BASE}/u/npub1x/hash/clip.mp4` },
+      { thumbnail: `${BASE}/npub1x/thumbnails/9-old.jpg` },
+    ],
+    publicBaseUrl: BASE,
+  });
+  assert.ok(keys.includes("u/npub1x/hash/clip.mp4"));
+  assert.ok(keys.includes("u/npub1x/hash/clip.torrent"));
+  assert.ok(keys.includes("npub1x/thumbnails/9-old.jpg"));
+});
+
+test("objects under a DIFFERENT base are never collected (bucket switch safety)", () => {
+  const keys = collectVideoStorageKeys({
+    videos: [
+      { url: "https://other-bucket.r2.dev/u/npub1x/hash/clip.mp4" },
+      { thumbnail: "https://other-bucket.r2.dev/npub1x/thumbnails/9.jpg" },
+    ],
+    publicBaseUrl: BASE,
+  });
+  assert.deepEqual(keys, []);
+});