VideoPress: Refresh media library processing status via Heartbeat#47358
Open
VideoPress: Refresh media library processing status via Heartbeat#47358
Conversation
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the WP Admin Media Library (grid view) experience for VideoPress uploads by surfacing processing state to the media Backbone models and using WP Heartbeat to refresh items automatically when transcoding completes.
Changes:
- Expose
videopress_statusattachment meta inwp_prepare_attachment_for_jsso the Media Library JS model knows which items are still processing. - Add a Media Library script that sends processing attachment IDs via Heartbeat and, when complete, refetches attachment data to update the grid item.
- Add a Heartbeat server handler to return current
videopress_statusvalues for requested attachment IDs.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| projects/packages/videopress/src/js/media-library-poll.js | Adds Heartbeat send/tick hooks to poll processing IDs and refresh attachments when complete. |
| projects/packages/videopress/src/class-attachment-handler.php | Includes videopress_status in attachment data prepared for the media JS model. |
| projects/packages/videopress/src/class-admin-ui.php | Enqueues the polling script on upload.php and implements the Heartbeat response payload. |
| projects/packages/videopress/changelog/fix-media-library-processing-status-refresh | Adds changelog entry describing the fix. |
Comments suppressed due to low confidence (4)
projects/packages/videopress/src/js/media-library-poll.js:62
- In the
heartbeat-tickhandler, if the server reportsstatus === 'complete'for an attachment, this will fire aget-attachmentAJAX request on every subsequent heartbeat until the first request returns (because the model’svideopress_statusisn’t updated until.done()). Consider marking the attachment as “refreshing” (e.g., a transient attribute) before sending, and clearing it in.always(), to avoid duplicate concurrent refresh requests for the same ID.
$.each( data.videopress_processing_status, function ( id, status ) {
const attachment = library.get( id );
if ( attachment && status === 'complete' ) {
wp.ajax
.send( 'get-attachment', {
data: { id: id },
} )
.done( function ( attrs ) {
attachment.set( attrs );
} );
projects/packages/videopress/src/class-admin-ui.php:458
heartbeat_received()trusts client-providedvideopress_processing_idsand loops through them unbounded. To avoid information leaks and potential authenticated DoS via many IDs, please (1) cap the number of IDs processed, (2) ensure the current user can at leastedit_post/read_postfor each attachment, and (3) consider priming the post/meta cache (e.g.,update_meta_cache) before readingvideopress_statusin a loop to avoid N+1 queries.
public static function heartbeat_received( $response, $data ) {
if ( empty( $data['videopress_processing_ids'] ) || ! is_array( $data['videopress_processing_ids'] ) ) {
return $response;
}
$statuses = array();
foreach ( $data['videopress_processing_ids'] as $id ) {
$id = (int) $id;
$status = get_post_meta( $id, 'videopress_status', true );
projects/packages/videopress/src/class-admin-ui.php:468
- This adds new server behavior (
heartbeat_received) that reads attachment meta based on client input. Since the package has PHPUnit coverage, please add unit tests covering: empty/invalid payloads, capability checks (once added), and that a known attachment ID returns the expectedvideopress_processing_statusmapping.
public static function heartbeat_received( $response, $data ) {
if ( empty( $data['videopress_processing_ids'] ) || ! is_array( $data['videopress_processing_ids'] ) ) {
return $response;
}
$statuses = array();
foreach ( $data['videopress_processing_ids'] as $id ) {
$id = (int) $id;
$status = get_post_meta( $id, 'videopress_status', true );
if ( $status ) {
$statuses[ $id ] = $status;
}
}
if ( ! empty( $statuses ) ) {
$response['videopress_processing_status'] = $statuses;
}
return $response;
projects/packages/videopress/src/class-attachment-handler.php:205
prepare_attachment_for_js()now exposesvideopress_statusto the media JS model. Since this is new API surface used by the new polling code, please add a PHPUnit test asserting that whenvideopress_statusmeta is set on a video attachment, the prepared JS array includesvideopress_status(and omits it when the meta is absent).
}
$status = get_post_meta( $post['id'], 'videopress_status', true );
if ( $status ) {
$post['videopress_status'] = $status;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage SummaryCoverage changed in 3 files.
1 file is newly checked for coverage.
|
obenland
force-pushed
the
add/vidp-175-videopress-media-library-does-not-refresh-the-processing
branch
from
08ed1c4 to
ca8932a
Compare
After uploading a video through the Media Library, the processing thumbnail persisted even after transcoding completed on the server. This adds a WP Heartbeat integration that polls for status changes and refreshes the grid item when transcoding finishes.
obenland
force-pushed
the
add/vidp-175-videopress-media-library-does-not-refresh-the-processing
branch
from
ca8932a to
2ad775f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
projects/packages/videopress/src/js/test/media-library-poll.js:167
- Missing test coverage for AJAX error scenarios. The tests only cover successful AJAX responses in the 'fetches attachment data when status becomes complete' test. There should be tests that verify behavior when the get-attachment AJAX request fails, such as network errors or server errors. This is important because the implementation doesn't currently handle these failures.
it( 'fetches attachment data when status becomes complete', () => {
const attachment = createAttachment( {
id: 10,
type: 'video',
subtype: 'videopress',
videopress_status: 'processing',
} );
setupLibrary( [ attachment ] );
const deferred = {
done: jest.fn( cb => {
cb( { id: 10, icon: 'new-icon.png' } );
return deferred;
} ),
};
global.wp.ajax.send.mockReturnValue( deferred );
loadScript();
handlers[ 'heartbeat-tick' ](
{},
{
videopress_processing_status: { 10: 'complete' },
}
);
expect( global.wp.ajax.send ).toHaveBeenCalledWith( 'get-attachment', {
data: { id: '10' },
} );
expect( attachment.set ).toHaveBeenCalledWith( { id: 10, icon: 'new-icon.png' } );
} );
projects/packages/videopress/src/js/media-library-poll.js:64
- Potential race condition when multiple videos complete simultaneously. If multiple videos finish processing at the same time, multiple get-attachment AJAX calls will be initiated in parallel (lines 56-62). Since there's no queuing or rate limiting, this could result in a burst of AJAX requests. While not necessarily a critical issue, consider implementing request batching or queuing to reduce server load if many videos complete simultaneously.
$.each( data.videopress_processing_status, function ( id, status ) {
const attachment = library.get( id );
if ( attachment && status === 'complete' ) {
wp.ajax
.send( 'get-attachment', {
data: { id: id },
} )
.done( function ( attrs ) {
attachment.set( attrs );
} );
}
} );
projects/packages/videopress/src/class-attachment-handler.php:349
- The heartbeat_received function does not validate user permissions before returning attachment status information. While WordPress's built-in get-attachment AJAX handler likely has its own permission checks, it's a security best practice to validate at each layer. Consider adding a capability check such as current_user_can('upload_files') to ensure only authorized users can query video processing status. This provides defense-in-depth and makes the security posture explicit in this code.
public static function heartbeat_received( $response, $data ) {
if ( empty( $data['videopress_processing_ids'] ) || ! is_array( $data['videopress_processing_ids'] ) ) {
return $response;
}
$statuses = array();
foreach ( $data['videopress_processing_ids'] as $id ) {
$id = (int) $id;
$status = get_post_meta( $id, 'videopress_status', true );
if ( $status ) {
$statuses[ $id ] = $status;
}
}
if ( ! empty( $statuses ) ) {
$response['videopress_processing_status'] = $statuses;
}
return $response;
}
projects/packages/videopress/src/js/media-library-poll.js:58
- The JavaScript passes 'id' as a string in line 58, but this inconsistency could cause issues. In line 164 of the test file, the test expects 'id' to be passed as a string ('10'), which matches the implementation. However, it would be more consistent to pass the id as a number since Backbone's library.get() already coerces types. Consider standardizing on one type for clarity.
data: { id: id },
projects/packages/videopress/src/js/media-library-poll.js:62
- Missing error handling for the AJAX request. The wp.ajax.send call in line 56-62 only handles the success case with .done(), but doesn't handle failures with .fail(). If the get-attachment AJAX request fails (e.g., network error, server error, unauthorized access), the attachment model won't be updated and there's no retry mechanism or user feedback. Consider adding a .fail() handler to log errors or implement retry logic.
wp.ajax
.send( 'get-attachment', {
data: { id: id },
} )
.done( function ( attrs ) {
attachment.set( attrs );
} );
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ook re-registration The wp_prepare_attachment_for_js filter passes an array, not a WP_Post. Update the @param type to match and fix Phan errors. Restore per-test re-registration of REST API hooks in Data_Test that was inadvertently removed in f3375fa. WorDBless resets all hooks between tests, so the rest_attachment_query filter needs to be re-registered in set_up().
Add test coverage for enqueue_media_library_poll. Update the deprecated videopress_prepare_attachment_for_js wrapper in editor-media-view.php to match the corrected @param array type.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
[Feature] VideoPress
Set heartbeat to fast (10s) while videos are processing and revert to standard (60s) when all are complete. Lower the minimalInterval on the upload page via heartbeat_settings filter to allow sub-15s ticks.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
projects/packages/videopress/src/class-attachment-handler.php:358
heartbeat_received()loops over all client-provided IDs and callsget_post_meta()without any bound on the number of IDs. To avoid abuse/perf issues, consider hard-limiting the maximum IDs processed per tick (and skipping non-positive/invalid IDs) before querying meta.
foreach ( $data['videopress_processing_ids'] as $id ) {
$id = (int) $id;
$status = get_post_meta( $id, 'videopress_status', true );
if ( $status ) {
$statuses[ $id ] = $status;
projects/packages/videopress/src/class-attachment-handler.php:336
heartbeat_settings()unconditionally overwritesminimalIntervalto 10. If another component already lowered this value, this will actually raise it and could break expected behavior. Consider only lowering it (e.g., take the min of the existing value and 10) rather than always overriding.
public static function heartbeat_settings( $settings ) {
$settings['minimalInterval'] = 10;
return $settings;
projects/packages/videopress/src/js/media-library-poll.js:39
- The
heartbeat-sendhandler always forces the Heartbeat interval back tostandardwhen there are no processing videos. This can override other features/plugins that intentionally set a different interval on the page. Consider only switching tofastwhen needed (and otherwise leaving the interval untouched, or restoring only if this script previously changed it).
$( document ).on( 'heartbeat-send', function ( e, data ) {
const ids = getProcessingVideoIds();
if ( ids.length ) {
data.videopress_processing_ids = ids;
wp.heartbeat.interval( 'fast' );
} else {
wp.heartbeat.interval( 'standard' );
}
projects/packages/videopress/src/js/media-library-poll.js:66
- When Heartbeat reports an attachment is
complete, the code triggers aget-attachmentAJAX fetch but does not update the model status (or track an in-flight fetch). If the AJAX call is slow or fails, the attachment will stay marked asprocessing, causing repeated Heartbeat payloads and potentially repeatedget-attachmentrequests on subsequent ticks. Consider marking the model as complete (or storing a “refreshing” flag) before/while fetching, and handling the AJAX failure path to avoid retry loops.
$.each( data.videopress_processing_status, function ( id, status ) {
const attachment = library.get( id );
if ( attachment && status === 'complete' ) {
wp.ajax
.send( 'get-attachment', {
data: { id: id },
} )
.done( function ( attrs ) {
attachment.set( attrs );
} );
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Fixes https://github.com/Automattic/videopress/issues/1220
Proposed changes:
videopress_statuspost meta throughwp_prepare_attachment_for_jsso the Backbone model knows which videos are still processing.complete, refetch the full attachment data viaget-attachmentAJAX, updating the Backbone model and re-rendering the grid item automatically.Other information:
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
/wp-admin/upload.php(Media Library grid view).videopress_statustocomplete).videopress_processing_idsin its payload (check Network tab foradmin-ajax.phpheartbeat requests).Changelog
Changelog entry included in
projects/packages/videopress/changelog/fix-media-library-processing-status-refresh.