Close three holes in the card release path
A second review pass over the pool. All three are the same shape: state or a listener outliving the video it belonged to. attachProxyFallback replaces whatever fallback an image currently has, so a call arriving late -- a race hitting its patience timeout after the card was recycled -- took away the live listener and left a dead one, and the new video's thumbnail would then fail with nothing behind it. This one was self-inflicted: the detach came in last round to stop the listeners accumulating, and introduced the clobber. It is token-guarded now, like every other path that can arrive late. The favourite pop is cleared by animationend, which never fires on a card release() has already detached -- detached elements run no animations. So the class rode into the pool and replayed on the next video the card showed. Favourite something and flick-scroll to see it. And withOrigin compared a token that dataset reports as undefined for an unstamped card, which matched every unstamped card instead of none -- failing open in the guard whose whole purpose is noticing that the grid has recycled the card out from under the player. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBDkEXP4htyXTCZUwMLphd
This commit is contained in:
@@ -75,7 +75,11 @@ App.player = App.player || {};
|
|||||||
};
|
};
|
||||||
|
|
||||||
const withOrigin = function(el, token, fn) {
|
const withOrigin = function(el, token, fn) {
|
||||||
if (el && el.dataset.playerToken === token) fn(el);
|
// `token` must be truthy in its own right: dataset yields undefined for
|
||||||
|
// a missing attribute, so without this an unstamped token would match
|
||||||
|
// every card that has no stamp -- including one the grid has recycled,
|
||||||
|
// which is precisely the case this guard exists to catch.
|
||||||
|
if (el && token && el.dataset.playerToken === token) fn(el);
|
||||||
};
|
};
|
||||||
|
|
||||||
const addCleanup = (fn) => cp.cleanups.push(fn);
|
const addCleanup = (fn) => cp.cleanups.push(fn);
|
||||||
|
|||||||
@@ -207,6 +207,12 @@ App.videos = App.videos || {};
|
|||||||
// shouldn't be left broken just because its host is fine in general.
|
// shouldn't be left broken just because its host is fine in general.
|
||||||
const attachProxyFallback = function(img, proxyUrl, token) {
|
const attachProxyFallback = function(img, proxyUrl, token) {
|
||||||
if (!proxyUrl) return;
|
if (!proxyUrl) return;
|
||||||
|
// Checked here too, not just in showThumbnail: this *replaces* whatever
|
||||||
|
// fallback the image currently has, so a call arriving for a generation
|
||||||
|
// the element has moved past would take away the live one and leave a
|
||||||
|
// dead one -- the recycled card's thumbnail would then have no fallback
|
||||||
|
// at all if it failed.
|
||||||
|
if (token !== undefined && img.dataset.thumbToken !== token) return;
|
||||||
// Held on the element so detachThumbnail can take it off again. On the
|
// Held on the element so detachThumbnail can take it off again. On the
|
||||||
// happy path it never fires and `once` never collects it, so a pooled
|
// happy path it never fires and `once` never collects it, so a pooled
|
||||||
// image would otherwise accumulate one closure per mount it has served.
|
// image would otherwise accumulate one closure per mount it has served.
|
||||||
@@ -898,6 +904,13 @@ App.videos = App.videos || {};
|
|||||||
// recycled card must stop answering to it.
|
// recycled card must stop answering to it.
|
||||||
delete card.dataset.playerToken;
|
delete card.dataset.playerToken;
|
||||||
|
|
||||||
|
// Added by favorites.toggle and normally taken off by animationend --
|
||||||
|
// which never fires here, because release() detaches the card first and
|
||||||
|
// a detached element runs no animations. Left on, the next video this
|
||||||
|
// card shows replays a "just favourited" pop nobody asked for.
|
||||||
|
const favorite = cardRefs(card).favorite;
|
||||||
|
if (favorite) favorite.classList.remove('just-favorited');
|
||||||
|
|
||||||
const menu = card.querySelector('.video-menu');
|
const menu = card.querySelector('.video-menu');
|
||||||
if (menu) menu.classList.remove('open');
|
if (menu) menu.classList.remove('open');
|
||||||
const menuBtn = card.querySelector('.video-menu-btn');
|
const menuBtn = card.querySelector('.video-menu-btn');
|
||||||
|
|||||||
@@ -61,6 +61,7 @@ INSPECT = """() => {
|
|||||||
heart_shown: fav ? fav.classList.contains('is-favorite') : null,
|
heart_shown: fav ? fav.classList.contains('is-favorite') : null,
|
||||||
heart_expected: v ? App.favorites.has(v) : null,
|
heart_expected: v ? App.favorites.has(v) : null,
|
||||||
stale_loading: card.classList.contains('is-loading'),
|
stale_loading: card.classList.contains('is-loading'),
|
||||||
|
stale_pop: fav ? fav.classList.contains('just-favorited') : false,
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
}"""
|
}"""
|
||||||
@@ -158,6 +159,13 @@ def check_cards(c, cards, phase):
|
|||||||
c.ok(f"{phase}: no card left in the loading state", not stale,
|
c.ok(f"{phase}: no card left in the loading state", not stale,
|
||||||
f"{len(stale)} stuck" if stale else "")
|
f"{len(stale)} stuck" if stale else "")
|
||||||
|
|
||||||
|
# The favourite pop is animated away by animationend, which never fires on a
|
||||||
|
# card released mid-animation -- so it can ride into the pool and replay on
|
||||||
|
# whatever video the card is bound to next.
|
||||||
|
popping = [x for x in cards if x["stale_pop"]]
|
||||||
|
c.ok(f"{phase}: no card replaying the favourite animation", not popping,
|
||||||
|
f"{len(popping)} popping, e.g. id={popping[0]['id']}" if popping else "")
|
||||||
|
|
||||||
|
|
||||||
def main():
|
def main():
|
||||||
c = Checks()
|
c = Checks()
|
||||||
|
|||||||
Reference in New Issue
Block a user