From 1c256808ff72b8a345b30e336394cb5f799a0212 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Wed, 24 Jun 2026 18:48:49 -0400 Subject: [PATCH] =?UTF-8?q?fix(more-menu):=20resolve=20modal=20=E2=8B=AF?= =?UTF-8?q?=20video=20so=20Event=20Details=20+=20Popularity=20work=20(#31)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The video modal's ⋯ trigger declares its context via `data-more-dropdown="modal"`, but attachMoreMenuHandlers read only `data-context`, so it defaulted to "card", skipped the modal video resolution, and opened the popover with a null video. The panel is built from that video, so every menu item got an empty data-event-id and both Event Details and Popularity reported "No video selected." - Read context from `data-more-dropdown` as well as `data-context`. - Modal branch falls back to the modal's `activeVideo` when getCurrentVideo() is null (deep-link opens don't set the playback service's currentVideo). - Defense-in-depth: handleEventDetailsAction resolves the video from app.videosMap.get(eventId); view-stats resolves via a new getVideoByEventId callback instead of a risky `|| currentVideo` that could show the wrong stats. - Regression tests in tests/more-menu-controller.test.mjs. Co-Authored-By: Claude Opus 4.8 --- js/ui/applicationBootstrap.js | 13 +++- js/ui/moreMenuController.js | 27 ++++++- tests/more-menu-controller.test.mjs | 107 ++++++++++++++++++++++++++++ todo/TODO_2026-06-20_pre-launch.md | 21 +++++- 4 files changed, 160 insertions(+), 8 deletions(-) diff --git a/js/ui/applicationBootstrap.js b/js/ui/applicationBootstrap.js index 40a1a6c9..3e69eebf 100644 --- a/js/ui/applicationBootstrap.js +++ b/js/ui/applicationBootstrap.js @@ -675,6 +675,10 @@ export default class ApplicationBootstrap { designSystem: app.designSystemContext, callbacks: { getCurrentVideo: () => app.currentVideo, + getVideoByEventId: (eventId) => + eventId && app.videosMap instanceof Map + ? app.videosMap.get(eventId) || null + : null, getCurrentUserNpub: () => app.getCurrentUserNpub(), getCurrentUserPubkey: () => app.pubkey, canCurrentUserManageBlacklist: () => @@ -700,9 +704,14 @@ export default class ApplicationBootstrap { app.showError?.("Event details are unavailable."); return Promise.resolve(); } - const video = payload?.video || null; + // Card popovers sometimes don't carry the video object on context, so + // resolve it from the menu item's eventId as a fallback (the object the + // grid already holds). This is what made the button silently no-op. + let video = payload?.video || null; + if (!video && payload?.eventId && app.videosMap instanceof Map) { + video = app.videosMap.get(payload.eventId) || null; + } if (!video) { - // Don't fail silently — this is the bug the ⋯ "Event Details" button hit. app.showError?.("No video selected for event details."); return Promise.resolve(); } diff --git a/js/ui/moreMenuController.js b/js/ui/moreMenuController.js index 087ba6fa..da6bd78d 100644 --- a/js/ui/moreMenuController.js +++ b/js/ui/moreMenuController.js @@ -45,6 +45,7 @@ export default class MoreMenuController { this.callbacks = { getCurrentVideo: callbacks.getCurrentVideo || (() => null), + getVideoByEventId: callbacks.getVideoByEventId || (() => null), getCurrentUserNpub: callbacks.getCurrentUserNpub || (() => null), getCurrentUserPubkey: callbacks.getCurrentUserPubkey || (() => null), canCurrentUserManageBlacklist: @@ -1018,7 +1019,13 @@ export default class MoreMenuController { event.preventDefault(); event.stopPropagation(); - const context = button.getAttribute("data-context") || "card"; + // The modal ⋯ trigger declares its context via `data-more-dropdown` + // ("modal"), not `data-context` — reading only the latter made it + // default to "card" and skip the modal video resolution below. + const context = + button.getAttribute("data-context") || + button.getAttribute("data-more-dropdown") || + "card"; let video = null; let pointerInfo = null; @@ -1027,6 +1034,11 @@ export default class MoreMenuController { if (context === "modal") { video = this.callbacks.getCurrentVideo(); + // currentVideo can be null when the modal opened without playback + // starting (e.g. a deep link); fall back to the modal's activeVideo. + if (!video && this.videoModal?.activeVideo) { + video = this.videoModal.activeVideo; + } if (Array.isArray(video?.pointer) && video.pointer.length >= 2) { pointerInfo = { pointer: video.pointer }; } @@ -1163,8 +1175,17 @@ export default class MoreMenuController { break; } case "view-stats": { - const targetVideo = - video || (context === "modal" ? currentVideo : null) || currentVideo; + // Prefer the card's own video; fall back to resolving it from the menu + // item's eventId (card popovers don't always carry the object), and only + // use the modal's current video for the modal context. + let targetVideo = video || null; + if (!targetVideo && dataset?.eventId) { + targetVideo = + this.callbacks.getVideoByEventId(dataset.eventId) || null; + } + if (!targetVideo && context === "modal") { + targetVideo = currentVideo || null; + } if (!targetVideo) { this.callbacks.showError("No video selected."); break; diff --git a/tests/more-menu-controller.test.mjs b/tests/more-menu-controller.test.mjs index a73c3e11..703cf188 100644 --- a/tests/more-menu-controller.test.mjs +++ b/tests/more-menu-controller.test.mjs @@ -252,3 +252,110 @@ test('unmute-author removes creators from viewer mute list', async () => { ], ]); }); + +// Regression: card popovers don't always carry the video object on the menu +// context. The view-stats and event-details actions must resolve the video from +// the menu item's eventId so the buttons work for cards (not just the modal). +test('event-details forwards the eventId so the app can resolve a card video', async () => { + let received = null; + const controller = new MoreMenuController({ + callbacks: { + getCurrentVideo: () => null, + handleEventDetailsAction: async (payload) => { + received = payload; + }, + showError: (message) => { + throw new Error(`Unexpected error: ${message}`); + }, + }, + }); + + // video === null mimics entry.context.video being absent on a card popover. + await controller.handleMoreMenuAction( + 'event-details', + { eventId: 'card-event-id', context: 'card' }, + null, + ); + + assert.equal(received?.eventId, 'card-event-id'); + assert.equal(received?.video, null); +}); + +test('view-stats looks the card video up by eventId instead of the modal video', async () => { + const resolvedVideo = { id: 'card-event-id', pubkey: 'authorhex' }; + const lookups = []; + const controller = new MoreMenuController({ + callbacks: { + // The old code used `|| currentVideo`, which would show this video's stats + // for any card whose popover lacked the object — the bug we're guarding. + getCurrentVideo: () => ({ id: 'wrong-modal-video' }), + getVideoByEventId: (eventId) => { + lookups.push(eventId); + return eventId === 'card-event-id' ? resolvedVideo : null; + }, + showError: (message) => { + throw new Error(`Unexpected error: ${message}`); + }, + }, + }); + + // Drive the real action with no video object (card popover) in a DOM-less + // harness — openPopularityModal no-ops without a document, so we assert the + // controller resolved the right video by eventId before opening. + await controller.handleMoreMenuAction( + 'view-stats', + { eventId: 'card-event-id', context: 'card' }, + null, + ); + + assert.deepEqual(lookups, ['card-event-id']); +}); + +// Regression: the modal ⋯ trigger declares its context via `data-more-dropdown` +// ("modal"), not `data-context`. Reading only `data-context` made it default to +// "card" and skip modal video resolution, so the popover opened with a null +// video and every action reported "No video selected." attachMoreMenuHandlers +// must treat the trigger as modal context and resolve the modal's active video. +test('modal ⋯ trigger resolves its video via data-more-dropdown + activeVideo fallback', async () => { + const activeVideo = { id: 'modal-video-id', pubkey: 'authorhex' }; + + const controller = new MoreMenuController({ + callbacks: { + // currentVideo is null (e.g. deep-link open without playback) — the bug. + getCurrentVideo: () => null, + canCurrentUserManageBlacklist: () => false, + }, + }); + // The modal is the authoritative on-screen video source. + controller.setVideoModal({ activeVideo }); + // Avoid the real DOM/popover engine; capture what toggleMoreMenu receives. + let toggled = null; + controller.toggleMoreMenu = (detail) => { + toggled = detail; + }; + controller.ensureGlobalMoreMenuHandlers = () => {}; + controller.getHTMLElementConstructor = () => null; + controller.document = {}; + + // Minimal trigger button mimicking the modal ⋯ markup. + let clickHandler = null; + const button = { + dataset: {}, + getAttribute: (name) => + name === 'data-more-dropdown' ? 'modal' : null, + addEventListener: (type, handler) => { + if (type === 'click') clickHandler = handler; + }, + }; + const container = { + querySelectorAll: () => [button], + }; + + controller.attachMoreMenuHandlers(container); + assert.equal(typeof clickHandler, 'function', 'click handler bound'); + + await clickHandler({ preventDefault() {}, stopPropagation() {} }); + + assert.equal(toggled?.context, 'modal'); + assert.equal(toggled?.video, activeVideo); +}); diff --git a/todo/TODO_2026-06-20_pre-launch.md b/todo/TODO_2026-06-20_pre-launch.md index 9a0c0d81..266833b8 100644 --- a/todo/TODO_2026-06-20_pre-launch.md +++ b/todo/TODO_2026-06-20_pre-launch.md @@ -497,9 +497,24 @@ SILENT one: `handleEventDetailsAction` only opened the modal `if (… && payload - [x] `EventDetailsModal.open` now calls `openStaticModal` FIRST, then renders in a try/catch, so a render hiccup can't leave the click looking dead. - [x] The ⋯ popover is dismissed before opening the modal. -- [ ] **VERIFY live** (I couldn't repro by inspection): click ⋯ → Event Details on - a card AND in the video modal. If it still does nothing, the new visible - error / console warn will say exactly why (no video vs open() throw). +- [x] **Actual root cause (2nd live test):** BOTH Event Details and Popularity + reported "No video selected" from the **video modal's** ⋯ button. The modal + trigger declares its context as `data-more-dropdown="modal"`, but + `attachMoreMenuHandlers` read only `data-context` → defaulted to `"card"` → + skipped the modal-only video resolution → opened the popover with a **null + video**, so the panel's `data-event-id` was empty and every action failed. + The panel is built from `entry.context.video`, so this nulled the whole menu. + Fixes (all in `moreMenuController.js`): + 1. Read context from `data-more-dropdown` as well as `data-context`. + 2. Modal branch falls back to `this.videoModal.activeVideo` when + `getCurrentVideo()` is null (deep-link opens don't set playback's + currentVideo). + 3. Defense-in-depth: `handleEventDetailsAction` resolves the video from + `app.videosMap.get(eventId)`, and `view-stats` uses a new + `getVideoByEventId` callback (dropping a risky `|| currentVideo`). + Regression tests added in `tests/more-menu-controller.test.mjs`. +- [ ] **VERIFY live**: click ⋯ → Event Details AND ⋯ → Popularity in the video + modal AND on a grid/channel card; both should open the correct video. ### 28. Beacon torrent app stuck — spinner never resolves (BUG) - [ ] The torrent beacon app (`scripts/build:beacon` / `torrent/` integration) shows