VideoPress: show a deleting progress state in the dashboard library#49558
VideoPress: show a deleting progress state in the dashboard library#49558vianasw wants to merge 3 commits into
Conversation
Deleting media from the modernized VideoPress dashboard gave no feedback until the listing refetched, so the action felt frozen (CFT item VP09). - Add a 'deleting' upload status that rides the same in-flight channel as 'promoting': rows get a "Deleting…" overlay with an indeterminate progress bar in the grid layout and a "Deleting…" pill in the table layout, and stay non-interactive (no title link, no thumbnail button, no eligible row actions) until the refetched listing removes them. - Run bulk deletions in parallel (Promise.allSettled) instead of sequentially, and reject with a DeleteVideosError carrying the failed ids so partial failures report a precise count and keep the surviving rows selected. - Invalidate the library cache on settle and await the refetch before clearing the overlay so rows never flash back to their normal state ahead of removal; scope the invalidation away from item queries so the details page doesn't refetch the id it just deleted. - React via the mutateAsync promise instead of mutate-level callbacks, which TanStack drops when another mutation starts on the same observer or the component unmounts mid-flight. - Details page: show a persistent "Deleting video…" snackbar while the delete is in flight, guard against double-fire, and skip the post-delete navigation if the user already left the page. Fixes VIDP-251. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. Notes from the review (verified non-blocking): the 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Code Coverage SummaryCoverage changed in 4 files.
1 file is newly checked for coverage.
|
Cleanup pass over the previous commit, no behavior changes: - Hoist the duplicated LibraryItem test fixture into test-utils/library-item.ts and reuse it in both library test suites. - Merge the promoting/deleting thumbnail overlays into one block (they differed only in the label). - Replace the settle notices' remove-then-create dance with same-id replacement (the notices store drops an existing notice with the same id on create). - Flip isVideoPressIdle from a status blocklist to an allowlist on 'idle', matching the other render sites, so future statuses are excluded from row actions by default. - Flatten the value-threaded .then/.catch/.then chain in deleteItems into async/await with a linear cleanup tail; hoist the requested-ids Set out of the selection filter. - Drop the no-op size guard around the renderedItems overlay map. - Share the item-query key segment between use-video and the delete hook's invalidation predicate instead of re-encoding 'item' at each site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
From a recall-mode review pass over the branch: - Drop deleted ids' item-detail queries from the cache on settle. Invalidating doesn't help (the refetch 404s and stale data is retained), so a cached entry let a back-navigation render a ghost editor for a deleted video. - Key the details page's in-progress notice by video id so two overlapping deletes (start one, navigate away mid-flight, delete another) can't replace each other's notices. - Disable Save on the details page while a delete is in flight so a slow delete can't be raced by a meta update. - Announce library deletes to screen readers: the row overlay/pill is purely visual, so surface an in-progress notice (replaced in place by the settle notice) matching the details-page pattern. - Finish the LIBRARY_ITEM_QUERY_SEGMENT migration in use-update-video-meta and use-update-video-poster, which still hardcoded 'item'. - Changelog type fixed → added (new UI state + behavior change, not a bug fix). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CGastrell
left a comment
There was a problem hiding this comment.
Reviewed both commits (initial implementation + the bdff83a cleanup pass). The deleting-progress flow is carefully designed and well-tested.
Verified against source:
- Cache invalidation predicate correctly excludes item-detail queries (
LIBRARY_ITEM_QUERY_SEGMENT), avoiding the 404+retry stall on the details page;mutateAsync+onSettledreturning the invalidation promise keeps the "Deleting…" overlay until the refetched listing actually drops the rows — no flash-back. - Parallel
Promise.allSettled+DeleteVideosErrorpartial-failure contract is sound: failed rows restore to interactive and stay selected, successful ones disappear. - Non-interactivity while deleting is enforced at every render site (action eligibility, title link, thumbnail button).
- Same-id notice replacement on the details page is correct —
@wordpress/noticesdrops the existing same-id notice on create, so theexplicitDismisssnackbar is cleanly replaced (no orphan).
The cleanup commit is a strict improvement (flattened async/await, merged overlay block, shared query-key constant, hoisted test fixture, allowlist-on-idle eligibility) with no regressions.
LGTM 🚀
Fixes VIDP-251 (CFT feedback item VP09, reported in #49485)
Proposed changes
Deleting media from the modernized VideoPress dashboard gave no feedback until the listing refetched, so the action felt frozen — especially noticeable because the backend also removes the remote VideoPress copy, which takes a while.
'deleting'upload status rides the same in-flight channel as'promoting': the grid shows a "Deleting…" overlay with an indeterminate progress bar on each affected card, the table shows a "Deleting…" pill next to the title. Rows mid-delete are non-interactive (no title link, no thumbnail click-through, no eligible row actions), so a slow delete can't be double-fired or raced by an edit.Promise.allSettledinstead of a sequentialawaitloop — directly addressing the "slow" half of the complaint. Partial failures reject with aDeleteVideosErrorcarrying the failed ids: the error notice reports a precise count ("Failed to delete 1 video."), failed rows return to their normal interactive state and stay selected, successful ones disappear.mutateAsyncpromise rather than mutate-level callbacks, which TanStack silently drops when another mutation starts on the same observer (overlapping deletes) or the component unmounts mid-flight. The cache invalidation excludes per-item queries so the details page no longer refetches the id it just deleted (a guaranteed 404 + retry that would stall navigation).Known cosmetic limitation: if every selectable row is mid-delete, DataViews briefly collapses the bulk-selection checkbox column until the rows are removed. Transient and harmless.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Requires the modernized VideoPress dashboard (
add_filter( 'rsm_jetpack_ui_modernization_videopress', '__return_true' );) and a site with VideoPress videos.DELETE /wp/v2/media/{id}request in devtools). The failed row should return to its normal interactive state, stay selected, and an error notice with the failed count should appear.Screenshots (captured on a local site with mocked media responses; the delete request is stalled 8s to make the in-flight state visible)
Unit tests:
cd projects/packages/videopress && pnpm test(covers parallel deletion + partial-failure contract, the deleting render states in both layouts, action eligibility while deleting, and the cache-invalidation predicate).🤖 Generated with Claude Code