mirror of
https://github.com/PR0M3TH3AN/bitvid.git
synced 2026-10-27 07:52:24 +00:00
fix(more-menu): resolve modal ⋯ video so Event Details + Popularity work (#31)
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 <[email protected]>
This commit is contained in:
1 parent
63dcb5f6ed
commit
1c256808ff
4 files changed
+160
-8
No files matched your search
@@ -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();
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user