PROD-9829: Fix comment video thumbnails and low-res thumbnail regeneration - #5028
Open
rezwan-buddyboss wants to merge 1 commit into
Open
PROD-9829: Fix comment video thumbnails and low-res thumbnail regeneration#5028rezwan-buddyboss wants to merge 1 commit into
rezwan-buddyboss wants to merge 1 commit into
Conversation
…ation Issue 1 — comment video attachments lost their thumbnail after posting: - bp_video_add_generate_thumb_background_process() excluded videos with privacy 'comment' from the FFmpeg-based background thumbnail job, alongside 'forums'/'message'. Removed 'comment' from that exclusion so the fallback path works for comment attachments too. - Root cause was a JS race condition: bp.Nouveau.getVideoThumb() captures a video frame to canvas fully asynchronously, and the comment composer's upload 'success' handler read the captured <img> immediately instead of waiting for it, unlike the top-level post composer which already polls for the dz-has-thumbnail/dz-has-no-thumbnail signal. Ported that same wait into the comment composer, pushing the video data synchronously (so comment-content validation still sees it immediately) and backfilling js_preview once the capture finishes. - Hand-patched buddypress-activity.min.js to match, since this checkout has no node_modules installed and can't run the grunt build (known peer-dependency conflict) — flagging for a proper rebuild on review. Issue 2 — low-resolution video thumbnails were regenerated on every page render when symlinks are enabled, since there was no record of thumbnail sizes a video is physically too small to ever produce. Added a '_bb_video_thumb_unavailable_sizes' postmeta cache so a size is attempted once and skipped on all subsequent requests if still missing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PROD link: https://buddyboss.atlassian.net/browse/PROD-9829
This ticket bundles two separate reported bugs; both are fixed in this PR.
Issue 1 — comment video attachments lose their thumbnail after posting
Uploading a video to a top-level activity post generates and keeps a thumbnail correctly. Uploading the same video as an activity comment attachment shows the thumbnail preview during upload, but once the comment is submitted, the thumbnail disappears and the default placeholder shows instead — reloading the page doesn't fix it. The video itself plays fine; only the thumbnail is lost.
Root cause
Two separate problems compounded:
bp_video_add_generate_thumb_background_process()explicitly excludedprivacy === 'comment'videos (alongsideforums/message) from the FFmpeg-based background thumbnail job, so comment attachments never got a server-side regeneration fallback the way post attachments do.bp.Nouveau.getVideoThumb()captures a video frame to a<canvas>/<img>fully asynchronously, with no completion callback. The top-level post composer already accounts for this — it polls for adz-has-thumbnail/dz-has-no-thumbnailclass signal before reading the captured image. The comment composer's uploadsuccesshandler never got that same wait: it read$( file.previewElement ).find( '.dz-video-thumbnail img' ).attr( 'src' )immediately, before the async capture had necessarily finished, so it frequently pushed an empty/undefined preview into the comment's video data.Fix
'comment'from the exclusion array inbp_video_add_generate_thumb_background_process()(src/bp-video/bp-video-functions.php:1034), so comment attachments get the same background-job fallback post attachments already have.src/bp-templates/bp-nouveau/js/buddypress-activity.js): the video data object is still pushed synchronously (so comment-content validation sees it immediately and doesn't block submission), but ifjs_previewcame back empty, asetIntervalpolls for thedz-has-thumbnail/dz-has-no-thumbnailclass and backfillsjs_previewon that same object (by reference) once the async capture actually finishes.buddypress-activity.min.jswas hand-patched to match, rather than regenerated viagrunt uglify— this checkout has nonode_modulesinstalled and a known peer-dependency conflict blocksnpm installhere. This should be regenerated via a proper grunt build during review rather than trusted long-term as a hand-patch.Issue 2 — low-resolution videos re-trigger thumbnail regeneration on every page load
When a source video's dimensions are smaller than a registered thumbnail size (e.g.
bb-video-poster-popup-image), that size can never be generated. Reported behavior: instead of failing once and remembering that, the system re-attempts generating that size on every single page render, and reportedly multiple times per request — degrading feed load time in proportion to how many small videos are on the page. Not reproduced locally, but the code-level defect the ticket points at is real and independently confirmed by reading the function.Root cause
bb_video_get_attachment_symlink()callsbp_video_regenerate_attachment_thumbnails( $attachment_id )— a fullwp_generate_attachment_metadata()regeneration — every time a requested size's file doesn't already exist (! $file), with no memory of whether that size has already been tried and failed. For a size the source video is simply too small to ever produce, every single page load that touches this attachment re-runs the full regeneration for that size, forever.Fix
Added a
_bb_video_thumb_unavailable_sizespostmeta array, following the starting point the ticket itself suggested. Before callingbp_video_regenerate_attachment_thumbnails(), the requested$sizeis checked against that array and skipped if already recorded. After a regeneration attempt, if the file still doesn't exist, the size is added to the array so no future request re-attempts it. A size only ever costs one real regeneration attempt per attachment; every request after that is a cheap postmeta lookup.