Drag & Drop from local desktop

[AllTerrainDeveloper]

Drag.and.drop.mov

[AllTerrainDeveloper]

@claude can you review?

[claude]

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

---

Code Review — PR #236: Drag drop from local desktop

• Read changed source files
• Review core OS file drop module (src/os-file-drop/)
• Review bridge & iframe integration
• Review PHP side (includes/render/)
• Review types and hooks
• Review tests
• Review docs
• Post final review

---

This is a well-structured feature. The architecture (lazy dialog, sentinel-based idempotency, hook pipeline, server-sourced allowed-mime config) follows the established patterns in this codebase correctly. The security model around postMessage is sound. Below are the issues I found, ordered by severity.

---

Bugs / Correctness

1. Dead event listener in dialog.ts — src/os-file-drop/dialog.ts:181

modal.addEventListener( 'remove', () => resolve(), { once: true } );

'remove' is not a standard DOM event. This listener will never fire. The MutationObserver at line 182–190 already handles modal-removal correctly, so there's no functional breakage — this is just dead code that could mislead a future reader. Fix this →

2. Duplicate JSDoc on dropConfig? — src/types.ts

Two consecutive /** ... */ blocks were added for the single dropConfig? field — the @since 0.30.0 tag is in the first block, but TS tooling / TSDoc only picks up the last /** before a declaration, so @since is silently dropped in generated docs.

3. Misplaced import type in manager.ts — src/os-file-drop/manager.ts:121

// (two functions defined above this line)
import type { DropConfig } from './types';  // ← mid-file

This import type appears after the dragHasFiles and resolveWindowIdFromSource function declarations. ES modules hoist imports so it compiles correctly, but eslint-plugin-import's import/first rule flags any non-import statement before an import. This is likely a cut-paste artifact and should be moved to the top with the other imports. Fix this →

---

Code Quality

4. formatBytes is duplicated between manager.ts and dialog.ts, with different precision

manager.ts:697 uses .toFixed(0) for MB ("5 MB"), while dialog.ts:229 uses .toFixed(1) ("5.0 MB"). The rejection toast and the dialog header would show the same file's size differently. Both functions are private and unexported — one of them should call the other, or the function should be extracted to a shared utility. Fix this →

5. Passthrough selectors copied in three places

IFRAME_PASSTHROUGH_SELECTORS is defined in manager.ts:66, copied verbatim into iframe-bridge-standalone.ts:607 as dropPassthroughSelectors, and likely again in the PHP chromeless bridge inline JS. A future addition (e.g. a new Gutenberg drop target) requires updating all three. The TS copies can't easily share a module (standalone bundle), but the comment on the constant in manager.ts says "Public surface — re-exported via the bridge protocol doc" — it's worth a // Keep in sync with iframe-bridge-standalone.ts and chromeless-bridge.php warning on all three.

6. No cap on file count

A user dragging a 200-file selection from Finder would get a 200-entry modal with no feedback. A defensive MAX_FILES constant (e.g. 20) with a toast for the excess would keep the dialog usable.

---

Internationalization

7. Hardcoded English strings throughout

Both manager.ts and dialog.ts contain user-facing copy that isn't passed through WordPress's __() / _n() localization functions. Examples: "Drop to upload", "is not an allowed file type.", "Upload to Media Library", "Uploading…", "Uploaded to Media Library.". If i18n is out of scope for this PR, a tracking issue would help — these strings will be very hard to find later.

---

Security (things done well)

• ✅ instanceof File validation on postMessage files before accepting them — correct, same-origin postMessage preserves File identity so this works and defends against crafted {type:'desktop-mode-os-file-drop', files:[{}]} messages.
• ✅ ev.origin !== window.location.origin guard before processing messages in both the shell and iframe sides.
• ✅ resolveWindowIdFromSource requiring the source frame to match a real <iframe> in the shell DOM before dispatching — correct defence-in-depth.
• ✅ sanitizeFilename is framed as a UX pre-fill only; the comment correctly defers to sanitize_file_name() server-side as canonical.
• ✅ PHP allows-mime list is user-scoped (get_allowed_mime_types( $user_id )) and capability-gated (upload_files).

---

Minor nitpick

dialog.ts:134 casts uploadBtn to { disabled: boolean } rather than HTMLElement & { disabled?: boolean }. The <wpd-button> component presumably exposes disabled as a reflected attribute — el.setAttribute('disabled', '') / el.removeAttribute('disabled') would be cleaner and avoid the cast.

---

Summary

The feature is architecturally sound and follows the codebase's conventions well. The four items I'd prioritize before merging are: the dead 'remove' listener (#1), the mid-file import (#3), the duplicate JSDoc losing @since (#2), and the formatBytes precision inconsistency (#4). The passthrough-selectors sync note (#5) and i18n (#7) are worth a follow-up issue but are not merge blockers.
·

[github-actions]

✅ 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