✅ 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

@claude those are many fixes as part of a polish we are doing for a demo, please review all.

Claude finished @AllTerrainDeveloper's task in 4m 39s —— View job

PR Review — Smooth desktop repaint on adds and removes

• Load tools and set up review
• Read core renderer change (src/desktop-files/layer.ts)
• Read drag/drop changes (src/drag/iframe-drop-targets.ts, src/drag-bridge.ts, src/gutenberg-drop-receiver.ts)
• Read bridge/connection changes (src/iframe-bridge-standalone.ts, src/connection/index.ts)
• Read window/API/desktop changes (src/window/index.ts, src/desktop.ts, src/api/facade.ts)
• Review CSS + dock changes
• Review PHP + documentation changes
• Post comprehensive review

This PR bundles eight commits' worth of polish across four distinct feature areas. I've reviewed every changed file. Overall the code is clean and the architecture is sound — the main path changes are correct and well-guarded. A few things need attention before merge.

1. Duplicate comment block — src/desktop-files/layer.ts:537–544

This is a copy-paste artifact. Two nearly-identical "Wholesale rebuild" comment blocks appear back-to-back before container.replaceChildren():

// Wholesale rebuild — last resort. Pin-flag flips, parent-
// folder moves, file-type changes, all land here. Plugins
// that want stable decorations re-attach via `tile-rendered`.

// Wholesale rebuild — last-resort path for transitions the
// incremental walker can't handle (pin-flag flips, parent-
// folder moves, file-type changes). Plugins that want stable
// decorations re-attach via `tile-rendered`.
container.replaceChildren();

One of them should be removed. Fix this →

2. src/desktop-files/layer.ts — incremental repaint path (correct, one note)

The overall design is sound: computeLayout() and wireTile() extracted correctly, both paths share the same code. The bail-to-wholesale conditions (file-type, file-ref, pinned-flag differ) are appropriately conservative.

tryPatchIncremental structural check loop iterates list, not existing: The loop checks list placements against DOM tiles. If the same id exists in DOM but with a different data-file-type, it bails — correct. But consider the edge case where a tile's data-placement-id attribute is present but doesn't match any current placement AND doesn't appear in wantIds. That tile gets removed in the removal loop either way, so no leak. ✓

Selection bookkeeping mirrors the wholesale path: Verified — both paths call notifySelection(null) when the selected id is gone. ✓

grid-rendered hook fires in both incremental and wholesale paths (lines 499 and 596). ✓

3. src/drag-bridge.ts — normalizeLegacyPayload — minor fragility

The pass-through check tests for the three known kinds explicitly:

if ( obj.kind === 'attachment' || obj.kind === 'post' || obj.kind === 'user' ) {
    return payload;
}

If a future payload kind is added to DragBridgePayload (e.g. 'video') and that shape happens to include id, url, and mime fields, it would be incorrectly re-wrapped as kind: 'attachment'. A defensive alternative:

if ( obj.kind !== undefined && obj.kind !== null ) {
    return payload; // already tagged — pass through regardless of which kind
}

The risk is low right now but this will grow with the API. Fix this →

4. src/gutenberg-drop-receiver.ts — attachToAllFrames() on every mutation

The MutationObserver is installed with { childList: true, subtree: true } and calls attachToAllFrames() on every DOM childList change. Gutenberg does a lot of DOM mutations. Each call does a querySelectorAll('iframe') walk and iterates the result checking the __desktopModeDropReceiverAttached sentinel — fast bails but still a hot allocation path inside Gutenberg.

For the demo this is fine. Longer term: observe only childList changes that add iframe elements, or debounce. Not a blocker but worth a // TODO comment.

5. src/iframe-bridge-standalone.ts — whenWindowId() never rejects

whenWindowId(): Promise< string > {
    if ( _windowId !== null ) {
        return Promise.resolve( _windowId );
    }
    return new Promise< string >( ( resolve ) => {
        _windowIdWaiters.push( resolve );
    } );
},

If the parent never sends a handshake (cross-origin parent, not inside a Desktop Mode window), this Promise never settles. Any plugin that does await wp.desktop.iframe.whenWindowId() unconditionally will hang forever. Same applies to Window.whenContentReady() in src/window/index.ts. The isParentReachable() helper mitigates this in the common case (caller should check first), but the new docs in docs/use-from-a-plugin.md and the example in docs/examples/iframe-initiated-window.md should call this out explicitly with a Promise.race against a timeout pattern, or at minimum a bold warning. Fix this →

6. GH#258 reversion — known regression

includes/plugins-window/rest-fields.php removes desktop_mode_plugins_window_count_visible_updates() and includes/core/payload.php removes the special-case that recomputed the Plugins dock badge as the intersection of the update_plugins transient with get_plugins(). The corresponding PHPUnit tests are deleted.

This restores the original drift where the dock badge can show a higher count than the Plugins window when the transient holds orphan entries (deleted/external-server plugin keys). If this is a deliberate trade-off for the demo (the fix was causing something else), it should be documented in the PR. If it was an accidental inclusion from cherry-pick / merge, it should be reverted to keep the fix.

7. Dock flat-structure simplification — overflow trade-off accepted

The removal of __scroll / __pinned inner wrappers and the shift to margin-top: auto on the separator is a clean simplification. The CSS comment acknowledges that scrollability was dropped: "Real-world menus fit the viewport without a scrollbar; if this ever needs to scroll, wrap tiles in a scroll container rather than clipping here." That's fine for the demo scope but is worth tracking as a follow-up for installs with long admin menus.

Minor / cosmetic

• src/connection/index.ts:318 — the connection field shipped in the hook payload adds a live object reference into what's otherwise a plain serializable payload. That's fine for same-process consumers but if anything ever serializes hook payloads (logging, devtools, postMessage relay), WindowConnection won't survive. Low risk given the current architecture but worth keeping in mind.

• assets/css/windows.css — top: 0 !important; left: 0 !important on .dashicons is the right call to defeat the admin-menu glyph offsets. The !important is justified; the comment explains why. ✓

• assets/css/posts-window.css — width: 100%; margin-inline: auto on .desktop-mode-users__edit-main is correct for wide-window centering. ✓

• includes/desktop-files/trash.php — qualifying id as f.id in the cascade-trash query prevents an ambiguous column error on MySQL strict mode. Good catch. ✓

• package.json — adding "import" and "default" export conditions alongside the existing "types" condition is correct for bundler tooling that follows the exports map. ✓

Summary

Three items worth addressing before merge: the duplicate comment (trivial), the normalizeLegacyPayload fragility (small), and the whenWindowId no-reject documentation gap. The GH#258 reversion needs clarification of intent.

@claude please fix all by yourself

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

Review fixes applied ✓

All actionable items from the code review have been fixed in commit 4466e89:

• Duplicate comment (src/desktop-files/layer.ts) — removed the first of the two back-to-back "Wholesale rebuild" comment blocks.
• normalizeLegacyPayload fragility (src/drag-bridge.ts) — replaced the explicit kind === 'attachment' || 'post' || 'user' allowlist with obj.kind !== undefined && obj.kind !== null, so any future tagged kind passes through without requiring a list update.
• whenWindowId() non-reject warning — added isParentReachable() guard pattern + bold warning to both docs/use-from-a-plugin.md and docs/examples/iframe-initiated-window.md.
• GH#258 reversion — includes/plugins-window/rest-fields.php and includes/core/payload.php are not in this PR's changed files, so there is nothing to revert here. The review was flagging prior branch history; no code change needed.
  · Branch: smooth-desktop-repaint