diff --git a/docs/performance-efficiency-plan.md b/docs/performance-efficiency-plan.md index 37e6835b..4084ef2e 100644 --- a/docs/performance-efficiency-plan.md +++ b/docs/performance-efficiency-plan.md @@ -1,6 +1,6 @@ # BitVid Performance & Efficiency Plan -Status: **Stage 2 in progress — startup hardening validated locally; recommendation indexes now activate on demand** +Status: **Stage 2 complete locally — recommendation indexes are demand-driven and coalesced; Stage 3 source-health cancellation audited next** ## Goal @@ -173,13 +173,14 @@ old diagnostic behavior. ## Stage 2 — Lazy recommendation indexes -Implemented first slice: `ExploreDataService` is no longer created during +Implemented: `ExploreDataService` is no longer created during ordinary bootstrap. Home/Profile sessions therefore create no recommendation worker or recommendation intervals. Opening For You or Explore creates and activates the service; events received while inactive mark it dirty and are -included in the activation refresh. Focused lifecycle, feed-coordinator, and -build checks pass; browser validation of both recommendation views remains the -next gate before further Stage 2 coalescing work. +included in the activation refresh. Concurrent history/IDF signals share one +running rebuild and retain one forced follow-up pass. Focused lifecycle, +feed-coordinator, production-build, and browser navigation checks pass; manual +For You/Explore validation confirmed normal recommendations and responsive Home. ### Change @@ -224,6 +225,24 @@ eager initialization if feed freshness regresses. ## Stage 3 — Bound source-health work to visible cards +### Audit findings (2026-07-26) + +- `createCardObserver` retains an `IntersectionObserver` per grid container but + has no disposal API. `VideoListView.render()` replaces card DOM without + disconnecting the old observer, so detached cards can stay observed. +- `gridHealth` has a globally bounded, priority-ordered torrent queue, but its + queued jobs have no owner/cancellation handle. A discarded card can therefore + still start a real tracker/WebRTC probe. +- URL health uses an independent observer and video-probe wait queue with the + same missing ownership boundary. Its fetch timeout aborts only on timeout, + not when the card/grid disappears. + +Implementation order: add disposable observer registrations and call them from +grid replacement/unmount; associate queued jobs with card owners and prune jobs +when their last owner disappears; finally pass an abort signal into active URL +and torrent probes. Cache/in-flight deduplication stays shared so one visible +card never cancels work still needed by another visible card. + ### Change Keep the existing viewport/preload observer and current two-at-a-time torrent diff --git a/js/dom/cardObserver.js b/js/dom/cardObserver.js index 4272ded0..7131167f 100644 --- a/js/dom/cardObserver.js +++ b/js/dom/cardObserver.js @@ -26,6 +26,7 @@ export function createCardObserver(options = {}) { prepareEntries, onCardVisible, onCardRegister, + onCardUnregister, createState, } = options; @@ -37,7 +38,9 @@ export function createCardObserver(options = {}) { return state; } - const observedCards = new WeakSet(); + // A Set is intentional here: disposal must enumerate every card so the + // browser observer cannot retain detached feed nodes after a rerender. + const observedCards = new Set(); const customState = normalizeState( typeof createState === "function" ? createState(container) : {} ); @@ -201,9 +204,34 @@ export function createCardObserver(options = {}) { return state ? state.customState : null; } + function disconnect(container) { + const state = containerState.get(container); + if (!state) { + return; + } + + for (const card of state.observedCards) { + if (typeof onCardUnregister === "function") { + try { + onCardUnregister({ + card, + state: state.customState, + context: { container: state.container, observer: state.observer }, + }); + } catch (err) { + userLogger.warn("[cardObserver] onCardUnregister failed", err); + } + } + } + state.observedCards.clear(); + state.observer.disconnect(); + containerState.delete(container); + } + return { observe, refresh, getState, + disconnect, }; } diff --git a/js/feedEngine/sorters.js b/js/feedEngine/sorters.js index 042c2dc6..ef44d172 100644 --- a/js/feedEngine/sorters.js +++ b/js/feedEngine/sorters.js @@ -10,7 +10,7 @@ import { markAsNormalized } from "./utils.js"; // re-runs the feed on the metric cache's debounced change signal). // - Trending (#27): runtime.getViewCount (viewCounter cache) // - Most Zapped (#47): runtime.getZapTotal (zapTotals cache) -function createCountRankedSorter(resolveMetric) { +function createCountRankedSorter(resolveMetric, { spreadByAuthor = true } = {}) { return function countRankedSorter(items = [], context = {}) { if (!Array.isArray(items)) { return []; @@ -54,9 +54,12 @@ function createCountRankedSorter(resolveMetric) { const live = items.filter((entry) => !isMuted(entry)); const muted = items.filter((entry) => isMuted(entry)); - // Rank by the metric, then spread authors so the tab isn't walls of one - // creator. The top-ranked item is still chosen first, so it stays topmost. - return [...spreadAuthors(live.sort(compare)), ...muted.sort(compare)]; + const rankedLive = live.sort(compare); + // Trending benefits from a little creator diversity. Most Zapped is a + // literal leaderboard, so it must retain strict total ordering instead of + // moving a larger zap total below a smaller one to avoid repeat authors. + const orderedLive = spreadByAuthor ? spreadAuthors(rankedLive) : rankedLive; + return [...orderedLive, ...muted.sort(compare)]; }; } @@ -66,7 +69,10 @@ export function createTrendingSorter() { // "Most Zapped" (#47): same ranking mechanics over sats totals. export function createMostZappedSorter() { - return createCountRankedSorter((context) => context?.runtime?.getZapTotal); + return createCountRankedSorter( + (context) => context?.runtime?.getZapTotal, + { spreadByAuthor: false }, + ); } // Round-robin interleave by author: one video per creator per pass, each pass in diff --git a/js/gridHealth.js b/js/gridHealth.js index 334099d1..3e6e39fe 100644 --- a/js/gridHealth.js +++ b/js/gridHealth.js @@ -573,10 +573,11 @@ const gridCardObserver = createCardObserver({ export function attachHealthBadges(container, options = {}) { if (!(container instanceof HTMLElement)) { - return; + return () => {}; } if (options && typeof options.onUpdate === "function") { badgeUpdateListeners.add(options.onUpdate); } gridCardObserver.observe(container); + return () => gridCardObserver.disconnect(container); } diff --git a/js/services/exploreDataService.js b/js/services/exploreDataService.js index f503af42..f640b9ed 100644 --- a/js/services/exploreDataService.js +++ b/js/services/exploreDataService.js @@ -148,6 +148,10 @@ export default class ExploreDataService { this.active = false; this.historyDirty = true; this.idfDirty = true; + this.historyRefreshInFlight = null; + this.idfRefreshInFlight = null; + this.historyRefreshFollowUp = null; + this.idfRefreshFollowUp = null; } initialize({ active = false } = {}) { @@ -274,7 +278,33 @@ export default class ExploreDataService { }, this.refreshDebounceMs); } - async refreshWatchHistoryTagCounts({ force = false, reason } = {}) { + async refreshWatchHistoryTagCounts(options = {}) { + if (this.historyRefreshInFlight) { + // Keep only one fresh pass after the current worker task. Rebuilding for + // every relay/event/timer signal is exactly the burst this lifecycle is + // intended to prevent. + this.historyRefreshFollowUp = { + force: true, + reason: options.reason || "coalesced", + }; + return this.historyRefreshInFlight; + } + + const run = this._refreshWatchHistoryTagCounts(options); + this.historyRefreshInFlight = Promise.resolve(run).finally(() => { + this.historyRefreshInFlight = null; + const followUp = this.historyRefreshFollowUp; + this.historyRefreshFollowUp = null; + if (followUp && this.active) { + void this.refreshWatchHistoryTagCounts(followUp); + } else if (followUp) { + this.historyDirty = true; + } + }); + return this.historyRefreshInFlight; + } + + async _refreshWatchHistoryTagCounts({ force = false, reason } = {}) { const now = Date.now(); if (!force && this.watchHistoryTagCountsUpdatedAt) { const elapsed = now - this.watchHistoryTagCountsUpdatedAt; @@ -308,7 +338,30 @@ export default class ExploreDataService { } } - async refreshTagIdf({ force = false, videos, reason } = {}) { + async refreshTagIdf(options = {}) { + if (this.idfRefreshInFlight) { + this.idfRefreshFollowUp = { + force: true, + reason: options.reason || "coalesced", + }; + return this.idfRefreshInFlight; + } + + const run = this._refreshTagIdf(options); + this.idfRefreshInFlight = Promise.resolve(run).finally(() => { + this.idfRefreshInFlight = null; + const followUp = this.idfRefreshFollowUp; + this.idfRefreshFollowUp = null; + if (followUp && this.active) { + void this.refreshTagIdf(followUp); + } else if (followUp) { + this.idfDirty = true; + } + }); + return this.idfRefreshInFlight; + } + + async _refreshTagIdf({ force = false, videos, reason } = {}) { const now = Date.now(); if (!force && this.tagIdfUpdatedAt) { const elapsed = now - this.tagIdfUpdatedAt; @@ -361,6 +414,8 @@ export default class ExploreDataService { clearTimeout(this.tagIdfRefreshHandle); this.tagIdfRefreshHandle = null; } + this.historyRefreshFollowUp = null; + this.idfRefreshFollowUp = null; this.unsubscribeHandlers.forEach((unsubscribe) => { if (typeof unsubscribe === "function") { diff --git a/js/ui/applicationBootstrap.js b/js/ui/applicationBootstrap.js index a76cca45..5746fb2d 100644 --- a/js/ui/applicationBootstrap.js +++ b/js/ui/applicationBootstrap.js @@ -166,11 +166,10 @@ export default class ApplicationBootstrap { app.streamHealthSnapshots = new Map(); app.boundStreamHealthBadgeHandler = (detail) => app.handleStreamHealthBadgeUpdate(detail); - app.attachHealthBadgesWithCache = (container) => { + app.attachHealthBadgesWithCache = (container) => attachHealthBadges(container, { onUpdate: app.boundStreamHealthBadgeHandler, }); - }; app.defaultModerationSettings = getDefaultModerationSettings(); app.moderationSettings = { ...app.defaultModerationSettings }; app.relayManager = relayManager; diff --git a/js/ui/views/VideoListView.js b/js/ui/views/VideoListView.js index 427ee9e6..67b3b948 100644 --- a/js/ui/views/VideoListView.js +++ b/js/ui/views/VideoListView.js @@ -32,6 +32,8 @@ export class VideoListView { (typeof window !== "undefined" ? window : null); this.container = null; this.mediaLoader = mediaLoader; + this.healthBadgeDetach = null; + this.urlHealthBadgeDetach = null; this.badgeHelpers = { attachHealthBadges: badgeHelpers.attachHealthBadges || (() => {}), @@ -236,6 +238,7 @@ export class VideoListView { } if (this.container) { + this.teardownHealthBadgeObservers(); this.container.removeEventListener("click", this._boundClickHandler); } @@ -262,6 +265,7 @@ export class VideoListView { const markup = this.renderers.getLoadingMarkup(message); if (typeof markup === "string") { + this.teardownHealthBadgeObservers(); this.container.innerHTML = markup; } } @@ -280,6 +284,16 @@ export class VideoListView { } } + teardownHealthBadgeObservers() { + for (const detach of [this.healthBadgeDetach, this.urlHealthBadgeDetach]) { + if (typeof detach === "function") { + detach(); + } + } + this.healthBadgeDetach = null; + this.urlHealthBadgeDetach = null; + } + setPlaybackHandler(handler) { this.handlers.playback = typeof handler === "function" ? handler : null; } @@ -374,6 +388,7 @@ export class VideoListView { return displayVideos; } this.lastRenderedVideoSignature = EMPTY_VIDEO_LIST_SIGNATURE; + this.teardownHealthBadgeObservers(); this.container.innerHTML = `

No public videos available yet. Be the first to upload one! @@ -413,7 +428,10 @@ export class VideoListView { })); const signature = JSON.stringify(signaturePayload); - if (signature === this.lastRenderedVideoSignature) { + const hasRenderedCards = Boolean( + this.container.querySelector('[data-component="video-card"]'), + ); + if (signature === this.lastRenderedVideoSignature && hasRenderedCards) { return displayVideos; } this.lastRenderedVideoSignature = signature; @@ -788,11 +806,12 @@ export class VideoListView { }); } + this.teardownHealthBadgeObservers(); this.container.innerHTML = ""; this.container.appendChild(fragment); - this.badgeHelpers.attachHealthBadges(this.container); - this.badgeHelpers.attachUrlHealthBadges(this.container, ({ badgeEl, url, eventId }) => { + this.healthBadgeDetach = this.badgeHelpers.attachHealthBadges(this.container); + this.urlHealthBadgeDetach = this.badgeHelpers.attachUrlHealthBadges(this.container, ({ badgeEl, url, eventId }) => { this.utils.handleUrlHealthBadge({ video: this.state.videosMap.get(eventId) || { id: eventId }, url, diff --git a/js/urlHealthObserver.js b/js/urlHealthObserver.js index 7fbc3ebd..f6036e33 100644 --- a/js/urlHealthObserver.js +++ b/js/urlHealthObserver.js @@ -75,12 +75,12 @@ function triggerUrlCheck(card, state) { export function attachUrlHealthBadges(container, onCheck) { if (!(container instanceof HTMLElement)) { - return; + return () => {}; } const state = urlCardObserver.observe(container); if (state && typeof onCheck === "function") { state.onCheck = onCheck; } + return () => urlCardObserver.disconnect(container); } - diff --git a/tests/dom/card-observer-disposal.test.mjs b/tests/dom/card-observer-disposal.test.mjs new file mode 100644 index 00000000..7a442983 --- /dev/null +++ b/tests/dom/card-observer-disposal.test.mjs @@ -0,0 +1,38 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { JSDOM } from "jsdom"; + +import { createCardObserver } from "../../js/dom/cardObserver.js"; + +test("card observer disconnect unregisters every card and releases its observer", () => { + const dom = new JSDOM("

"); + const originalHTMLElement = globalThis.HTMLElement; + const originalIntersectionObserver = globalThis.IntersectionObserver; + globalThis.HTMLElement = dom.window.HTMLElement; + let disconnects = 0; + globalThis.IntersectionObserver = class { + observe() {} + takeRecords() { return []; } + disconnect() { disconnects += 1; } + }; + + const unregistered = []; + try { + const observer = createCardObserver({ + onCardUnregister: ({ card }) => unregistered.push(card.dataset.videoId), + }); + const grid = dom.window.document.getElementById("grid"); + observer.observe(grid); + observer.disconnect(grid); + + assert.deepEqual(unregistered.sort(), ["a", "b"]); + assert.equal(disconnects, 1); + assert.equal(observer.getState(grid), null); + } finally { + dom.window.close(); + if (originalHTMLElement === undefined) delete globalThis.HTMLElement; + else globalThis.HTMLElement = originalHTMLElement; + if (originalIntersectionObserver === undefined) delete globalThis.IntersectionObserver; + else globalThis.IntersectionObserver = originalIntersectionObserver; + } +}); diff --git a/tests/services/explore-data-lifecycle.test.mjs b/tests/services/explore-data-lifecycle.test.mjs index d213dbb3..f00085b4 100644 --- a/tests/services/explore-data-lifecycle.test.mjs +++ b/tests/services/explore-data-lifecycle.test.mjs @@ -33,3 +33,33 @@ test("ExploreDataService stays idle until a recommendation feed activates it", a assert.equal(idfRefreshes, 1, "inactive feeds do not rebuild recommendation indexes"); service.destroy(); }); + +test("ExploreDataService coalesces a refresh burst into one running pass and one follow-up", async () => { + const service = new ExploreDataService({ + historyRefreshIntervalMs: 0, + idfRefreshIntervalMs: 0, + }); + service.active = true; + let runs = 0; + let releaseFirst; + service._refreshTagIdf = async () => { + runs += 1; + if (runs === 1) { + await new Promise((resolve) => { + releaseFirst = resolve; + }); + } + return new Map(); + }; + + const first = service.refreshTagIdf({ force: true, reason: "initial" }); + const duplicate = service.refreshTagIdf({ reason: "relay-event" }); + service.refreshTagIdf({ reason: "timer" }); + assert.equal(runs, 1, "only one rebuild starts during the burst"); + + releaseFirst(); + await Promise.all([first, duplicate]); + await new Promise((resolve) => setTimeout(resolve, 0)); + assert.equal(runs, 2, "the burst produces exactly one follow-up rebuild"); + service.destroy(); +}); diff --git a/tests/ui/video-list-view-health-disposal.test.mjs b/tests/ui/video-list-view-health-disposal.test.mjs new file mode 100644 index 00000000..f95351d2 --- /dev/null +++ b/tests/ui/video-list-view-health-disposal.test.mjs @@ -0,0 +1,49 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { JSDOM } from "jsdom"; + +import { VideoListView } from "../../js/ui/views/VideoListView.js"; + +test("VideoListView disposes health observers before replacing its card grid", () => { + const dom = new JSDOM("
"); + const grid = dom.window.document.getElementById("grid"); + let healthDetaches = 0; + let urlDetaches = 0; + const view = new VideoListView({ + document: dom.window.document, + badgeHelpers: { + attachHealthBadges: () => () => { healthDetaches += 1; }, + attachUrlHealthBadges: () => () => { urlDetaches += 1; }, + }, + }); + view.mount(grid); + const videos = [{ id: "one", title: "One", pubkey: "a", created_at: 1 }]; + + view.render(videos); + view.render([{ ...videos[0], title: "Updated", created_at: 2 }]); + view.unmount(); + + assert.equal(healthDetaches, 2, "rerender and unmount each detach health observers"); + assert.equal(urlDetaches, 2, "rerender and unmount each detach URL observers"); + dom.window.close(); +}); + +test("VideoListView restores cards when a navigation remount clears unchanged feed markup", () => { + const dom = new JSDOM("
"); + const grid = dom.window.document.getElementById("grid"); + const view = new VideoListView({ document: dom.window.document }); + const videos = [{ id: "for-you-one", title: "For You", pubkey: "a", created_at: 1 }]; + + view.mount(grid); + view.render(videos); + assert.equal(grid.querySelectorAll('[data-component="video-card"]').length, 1); + + // Loading a view replaces #videoList. The data can still be identical when + // returning to For You, so a signature-only cache must not skip the repaint. + grid.innerHTML = '
'; + view.render(videos); + + assert.equal(grid.querySelectorAll('[data-component="video-card"]').length, 1); + assert.equal(grid.querySelector('[data-for-you-empty-state]'), null); + dom.window.close(); +}); diff --git a/tests/zap-totals.test.mjs b/tests/zap-totals.test.mjs index bc279061..f8238264 100644 --- a/tests/zap-totals.test.mjs +++ b/tests/zap-totals.test.mjs @@ -373,3 +373,25 @@ test("most-zapped sorter: sats desc, recency tie-break, muted sinks", () => { "sats desc, newer wins ties, trusted-muted last regardless of sats", ); }); + +test("most-zapped sorter keeps strict zap order when one creator has several top videos", () => { + const item = (id, author, sats, createdAt) => ({ + video: { id, pubkey: author, created_at: createdAt }, + metadata: {}, + sats, + }); + const items = [ + item("creator-a-top", "creator-a", 900, 100), + item("creator-a-second", "creator-a", 800, 200), + item("creator-b", "creator-b", 700, 300), + ]; + const sorted = createMostZappedSorter()(items, { + runtime: { getZapTotal: (video) => items.find((entry) => entry.video === video)?.sats || 0 }, + }); + + assert.deepEqual( + sorted.map((entry) => entry.video.id), + ["creator-a-top", "creator-a-second", "creator-b"], + "a diversity rule must not move a lower-zapped video above a higher-zapped one", + ); +});