Skip to content

Add Media section to "My WordPress" + uniform preview-pane hook surface#232

Merged
AllTerrainDeveloper merged 6 commits into
trunkfrom
add-media-to-my-wordpress
May 17, 2026
Merged

Add Media section to "My WordPress" + uniform preview-pane hook surface#232
AllTerrainDeveloper merged 6 commits into
trunkfrom
add-media-to-my-wordpress

Conversation

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator

@AllTerrainDeveloper AllTerrainDeveloper commented May 17, 2026

Summary

Adds a fourth section to the My WordPress native window (alongside Posts, Pages, Users): Media.

  • Browse view — CSS-grid of thumbnails (newest first, infinite-scroll). Tiles are drag sources (kind: 'attachment'); the section never accepts drops — the existing window-level reject claimant handles that.
  • Type-aware preview pane — switches on MIME group: image/*<img>, video/*<video controls>, audio/* → poster + <audio>, PDFs / docs → big dashicon + "Open file" link. Metadata grid (filename, dimensions, file size, MIME, uploaded, uploader, alt text, caption, description).
  • Drill-in view — double-click a media item (or "See where this is used") opens a media-detail route showing every public post / page / CPT that references it (_thumbnail_id meta + wp-image-<id> class + raw URL embeds in post_content). Per-row capability-gated. Click a row to jump to that post's detail in the same window.
  • Navigation history back — the breadcrumb back button now uses a per-window history stack instead of the parent-folder rule, so cross-hierarchy jumps unwind in the order you made them (Media → Media-detail → referenced post → back lands on Media-detail, not Posts list).
  • JS entity-kind dispatcherwp.desktop.myWordpress.registerEntityKind( kind, renderer ) lets third-party plugins ship their own My WordPress section type without patching the bundle.

Extensibility (Experimental, since 0.21.0)

The hook surface is uniform across every section (posts, pages, users, media, plugin-defined kinds) — one descriptor, all surfaces.

PHP

Hook Type Purpose
desktop_mode_my_wordpress_preview_actions filter Server-declared action descriptors { id, label, icon, capability, mime (PCRE), sections, script }. Capability-gated before shipping to the bundle; script handle auto-enqueued.
desktop_mode_my_wordpress_media_usage filter The { media, usedIn } payload returned by GET /desktop-mode/v1/media-usage/<id>. Plugins (ACF, page builders, Yoast image meta) push additional rows here.
desktop_mode_my_wordpress_media_usage_cache_ttl filter Transient TTL (default 300s).
desktop_mode_my_wordpress_entities filter (existing) Now documented to accept kind => 'media' as well as plugin-defined kinds backed by a JS renderer.

JS

Hook Type Purpose
desktop-mode.my-wordpress.preview-actions filter (actions, { entityId, kind, mime?, item }) => actions — wire onSelect handlers onto server descriptors.
desktop-mode.my-wordpress.preview-extras action ({ slot: 'header'|'meta'|'footer', container, entityId, kind, item }) => void — inject arbitrary DOM into named slots.
desktop-mode.my-wordpress.tile-context-menu filter Same action shape, surfaced in per-tile right-click menus.
desktop-mode.my-wordpress.status-bar filter (existing) Unchanged.

Public API — wp.desktop.myWordpress

interface MyWordpressApi {
    openDetail( args: { entityId, postId, postTitle } ): void;
    openMedia( args: { mediaId, mediaTitle? } ): void;                    // new
    registerEntityKind( kind, renderer ): () => void;                     // new
}

New REST endpoint

GET /desktop-mode/v1/media-usage/<id> — drives the drill-in view.

{
    media:  { id, title, mime, sourceUrl, filename, date, author },
    usedIn: Array<{ postId, postType, postTypeLabel, title, status,
                    link, editLink, usedAs:'featured'|'content'|'meta',
                    authorId, authorName, date }>
}

Transient-cached (5 min default) per attachment_id × viewer-capability bucket. Cache busts on save_post, deleted_post, delete_attachment.

Files

New PHP

  • includes/my-wordpress/media-usage.php — endpoint + transient cache
  • includes/my-wordpress/preview-actions.php — descriptor aggregator + script auto-enqueue

New TS

  • src/my-wordpress/kind-registry.ts — runtime renderer registry
  • src/my-wordpress/media-rest.tsfetchMediaPage + fetchMediaUsage
  • src/my-wordpress/media-list.ts — grid + drag-out
  • src/my-wordpress/media-preview.ts — MIME-aware preview + action row + slot hooks
  • src/my-wordpress/media-detail.ts — drill-in usage list

Edited

  • src/my-wordpress/index.ts — registry-backed navigate(), media-detail route, history-stack back button, openMedia + registerEntityKind on the public API
  • src/my-wordpress/types.tsRoute extension, media types
  • src/my-wordpress/rest.ts — media branch in fetchEntityTotal
  • includes/my-wordpress/window.php — Media entity, previewActions + mediaPerPage in config
  • includes/my-wordpress/bootstrap.php — require new files
  • assets/css/my-wordpress.css — grid, preview pane, drill-in

Docs

  • docs/hooks-reference.md — three new filters documented
  • docs/javascript-reference.md — new hooks + public API
  • docs/examples/my-wordpress-media-action.md — copy/paste recipe

Tests

  • Vitestmy-wordpress-kind-registry.test.ts, my-wordpress-media-preview.test.ts (10 tests; full suite 1342 passing)
  • PHPUnitmyWordpressMediaUsage.php (5 tests: featured + content embeds, subscriber draft gating, transient cache, filter extensibility, 404), myWordpressPreviewActions.php (2 tests: capability gating, invalid-entry filtering)

Test plan

  • Hard-refresh after pulling — both desktop.min.js and my-wordpress.min.js are rebuilt; the lazy my-wordpress bundle is what runs inside the window.
  • My WordPress root shows Media · N alongside Posts / Pages / Users.
  • Open Media — grid of thumbnails, newest first. Non-image MIME shows a dashicon fallback.
  • Click a tile — right pane shows the correct preview (image / video / audio / document).
  • Double-click a tile (or "See where this is used") — drill-in shows referencing posts.
  • Click a referencing post — opens its detail. Press Back — lands on the Media drill-in (not Posts list).
  • Drag a media tile onto the wallpaper — placement created with kind: 'attachment'.
  • Drop a post / user tile onto the Media section — rejected with the "Can't drop here" hint.
  • Subscriber viewer — drill-in hides drafts they can't read.
  • Register a "Compress image" plugin descriptor (see docs/examples/my-wordpress-media-action.md) — button appears only on image items.

🤖 Generated with Claude Code

Open WordPress Playground Preview

- Implemented a type-aware media preview pane in `media-preview.ts` that handles various MIME types (images, videos, audio, documents) and displays appropriate elements.
- Created REST helpers in `media-rest.ts` for fetching media items and their usage, including a new endpoint for media usage.
- Added PHPUnit tests for the media usage endpoint, ensuring correct functionality and capability gating.
- Developed tests for preview actions, verifying that actions are filtered based on user capabilities and that invalid entries are dropped.
- Introduced Vitest tests for the kind registry and media preview functionalities, ensuring proper registration and rendering of media items.
@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude can you review this PR please?

@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @AllTerrainDeveloper's task in 3m 48s —— View job


PR Review: Add Media section to "My WordPress" + uniform preview-pane hook surface

  • Gather context and understand request
  • Read PHP files (media-usage.php, preview-actions.php, window.php, bootstrap.php)
  • Read TypeScript files (kind-registry.ts, media-rest.ts, media-list.ts, media-preview.ts, media-detail.ts, index.ts, types.ts, rest.ts)
  • Read test files
  • Read docs changes
  • Read CSS changes
  • Synthesize and post review

This is a large, well-structured PR. The PHP is clean, the hook surface is well-designed, and the tests are solid. Below are bugs and concerns ordered by severity.


Bugs

1. Regex operator precedence in MIME_DASHICON_MAPmedia-preview.ts:40

{ test: /^application\/zip|x-tar|x-rar|x-7z/, icon: 'dashicons-media-archive' },

The | alternates the entire expression, not just the right-hand fragment. This regex matches ^application/zip OR x-tar OR x-rar OR x-7z — so any MIME string containing x-tar anywhere (e.g. audio/x-tar-custom) would incorrectly get the archive dashicon. Fix:

{ test: /^application\/(zip|x-tar|x-rar|x-7z)/, icon: 'dashicons-media-archive' },

Fix this →

2. CPT usage rows route to the wrong entity — media-detail.ts:282–284

openDetailInWindow({
    entityId: row.postType === 'page' ? 'pages' : 'posts',
    postId: row.postId,
    postTitle: row.title,
});

Any CPT (WooCommerce product, shop_order, plugin-defined types) lands in the 'posts' entity, which fetches from wp/v2/posts. Clicking the row opens a detail view that then tries to load that post from the posts endpoint — which 404s. The fix is to check the registered entities list for a matching restPath (e.g. wp/v2/product) rather than hard-coding 'page' → pages, else → posts:

const match = getConfig().entities.find(
    (e) => e.kind === 'post' && e.restPath.endsWith( '/' + row.postType ) ||
           e.id === row.postType + 's' // simple pluralisation fallback
);
const entityId = match?.id ?? 'posts';

Or simply check row.postType against entity IDs directly (row.postType === 'page' ? 'pages' : row.postType + 's' would work for most standard WP CPTs, but ideally this should match against the registered entity list).

Fix this →


Medium concerns

3. registerEntityKind timing — public API docs gap

wp.desktop.myWordpress is assigned at the end of the lazy-loaded my-wordpress bundle, which only runs the first time the window opens. A plugin that calls wp.desktop.myWordpress.registerEntityKind(...) at script-load time (the natural pattern) will throw Cannot read properties of undefined because the bundle hasn't loaded yet.

The docs example in my-wordpress-media-action.md shows the registration call without any timing guard. The example needs a guard, and the docs should advise using wp.desktop.onWindow or a desktop-mode-window-opened listener to defer the call:

document.addEventListener('desktop-mode-window-opened', function handler(e) {
    if (e.detail?.windowId !== 'desktop-mode-my-wordpress') return;
    wp.desktop.myWordpress.registerEntityKind('my-plugin/order', renderer);
    document.removeEventListener('desktop-mode-window-opened', handler);
});

Or expose an early-registration queue in the bundle (like how desktopModeNativeWindows queues render callbacks) so callers don't need to reason about lazy-load timing.

4. Cache bust functions duplicate the key format — media-usage.php:327–328

desktop_mode_my_wordpress_media_usage_cache_key() owns the key format. The bust functions (bust_for_post, bust_for_attachment) hard-code the same format again:

delete_transient( 'dm_media_usage_' . (int) $attachment_id . '_edit_v1' );
delete_transient( 'dm_media_usage_' . (int) $attachment_id . '_read_v1' );

The helper function also hard-codes the two bucket values (edit, read). If a third capability bucket were ever added, bust functions would silently miss it. Consider:

foreach ( array( 'edit', 'read' ) as $bucket ) {
    delete_transient( 'dm_media_usage_' . $bucket_key_prefix . $bucket . '_v1' );
}

Or export the key-format logic into a shared helper that both the writer and busters call.

Fix this →

5. Filter applied to every request but cached data never includes filter output

In media_usage_callback():

// On cache hit:
return apply_filters( 'desktop_mode_my_wordpress_media_usage', $cached, $attachment_id );

// On cache miss:
set_transient( $cache_key, $payload, ... );
return apply_filters( 'desktop_mode_my_wordpress_media_usage', $payload, $attachment_id );

The transient stores only the pre-filter payload. On a cache hit, the filter still runs, meaning plugin-added rows (ACF image meta, etc.) are live but the base payload is stale until cache bust. This is probably the intended design (plugin data stays fresh), but it's not documented anywhere. A comment explaining this choice would help future contributors.


Low / style concerns

6. import statement after a function definition — index.ts:51–54

The getDragManager function definition (lines 45–50) appears before the renderBreadcrumbs import (lines 51–54). All imports should live at the top of the file. This is likely a merge artifact from adding getDragManager to the top of the file without moving it before the imports. TypeScript hoists imports so it works, but it reads as a style error and will likely draw a lint warning depending on the ESLint import order config.

7. getDragManager() and stripTags() duplicated three times each

Both helpers appear verbatim in index.ts, media-list.ts, and media-detail.ts (three copies each). They belong in a shared utility file or can be imported from the one that already exists. Not a bug, but DRY violation that will drift.

8. void sprintf import suppressor — media-preview.ts:421–422

// Suppress an unused-import diagnostic when sprintf isn't used in
// this build — keep the i18n surface ready for future strings.
void sprintf;

AGENTS.md explicitly warns against future-proofing that isn't needed now. If sprintf is unused, remove the import and add it back when the string is added. A stale void x will confuse future readers.

9. <dl> with <span> term/value children — media-preview.ts:208–243

The buildMetadataGrid function returns a <dl> element, but buildMetaRow builds <div> wrappers with <span> children (not <dt>/<dd>). <div> wrappers inside <dl> are valid HTML5, but using <span> instead of <dt>/<dd> breaks the semantic list structure, which matters for screen readers navigating the metadata grid with definition-list keyboard shortcuts.

10. Misleading test name — myWordpressMediaUsage.php:208

public function test_unknown_attachment_returns_404() {
    ...
    $this->assertSame( 403, $response->get_status() );
}

The method is named _returns_404 but asserts 403. The 403 is correct (permission callback returns false → WP issues 403 to authenticated users), but the name says 404. Rename to test_unknown_attachment_returns_403_via_permission_gate.


Nitpicks

  • media-detail.ts:239: host.route.kind === 'media-detail' ? host.route.entityId : 'media' — the fallback branch is dead code since renderMediaDetail is only ever called from navigate() when route.kind === 'media-detail'. The ternary can be collapsed to host.route.entityId.
  • media-preview.ts:287–291: MIME sections filter silently ignores a * wildcard correctly, but ctx.entityId is never '*' — the section filter should accept '*' as "show on all sections" which it does (a.sections.includes('*')). ✅ This is fine.
  • media-list.ts:386: void loadMore() loads page 1 unconditionally before the IntersectionObserver. If IntersectionObserver is absent (old Safari), page 1 loads but infinite scroll silently breaks. A plain fallback loadMore() chain in the typeof IntersectionObserver === 'undefined' else-branch would degrade gracefully.

What's solid

  • PHP permission model is correct: permission_callback enforces read_post on the attachment; per-row current_user_can('read_post') gates drafts from subscribers.
  • Cache bust coverage is complete: save_post, deleted_post, delete_attachment are all wired.
  • The resolvePreviewActions filter-on-cached-descriptors pattern (re-evaluating client-side sections + mime per item) is the right way to avoid stale action visibility.
  • kind-registry.ts unregister semantics are correct — stale unregister functions don't remove a subsequent re-registration (renderers.get(kind) === renderer guard).
  • Docs are updated in the same PR. ✅
  • Test coverage for capability gating, transient cache, filter extensibility, and filter malformed-entry guards is thorough.
    |

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 17, 2026

✅ WordPress Plugin Check Report

✅ Status: Passed

📊 Report

All checks passed! No errors or warnings found.


🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude another round please?

@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @AllTerrainDeveloper's task in 6m 2s —— View job


PR Review — Round 2

  • Gather context and understand request
  • Read all new TypeScript source files
  • Read PHP files
  • Read test files
  • Read CSS and docs changes
  • Synthesize and post review

Round 1's bugs are all fixed — the regex is right, CPT routing uses the new entityIdForPostType, the cache buster uses the shared helper, dom-utils.ts eliminates the duplications, dt/dd semantics are correct, the test name is fixed, and the early-stub queue resolves the timing concern. Good iteration.

Here's what this pass found.


Bugs

1. Status bar painted with stale data after navigation-during-fetch — media-detail.ts:321

renderMediaDetail is async. The statusBar reference is resolved before the await fetchMediaUsage() call (line 238–244) by querying the live template element. That element is not inside host.body — it lives alongside it in the root template — so state.body.replaceChildren() (called by navigate() when the user moves away) does not remove it from the DOM.

If the user opens a media-detail view and navigates to a different route while the fetch is in-flight:

  1. navigate() fires → clearTeardown() + body.replaceChildren() — the new route paints its own status bar.
  2. The fetch resolves; execution continues from line 289.
  3. All the DOM work (left.replaceChildren(), right.replaceChildren(), list.appendChild()) lands in a detached subtree — harmless.
  4. Line 321paintStatus( statusBar, usage.usedIn.length, entityId ) — the statusBar is still attached and live. The media-detail count overwrites whatever the new route just painted.

The defensive check at line 264 (host.route.kind !== 'media-detail') does not help here: host.route is a snapshot captured in makeRenderHost() at call time, not the live state.route. It will always equal 'media-detail' for this invocation.

Fix — add a liveness guard after the await using the detached state of wrap as the signal:

usage = await fetchMediaUsage( mediaId );
// Guard against the user navigating away mid-fetch.
if ( ! wrap.isConnected ) {
    return;
}

Fix this →


2. desktop-mode.my-wordpress.tile-context-menu hook declared in the PR but never implemented

The PR description table lists:

desktop-mode.my-wordpress.tile-context-menu | filter | Same action shape, surfaced in per-tile right-click menus.

Searching src/ and docs/ finds zero occurrences. Neither the media grid tiles nor the existing post tiles call applyFilters('desktop-mode.my-wordpress.tile-context-menu', …). The hook is described as public API but the filter is never fired, the docs don't document it, and there's no JS hook registration entry for it in javascript-reference.md.

Either remove the row from the PR description or implement + document the hook before merging.


Medium concerns

3. resolvePreviewActions silently skips MIME filter when ctx.mime is undefined — media-preview.ts:285

if ( a.mime && ctx.mime ) {
    // … test regex …
}

When a.mime is set but ctx.mime is absent, the entire mime block is skipped and the action passes through. The built-in call sites always supply mime, but the docs expose resolvePreviewActions as callable by plugins on any section kind — and they won't set mime for post/user contexts. A plugin that adds a media-only action (mime: '^image/') and that same action descriptor ends up in the previewActions config array visible to a post-section call will show up even on text posts.

The correct semantics for "I set a mime pattern but the context doesn't have one" is probably return false (fail closed), not pass-through:

if ( a.mime ) {
    if ( ! ctx.mime ) {
        return false; // MIME-scoped action can't apply to a non-media item.
    }
    try {
        if ( ! new RegExp( a.mime ).test( ctx.mime ) ) {
            return false;
        }
    } catch {
        return false;
    }
}

Fix this →

4. Stub unregister becomes a no-op after bundle drain — early-api.ts:70–76 / index.ts:5672

The stub registerEntityKind pushes into queue and returns a closure that splices from queue. When the lazy bundle mounts, it drains queue (pending.length = 0, index.ts:5672) and replaces desktopGlobal.myWordpress with the real API.

Any stub-unregister function a plugin cached before the bundle loaded now splices from an empty array — the real registry (renderers map in kind-registry.ts) retains the kind permanently. A plugin that registers a kind at script-load time and wants to unregister it later (e.g., beforeunload, or a SPA-style cleanup) silently fails.

The simplest fix: after drain, swap each queue entry's stub unregister for the real unregister returned by the registry call:

for ( const entry of pending ) {
    try {
        const realUnregister = registerEntityKind( entry.kind, entry.renderer );
        // Patch the entry so any held stub-unregister now delegates to the real one.
        entry.renderer = realUnregister as unknown as ( ...args: unknown[] ) => void;
    } catch {  }
}

Or document in early-api.ts that stub unregisters become no-ops after mount and that the caller should re-call wp.desktop.myWordpress.registerEntityKind if they need a live unregister.


Nitpick

5. CPT usage rows always show the post dashicon — media-detail.ts:151–155

const icon = document.createElement( 'span' );
icon.className =
    row.postType === 'page'
        ? 'dashicons dashicons-admin-page'
        : 'dashicons dashicons-admin-post';

WooCommerce product, shop_order, any plugin-defined CPT gets the post icon. entityIdForPostType(row.postType) already looks up the entity by restPath; the same lookup could surface the entity's icon field. A one-liner fix when the entity is found:

const entityForRow = getConfig().entities.find(
    ( e ) => e.restPath.endsWith( '/' + row.postType )
);
icon.className = entityForRow
    ? `dashicons ${ entityForRow.icon }`
    : row.postType === 'page'
        ? 'dashicons dashicons-admin-page'
        : 'dashicons dashicons-admin-post';

What's solid in this revision

  • entityIdForPostType() is clean and the strategy comments are clear.
  • desktop_mode_my_wordpress_media_usage_cache_buckets() as the shared bucket list is the right call — writer and busters can't drift.
  • dom-utils.ts eliminating the three duplicate helpers: correct DRY resolution.
  • early-api.ts queue + drain pattern elegantly solves the lazy-bundle timing problem for the 95% case.
  • wrap.appendChild before the await (not after) means the spinner is visible immediately — correct UX sequencing.
  • The kind-registry.ts unregister guard (renderers.get(kind) === renderer) is subtle but right.

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude let's give it a last round please

@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @AllTerrainDeveloper's task in 6m 0s —— View job


PR Review — Round 3

  • Gather context and read current file state
  • Read PHP files
  • Read TypeScript source files
  • Read test files and docs
  • Synthesize and post final review

Round 2's findings are all addressed: the wrap.isConnected liveness guard is in place, tile-context-menu is implemented for post/page tiles and documented with the "follow-up for user/media" caveat, MIME filtering fails closed, the stub-unregister forwards through the real registry after drain, entityIconForPostType covers CPTs, and openUserTileMenu now has matching outside-click/Escape dismissers. Good iteration.

Two new bugs this round.


Bugs

1. $class_pattern matches attachment IDs that are numeric prefixes — media-usage.php:257

$class_pattern = '%wp-image-' . $attachment_id . '%';

The trailing % is a LIKE wildcard. For attachment ID 12, the generated pattern is %wp-image-12%, which matches any post_content containing wp-image-12 anywhere — including wp-image-120, wp-image-123, wp-image-1234, etc. On a site with sequential media IDs this means every post that embeds attachment 123 would also falsely appear in the drill-in view for attachment 12.

There is no PHP-side precision check after the SQL scan (lines 273–278 push every row directly into $rows_by_post), so the false positives reach the API response.

Fix — add a regex word-boundary re-check after the SQL fetch:

foreach ( (array) $content_rows as $pid ) {
    $pid = (int) $pid;
    if ( $pid <= 0 || isset( $rows_by_post[ $pid ] ) ) {
        continue;
    }
    $content_post = get_post( $pid );
    if ( ! $content_post ) {
        continue;
    }
    // The LIKE scan over-fetches: `%wp-image-12%` also matches
    // `wp-image-123`. Confirm with a word-boundary regex here.
    // Also accept posts that contain the full attachment URL (the
    // second LIKE arm), using the precise URL rather than basename.
    if ( ! preg_match( '/\bwp-image-' . $attachment_id . '\b/', $content_post->post_content )
         && ( '' === $file_url || false === strpos( $content_post->post_content, $file_url ) )
    ) {
        continue;
    }
    $rows_by_post[ $pid ] = 'content';
}

This supersedes the existing loop at lines 273–278.

A test covering the false-positive case (e.g., a post containing wp-image-{id}9) should accompany the fix.

Fix this →


2. Cache bust on save_post doesn't cover reference removal — media-usage.php:344–372

desktop_mode_my_wordpress_media_usage_bust_for_post() reads the post as it exists after the save, then extracts wp-image-N classes from the new content:

$post = get_post( $post_id );
if ( $post && … && false !== strpos( $post->post_content, 'wp-image-' ) ) {
    preg_match_all( '/wp-image-(\d+)/', $post->post_content, $m );
    // bust cache for those IDs
}

If a user removes a wp-image-12 block from a post and saves, the new content no longer contains the pattern. $m doesn't include 12, so the cache for attachment 12 is never busted. The drill-in view for attachment 12 continues to show that post as a reference until the TTL expires (up to 5 min).

The same issue applies to the deleted_post hook: get_post() returns the already-deleted post, which may return null from the object cache depending on timing, so the content scan may silently do nothing.

The correct fix requires knowing what was in the post before the change. Options in order of preference:

  1. Hook pre_post_update (fires before the update with the old data still in the database) to read the old post and record which attachments it referenced; then use those IDs in the bust.
  2. Run a broader bust: on any save_post, delete transients for all attachment IDs currently referenced and all IDs that were in the stored transient. The stored transient's usedIn array contains the prior set.
  3. Accept 5-minute eventual consistency for reference removal (document this explicitly in a code comment).

Fix this →


Low / style concerns

3. Test tear_down hard-codes transient key format — myWordpressMediaUsage.php:49–50

delete_transient( 'dm_media_usage_' . $this->attachment_id . '_edit_v1' );
delete_transient( 'dm_media_usage_' . $this->attachment_id . '_read_v1' );

The shared desktop_mode_my_wordpress_media_usage_cache_key() helper exists precisely to own this format. The test's tear_down bypasses it, so if the key scheme ever changes, cleanup silently stops working. Replace with:

foreach ( desktop_mode_my_wordpress_media_usage_cache_buckets() as $bucket ) {
    delete_transient(
        desktop_mode_my_wordpress_media_usage_cache_key( $this->attachment_id, $bucket )
    );
}

4. No test for MIME fail-closed path in resolvePreviewActions

The round-2 fix for the a.mime && !ctx.mime → return false case is correct, but the test suite in my-wordpress-media-preview.test.ts doesn't exercise it. A one-line addition to the existing resolvePreviewActions test covers it:

// MIME-scoped action must NOT appear in a non-media (no-mime) context.
const nonMediaCtx = { entityId: 'posts', kind: 'post', item: {} };
const nonMediaOut = resolvePreviewActions( actions, nonMediaCtx );
expect( nonMediaOut.map( ( a ) => a.id ) ).not.toContain( 'c' ); // c has mime: '^image/'
expect( nonMediaOut.map( ( a ) => a.id ) ).not.toContain( 'd' ); // d has mime: '^video/'

What's solid in this revision

  • wrap.isConnected liveness guard after fetchMediaUsage is well-placed and the comment explaining why host.route can't be trusted is exactly right.
  • early-api.ts slot pattern (entry.slot.unregister = registerEntityKind(...)) correctly bridges the pre-mount stub to the live registry without requiring callers to add guards.
  • entityIdForPostType / entityIconForPostType factored out cleanly; the strategy-comment in each explains the fallback chain.
  • The "pre-filter payload cached" design choice is now documented inline (lines 158–169 of media-usage.php) — that's the right place for it.
  • openUserTileMenu now has the same outside-click/Escape dismisser block as openTileMenu. ✅
  • tile-context-menu scope acknowledged in the docs ("follow-up" note) is the honest contract — better than silently wiring it to only some tiles.

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude one more?

@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @AllTerrainDeveloper's task in 5m 43s —— View job


PR Review — Round 4

  • Gather context and read current file state
  • Read PHP files (media-usage.php, preview-actions.php, window.php)
  • Read TypeScript source files (media-detail.ts, media-preview.ts, media-list.ts, index.ts, early-api.ts, kind-registry.ts, dom-utils.ts, media-rest.ts)
  • Read test files and docs
  • Synthesize and post final review

Round 3's findings are all landed: the numeric-prefix false positive is caught with the (?!\d) negative lookahead, the pre_post_update snapshot for reference removal is in place and tested, tear_down uses the shared key helper, and the MIME fail-closed path has a test. Good clean-up pass.

One real bug this round, one low-concern style issue.


Bug

1. deleted_post fires after the post row and its meta are already removed — bust is a no-op — media-usage.php:455

add_action( 'deleted_post', 'desktop_mode_my_wordpress_media_usage_bust_for_post' );

WordPress's wp_delete_post deletion sequence:

do_action( 'before_delete_post', $postid, $post );
// delete comments, term relationships...
delete_all_meta_for_post( $postid );   ← _thumbnail_id meta gone
do_action( 'delete_post', $postid, $post );
$wpdb->delete( $wpdb->posts, ... );    ← row gone
do_action( 'deleted_post', $postid, $post );  ← current hook

When deleted_post fires:

  • get_post( $post_id ) returns nullextract_refs() returns [].
  • _thumbnail_id meta has already been deleted by delete_all_meta_for_post.
  • There are no pre-save refs (those are populated by pre_post_update, which only fires on updates, not deletes).

The result: deleting a post that references attachment N never busts the media-usage cache for N. The drill-in view shows the deleted post as a reference until the 5-minute TTL expires.

Fix — swap to before_delete_post, which fires before any cleanup:

// Replace:
add_action( 'deleted_post', 'desktop_mode_my_wordpress_media_usage_bust_for_post' );
// With:
add_action( 'before_delete_post', 'desktop_mode_my_wordpress_media_usage_bust_for_post' );

before_delete_post passes ( $postid, $post ) — the function only reads the first arg, so the signature is compatible. At this hook point get_post() still returns the live row and get_post_meta() still finds _thumbnail_id.

Add a PHPUnit test to lock it in:

public function test_post_deletion_busts_attachment_cache() {
    $post = self::factory()->post->create( array(
        'post_status'  => 'publish',
        'post_content' => '<img class="wp-image-' . $this->attachment_id . '" src="x"/>',
    ) );
    // Warm the cache.
    $this->dispatch( $this->attachment_id );
    $cache_key = desktop_mode_my_wordpress_media_usage_cache_key(
        $this->attachment_id, 'edit'
    );
    $this->assertNotFalse( get_transient( $cache_key ) );
    // Delete the post — cache must be busted.
    wp_delete_post( $post, true );
    $this->assertFalse( get_transient( $cache_key ) );
}

Fix this →


Low / style concern

2. Missing return after the media-detail branch in navigate()index.ts:301

Every other branch in navigate() ends with an explicit return:

if ( route.kind === 'list' ) { ...; return; }
if ( route.kind === 'detail' ) { ...; return; }
if ( route.kind === 'sub-list' ) { ...; return; }
if ( route.kind === 'user-footprint' ) { ...; return; }
if ( route.kind === 'media-detail' ) {
    void renderMediaDetail( makeRenderHost( state ), route.mediaId );
    // ← no return
}

Because media-detail is the last branch, there's no observable bug today. But if a new route kind is ever added after it, the missing return becomes a silent fall-through where renderError (or nothing) fires for the media-detail route. One-line fix:

if ( route.kind === 'media-detail' ) {
    void renderMediaDetail( makeRenderHost( state ), route.mediaId );
    return;
}

Fix this →


What's solid in this revision

  • before_delete_post is actually the right hook for the deletion snapshot in the reference-removal test — test_reference_removal_busts_cache correctly exercises the wp_update_post path (which triggers pre_post_update + save_post). ✅
  • extract_refs accepts both int|WP_Post — meaning the fix for before_delete_post requires no signature change to the existing function. Consistent design.
  • class_re = '/wp-image-' . $attachment_id . '(?!\d)/' is the correct lookahead (rejects wp-image-123 while accepting wp-image-12", wp-image-12 , etc.).
  • The $file_url re-check in extract_refs uses strpos($haystack, $file_url) with the full URL — correct, not just the basename.
  • All Round 3 findings addressed. The before_delete_post pattern was already used correctly for the edit path (pre_post_update); the deletion path just needs the same treatment.

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude now?

@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @AllTerrainDeveloper's task in 5m 11s —— View job


PR Review — Round 5

  • Gather context and read current file state
  • Check Round 4 fixes are landed
  • Review PHP files
  • Review TypeScript source files
  • Review test files and docs
  • Synthesize and post final review

Round 4's findings are all addressed: before_delete_post is in place (with the correct inline comment explaining why deleted_post is too late), the deletion regression test is included, the missing return after media-detail is fixed (with an ESLint suppressor and an explanatory comment). Good clean-up pass.

One real bug this round.


Bug

1. onBack guard contradicts backDisabled — back button is enabled but a no-op after breadcrumb jumps — index.ts:469–488

backDisabled is isRoot && state.history.length === 0. When the user navigates to root from a deep view via the "My WordPress" breadcrumb (not via the back button), navigate() is called with opts.fromBack unset, so the current deep route gets pushed onto state.history. Control then reaches updateBreadcrumbs: isRoot = true, state.history.length > 0, so backDisabled = false — the button renders as enabled.

But onBack has:

onBack: () => {
    if ( state.route.kind === 'root' ) {
        return;  // ← guard fires immediately
    }
    
},

The user sees an enabled back button, clicks it, and nothing happens — the guard short-circuits before state.history.pop() is reached.

Concrete path to reproduce:

  1. Open My WordPress. Navigate to Media, then double-click a tile to open the media-detail view.
  2. Click "My WordPress" in the breadcrumb bar to jump directly to the root view.
  3. History now contains [{kind:'list', entityId:'media'}, {kind:'media-detail', …}]. Back button is visually enabled.
  4. Click back. onBack fires → state.route.kind === 'root' → returns immediately. Back button does nothing.

The fix is to remove the early-return guard. With it removed, pop() produces the previous route and navigate rewinds correctly. The empty-history-at-root case (history.pop() returns undefined) falls through to navigate(state, parentRoute({kind:'root'})), which produces another {kind:'root'} — a same-route no-op that routesEqual prevents from re-poisoning history. The backDisabled: isRoot && state.history.length === 0 condition already disables the button in that case anyway, so the guard is double-coverage that's in conflict with the enabled-state logic rather than a safety net:

onBack: () => {
    // Removed: if ( state.route.kind === 'root' ) { return; }
    const previous = state.history.pop();
    if ( previous ) {
        navigate( state, previous, { fromBack: true } );
        return;
    }
    navigate( state, parentRoute( state.route ), { fromBack: true } );
},

Fix this →


What's solid in this revision

  • before_delete_post hook + the inline comment at lines 455–462 explaining why deleted_post is too late is exactly the right documentation placement. Future contributors won't be tempted to revert it.
  • test_post_deletion_busts_attachment_cache locks in the regression with wp_delete_post($post, true) — force-delete path confirmed.
  • The extract_refs function uses preg_match_all('/wp-image-(\d+)/', ...) with a greedy \d+ match — correctly captures the full ID (not a prefix), so the pre/post save ref union computes the right attachment IDs to bust. No false positives there.
  • early-api.ts slot pattern (entry.slot.unregister = registerEntityKind(...)) bridges pre-mount unregisters cleanly. Duplicate-kind registrations in the queue resolve to "last call wins" with the first entry's unregister becoming a correctly-scoped no-op — consistent with the documented registry semantics.
  • The ESLint suppressor // eslint-disable-next-line no-useless-return on the media-detail branch return; is the right approach — explains the intent without leaving a silent asymmetry for future route additions.
  • tile-context-menu caveat in the docs ("user and media tile menus will subscribe in a follow-up") is the honest contract.

@AllTerrainDeveloper AllTerrainDeveloper merged commit aa4fdd5 into trunk May 17, 2026
5 checks passed
@AllTerrainDeveloper AllTerrainDeveloper deleted the add-media-to-my-wordpress branch May 17, 2026 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant