Skip to content

Add Featured Plugins View and Ribbon Component#225

Merged
AllTerrainDeveloper merged 11 commits into
trunkfrom
features-plugin
May 16, 2026
Merged

Add Featured Plugins View and Ribbon Component#225
AllTerrainDeveloper merged 11 commits into
trunkfrom
features-plugin

Conversation

@AllTerrainDeveloper

@AllTerrainDeveloper AllTerrainDeveloper commented May 15, 2026

Copy link
Copy Markdown
Collaborator
  • Implemented the Featured Plugins view in featured-view.ts, providing a curated gallery of plugins for Desktop Mode.
  • Introduced a new <wpd-ribbon> component for displaying diagonal corner banners, with customizable placement and tone.
  • Added styles for the <wpd-ribbon> component in wpd-ribbon.styles.ts.
  • Created tests for the <wpd-ribbon> component to ensure proper rendering and attribute handling.
  • Developed PHPUnit tests for the AJAX endpoint of the Featured tab, including caching and response validation.
Open WordPress Playground Preview

- Implemented the Featured Plugins view in `featured-view.ts`, providing a curated gallery of plugins for Desktop Mode.
- Introduced a new `<wpd-ribbon>` component for displaying diagonal corner banners, with customizable placement and tone.
- Added styles for the `<wpd-ribbon>` component in `wpd-ribbon.styles.ts`.
- Created tests for the `<wpd-ribbon>` component to ensure proper rendering and attribute handling.
- Developed PHPUnit tests for the AJAX endpoint of the Featured tab, including caching and response validation.
@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

@claude review this PR please =)

@claude

claude Bot commented May 15, 2026

Copy link
Copy Markdown

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


PR Review: Add Featured Plugins View and Ribbon Component

Tasks:

  • Gather context and understand the request
  • Read changed files and analyze the PR
  • Review <wpd-ribbon> component
  • Review featured-view.ts
  • Review PHP/AJAX changes
  • Review settings changes (dockPromotedPositions)
  • Review tests
  • Post comprehensive review feedback

This is a solid, well-structured PR that follows the project's established patterns throughout. Here's a detailed walkthrough.


<wpd-ribbon> Component

Overall: well done. The component follows the <wpd-*> pattern correctly — Component base class, defineComponent, shadow-DOM CSS, static help block, part="banner" exposed for ::part() overrides.

One notable CSS concern: the RTL rotation flip at wpd-ribbon.styles.ts:112–124 uses :host-context([dir='rtl']), which was deprecated in Chrome 122 and has never been supported in Firefox. The banner rotation won't flip for RTL users in Firefox — the inset-inline-* positioning will be correct, but the strip will lean the wrong direction. That said, wpd-flyout and wpd-rating-summary use the same pattern already, so this is consistent with the codebase rather than new technical debt. Worth an issue to track a :host([dir='rtl']) + @layer migration for the whole suite.

Tests: The vitest smoke tests cover all the important axes — default placement, attribute reflection, slot projection, part exposure. Good.

Fix this →


featured-view.ts

Overall: clean. The parallel Promise.all for the initial fetch (featured + installed) is the right call — avoids the "Install" button flash for already-installed plugins. The teardown pattern (unsubscribe + replaceChildren) is correct. Cross-tab sync via PLUGINS_CHANGED_TOPIC matches the other views.

describe function name at featured-view.ts:398 — this local error-formatter shares its name with the Vitest global describe. In production code this is harmless, but it's a small trap for anyone refactoring or copy-pasting. A name like formatError or describeErr would sidestep the ambiguity.

Fix this →

card.prepend(ribbon) at line 313 — relies on the <wpd-card> having position: relative set by the --featured CSS class. This is a fragile coupling — if someone removes the CSS class from the card but keeps the ribbon prepend, the ribbon anchors to a higher ancestor and floats over the wrong thing. A comment noting this dependency (or better, setting position: relative on the ribbon's parent programmatically) would help.


PHP/AJAX (ajax.php)

Security posture is correct: every handler calls desktop_mode_plugins_window_ajax_guard() which checks nonce + capability before anything else. install_plugins cap gate is appropriate.

info.discovered can go negative if some curated slugs fail hydration. At ajax.php:987:

'discovered' => count( $plugins ) - count( $curated ),

If plugins_api() returns a WP_Error for one of the curated slugs (e.g. a test slug, a plugin temporarily delisted from wp.org), count($curated) can exceed count($plugins), producing a negative value. This is a display-only issue (the JS uses it for informational headers), but it would be confusing in debug. A max(0, …) guard is cheap:

'discovered' => max( 0, count( $plugins ) - count( $curated ) ),

Fix this →

Cache key is static ('dm_pwfeatured_v1'). If a site uses desktop_mode_plugins_featured_slugs to return user-role-specific slugs (e.g., show a "premium" plugin only to admins), the single shared transient would leak the first caller's results to all others. Worth a doc note on the filter that the response is shared across all users who can view the Featured tab.

query_plugins fetching 100 popular plugins every cache miss is acceptable given the 1h TTL, but a wp.org outage or slow response during a cache miss will hold the Featured tab in skeleton-state for the full wp_remote_get timeout. Consider falling back to the curated-only payload if the discovery call fails (currently is_wp_error($discovered) is checked, so the curated rows still land — good — but the info.discovered field would be 0 and there's no indication to the user that discovery was partial).


Settings: dockPromotedPositions

Solid implementation. The PHP sanitizer at os-settings.php:462–498 is thorough: type-checks, sanitize_key on keys, coordinate bounds, 256-entry cap, cast to int. This mirrors the TypeScript sanitizer in state.ts which is good for defence-in-depth.

The persistDockPromotedPosition function in layer.ts:844 correctly reads the existing map and merges rather than overwrites, which prevents a drag of one icon from wiping another icon's saved position.

One minor observation: persistDockPromotedPosition casts coordinates from the cell object which stores CSS-pixel values. The sanitizeDockPromotedPositions function in PHP caps coords at 100_000 and the TS mirror does the same. Since real screen sizes don't approach that, this is fine.


docs/hooks-reference.md

The two new filters (desktop_mode_plugins_featured_slugs and desktop_mode_plugins_featured_response) are documented with signatures and examples — good. Both marked Experimental which is appropriate for a first release.

Missing: <wpd-ribbon> in docs/examples/layout-primitives.md. Per CLAUDE.md's rules:

Reference it in docs/components-reference.md if it's user-facing.

docs/components-reference.md doesn't exist, but docs/examples/layout-primitives.md is the natural home since ribbon is a layout decoration primitive. Adding a section there with the placement/tone matrix and the parent-must-be-positioned caveat would help plugin authors discover it.

Fix this →


PHP Template (window.php)

The new Featured tab label is esc_html_e( 'Desktop Mode plugins', 'desktop-mode' ). This is self-referential — users are already inside Desktop Mode. "Featured" or "For Desktop Mode" would be more natural tab labels and align with how the browse tab says "Browse" not "Browse WordPress plugins". Minor UX note.


Tests

PHPUnit (pluginsWindowFeaturedAjax.php): Comprehensive. The mock strategy (filter plugins_api to avoid real wp.org calls) is correct. Tests cover: auth gate (subscriber rejected), nonce gate, happy path (curated + discovered), dedup, caching, and the response filter. The dispatch_featured() helper cleanly isolates the AJAX machinery.

One observation: test_response_is_cached_for_subsequent_calls validates that _last_response is reused but swaps the mock between calls. This correctly proves the transient stuck rather than just checking for identical values by coincidence. Good test design.

No vitest coverage for featured-view.ts — the async load, skeleton painting, error state, and cross-tab sync are all exercised only in manual testing. Given the PHPUnit side is well-covered, this is a reasonable scope boundary for the PR, but worth a follow-up.


Summary

Area Verdict
<wpd-ribbon> component ✅ Well-structured; minor RTL concern consistent with existing codebase
featured-view.ts ✅ Clean; rename describeformatError
PHP/AJAX (ajax.php) ✅ Good security posture; fix info.discovered negative edge case
dockPromotedPositions ✅ Solid implementation, PHP + TS sanitizers in sync
Documentation ⚠️ <wpd-ribbon> missing from layout-primitives.md per CLAUDE.md policy
PHP tests ✅ Well-scoped, correct mocking strategy
TS tests ✅ Covers component contract; no featured-view.ts coverage (acceptable scope)

Three things worth fixing before merge:

  1. describeformatError in featured-view.ts
  2. max(0, ...) guard on info.discovered in ajax.php
  3. <wpd-ribbon> entry in docs/examples/layout-primitives.md

Everything else is polish. The Featured tab architecture (curated seed + requires_plugins scan + 1h cache + PHP filters) is the right approach given wp.org's current API limitations.

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

✅ 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

- Rename local `describe()` helper to `formatError()` in
  `src/plugins-window/featured-view.ts` so it stops shadowing the
  Vitest global `describe`, which was a copy-paste trap. Other
  plugins-window files (browse-view, installed-view, flyout-detail)
  still carry the same pattern but pre-date this PR and are out of
  scope.

- Wrap `count($plugins) - count($curated)` with `max( 0, ... )` in the
  Featured-tab AJAX handler so the `info.discovered` counter never
  underflows when a curated slug fails to hydrate (typo, plugin
  delisted from wp.org, plugins_api returning WP_Error).

- Document `<wpd-ribbon>` in `docs/examples/layout-primitives.md`
  with the placement matrix, parent-must-be-positioned caveat, the
  full CSS-variable surface, the `::part(banner)` extension point,
  and an accessibility note about decorative-only role.

- Note the shared-transient caveat on the
  `desktop_mode_plugins_featured_slugs` filter docstring — returning
  per-user or per-role slugs leaks across viewers via the single
  site-wide `dm_pwfeatured_v1` cache key.

- Add a comment on the featured-card branch in `featured-view.ts`
  pairing the `<wpd-ribbon>` prepend with the
  `__card--featured` class so future edits can't quietly remove
  the `position: relative` and re-anchor the ribbon to an unrelated
  ancestor.

Verified clean on the merged tree: tsc, eslint, vitest 1285/1285,
PHPUnit featured-ajax 9/9, full plugin build.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AllTerrainDeveloper AllTerrainDeveloper enabled auto-merge (squash) May 15, 2026 21:51
AllTerrainDeveloper and others added 7 commits May 16, 2026 00:01
Two structural-integrity bugs in the desktop-files drop layer that
could leave a user stranded outside the desktop tree.

Recycle bin self-trash
----------------------
The bin's wallpaper tile is both a drag source (every files-layer
tile is) and a drop target (the bin accepts trash gestures). Without
a guard, dragging the bin onto itself called `trashByFileType` on
the bin's own placement → REST DELETE → bin disappeared from the
desktop. The orphan-backfill re-placed it on the next page load,
but the in-session UX was alarming.

Fix in `src/desktop-files/recycle-bin-targets.ts`: short-circuit
`accept()` when the dragged placement's `file.ref` is the recycle
bin's window id. Same hook covers both the wallpaper tile drop
target and the bin-window-body drop target.

Folder cycle (parent ↔ descendant)
----------------------------------
The same-folder case (drop X onto X's own tile, or drop within the
folder X is already in) was guarded in `registerFolderDropTarget`.
But there was no guard for the descendant case — drag folder X into
folder Y when Y sits anywhere inside X's subtree. The REST PATCH
happily set `X.parent_id = Y` while `Y.parent_id` still led back
through `X`, producing an unreachable cycle that stranded every
descendant outside the root.

Authoritative fix server-side in `includes/desktop-files/store.php`:

  - New `desktop_mode_files_would_create_folder_cycle( $user_id,
    $moving_folder_id, $target_parent_id )` walks the parentage
    chain upward from the target. Returns true if it reaches the
    moving folder OR re-visits a node (pre-existing cycle in the
    data). Hard 256-depth cap as a defense against pathological
    trees.
  - `desktop_mode_files_move` calls it whenever a folder placement
    has its `parent_id` changed; on cycle, returns
    `WP_Error( 'desktop_mode_files_folder_cycle', …, 409 )`.

Client-side preflight in `src/desktop-files/layer.ts`:

  - `wouldCreateFolderCycle( movingFolderId, targetParentId )`
    mirrors the server walk, reading the live files-store. Same
    256-depth cap, same pre-existing-cycle safety.
  - Wired into both `canvasDropTarget.accept` (folder-window canvas
    drops) and `registerFolderDropTarget`'s `accept` (folder-tile
    drops). Rejection at `accept()` time gives instant snap-back
    feedback instead of a 409 toast after the REST hit.

Tests
-----

PHP (5 new in `tests/phpunit/tests/desktopFilesStore.php`):
- Cycle detector flags self target.
- Cycle detector flags descendant target (X→Y→Z, move X into Z).
- Cycle detector allows unrelated targets.
- Cycle detector allows move-to-root (target_parent = 0).
- `desktop_mode_files_move` returns the 409 WP_Error AND leaves
  the row's parent_id untouched when a cycle is detected.
- `desktop_mode_files_move` rejects "folder into itself."

JS (1 new in `tests/vitest/desktop-files-bin-drop.test.ts`):
- Dragging the recycle bin onto itself is rejected: no
  `data-desktop-mode-trash-drop-active` highlight, no `onCommit`,
  no DELETE issued, bin placement still in the store afterward.

Verified clean: tsc, eslint, vitest 1286/1286, PHPUnit
desktop-files-store 25/25 + bin-drop suite, full plugin build.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: dropping a desktop icon "always tends to be down to the
right" of the intended spot. Two complementary fixes.

Precision: subtract the ghost grab offset before snapping
---------------------------------------------------------
The canvas drop handler in `layer.ts` snapped to a grid cell based on
`(ev.clientX, ev.clientY)` — the cursor position. But the user grabs
a tile somewhere inside it (typically the center, ~44/48 px in from
the top-left), and the drag manager renders the ghost offset to match:

    ghost.topLeft = (cursor.x - offsetX, cursor.y - offsetY)

So the user SEES the ghost at the offset position, but the drop math
was snapping the CURSOR position. Result: every drop landed
~offset-amount down-and-right of the visible ghost. Bias exactly
equal to the grab offset — which is why the user's "down-right"
description was so consistent.

Fix: subtract `ghost.offsetX / offsetY` from the raw client coords
before the `snapToEmptyCell` call. Now the snap input is the tile's
intended top-left, matching what the user sees under the cursor.

Live preview overlay
--------------------
Even with the math fix, grid snapping skips a column when the cursor
lands right on a cell boundary. To remove that surprise, the canvas
drop target now renders a soft dashed outline at the cell the drop
WILL land in — updated on every `pointermove` while the canvas is
the active hover target, primed from the source tile's bounding rect
so it's visible from the first frame.

Implementation notes:

- One outline per layer, mounted inside the layer container so it
  lives in the same coordinate space as tiles. CSS-positioned via
  `translate3d` so updates are GPU-cheap.
- Tear-down tied to `onLeave` / `onDrop`. No leak — the listener
  detaches with the preview.
- Only fires for `desktop-file` payloads. Shortcut creates use
  row-major packing in `built-in-openers`, so a cursor-tracking
  preview would lie about the landing slot for them.
- Respects `prefers-reduced-motion` (no transition).

Files
-----
- `src/desktop-files/layer.ts` — ghost-offset math + preview install
  / teardown.
- `assets/css/desktop-files.css` — `.desktop-mode-files-drop-preview`
  styles with theming hooks (`--desktop-mode-drop-preview-bg`,
  `--desktop-mode-drop-preview-border`).

Verified: tsc, eslint, vitest 1286/1286 (existing drag tests still
pass — the snap math change is observable behaviorally but the JSDOM
ghost rect harness happens to align with the new math). Full plugin
build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: dragging a tile within the desktop pops a toast that
reads "admin moved this to 'another folder'" — most frequently
inside shared folders. The actor is the user themselves; nothing
external is moving the row. So the conflict is a self-409.

Root cause
----------
The fast-path repaint (`tryPatchPositions`, added 0.20.0 to fix the
"whole desktop reloads on drop" visual flash) keeps tile DOM
identity. That's the right call for the flash, but it has a side
effect: the pointerdown listener `attachTileDrag` installed only on
the wholesale-rebuild branch never re-runs, so the `placement`
captured in its closure is whatever the store held the LAST time
the layer fully repainted.

After a move, the canvas drop fires
`rest.updatePlacement( id, { … }, placement.updatedAtMs )`. That
third arg becomes the `If-Match` header. Once the server commits and
returns a fresh `updatedAtMs`, the heartbeat in shared folders
broadcasts the new value (`upsertPlacement( …, 'remote' )`). Local
store is fresh; the tile's drag closure isn't. The user's NEXT drag
of the same tile ships the old `If-Match` → server returns 409 with
`reason: 'parent_changed'` → `showConflictToast` paints "admin moved
this to 'another folder'."

Shared folders see this most because the heartbeat bumps
`updatedAtMs` on every active-peer tick, not just on mutations —
so the closure goes stale within seconds, no other-user interaction
required.

Fix
---
In `attachTileDrag`'s pointerdown handler, re-read the placement
from the live store by id before calling `dragManager.start`. The
closure-captured `placement` is now only used as a fallback when
the store no longer has the row (defensive — shouldn't happen in
practice). Every subsequent field the drag pipeline reads
(`updatedAtMs`, `parentId`, `meta.__synthFromDockItem`, file shape)
comes from the live row, so If-Match, synth detection, and cycle
detection all agree with the server's view.

Test (`tests/vitest/desktop-files-drag.test.ts`)
------------------------------------------------
- Mount layer with placement id=1, updatedAtMs=1.
- Apply a remote bump: `store.upsertPlacement({ …, updatedAtMs: 9999 }, 'remote')`.
- Verify same DOM node still in place (fast-path took the repaint).
- Spy on `dragManager.start`; fire pointerdown.
- Assert `start` was called with `data.placement.updatedAtMs === 9999`.

Suite: 1287/1287 vitest, tsc + eslint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: in a column rendered as

    My WordPress | empty cell | Icon A | Icon B

dragging Icon B onto the empty cell highlighted (and dropped onto)
the My WordPress slot, not the empty one. "It only happens with My
WordPress."

Root cause
----------
The layer's repaint reserves pinned-tile slots up the column 0 axis
(rows 0 .. N-1 in pinned-order), ignoring whatever `(x, y)` the
placement actually carries in the DB. That's deliberate — pinned
icons are framework anchors, not user-positioned tiles.

But `grid.ts`'s `buildOccupiedSet` only reads stored `(x, y)`. So
the pinned tile's VISUAL cell wasn't in the occupied set. With My
WordPress pinned at (0, 0), the canvas drop's `snapToEmptyCell`
saw column 0 row 0 as free and either:

  1. Drop math: when the cursor's target cell was occupied
     (anywhere else in column 0), the column-major scan picked
     (0, 0) as the first "empty" slot — landing the dropped tile
     visually on top of My WordPress.
  2. Drop preview: the same `snapToEmptyCell` call painted the
     preview outline at (0, 0).

Fix
---
New `buildVisualOccupiedSet( placements, excludeId? )` in `layer.ts`
that mirrors the repaint's allocation: sort pinned-first, hand each
pinned tile (col 0, row pinnedIdx) regardless of stored coords,
and only fall back to `pointToCell( p.x, p.y )` for non-pinned
rows. Same iteration order as the repaint (stable sort) so the
pinned-slot indices line up exactly with what's on screen, even
when multiple pinned tiles exist (today only My WordPress, but
the flag is generic).

All four `buildOccupiedSet` callsites in `layer.ts` now route
through the visual variant:

  - Live drop-preview update (`installCanvasDropPreview`)
  - Canvas drop's `snapToEmptyCell` for desktop-file payloads
  - Canvas drop's `nextRowMajorCell` for shortcut creates
  - Folder-tile drop's `nextRowMajorCell` for shortcut creates

The plain `buildOccupiedSet` import is dropped — no remaining
callers, and the visual variant is correct for every callsite.

Test
----
New regression in `tests/vitest/desktop-files-drag.test.ts`:

  - Mount a folder with [pinned My WordPress, Icon A, Icon B] —
    My WordPress's stored coords are deliberately wild (9999, 9999)
    to prove the test exercises the bug (the layer ignores them
    and slots it at (0, 0) regardless).
  - Drag Icon B's tile from (16, 346) to (16, 126).
  - Assert the PATCH body's `(x, y)` is (16, 126), i.e. cell
    (0, 1), NOT (16, 16) = (0, 0) = My WordPress's slot.

Suite: vitest 1288/1288, tsc + eslint + build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: "we can't drop anything into MY WordPress app..."

Before this change, dragging a desktop tile (or any payload) onto
the My WordPress window did nothing visible. The drag manager's
hit-test walks UP from the cursor's element to the first registered
drop target, stopping at the window boundary. My WordPress never
registered a target, so the walk bailed at `.desktop-mode-window`
and the manager `cancel(session, 'no-target')`ed silently — the
ghost just snapped back with no feedback to explain why.

Fix: My WordPress now registers a CLAIMANT drop target on its
window body whose `accept` is `() => false`. The hit-test stops at
this claimant before reaching the window boundary, so the manager
paints the no-drop cursor + reject snap-back animation. Users get
visible "this surface doesn't take drops" feedback instead of
guessing whether their drop registered.

Why a no-op claimant rather than a meaningful action: My WordPress
is a read-only directory listing over the WP REST API. Dropping a
desktop tile, a placement, or a shortcut onto it has no defined
semantic — posts already exist in WordPress; the directory listing
shows them by definition. If a future story asks for "drop a tile
onto My WordPress to route to its detail dossier" (the right-click
'Navigate into' equivalent), this claimant is the natural place to
plug it in — swap `accept: () => false` for `accept: payload =>
payload.type === 'desktop-file' && payload.data.placement.file.type
=== 'post'` and add an `onDrop` that calls the existing
`wp.desktop.myWordpress.openDetail` API.

The claimant also blocks the drag from falling through to the
wallpaper canvas under the window — drop registry walks stop at
the first registered target, so a tile released over My WordPress
no longer accidentally creates a placement on the desktop below.

Implementation
--------------
In `src/my-wordpress/index.ts`'s `renderInto`, register the target
right after `activeState = state` and push the deregister into
`state.teardown` so it's cleaned up by the existing
`desktop-mode-window-closed` handler. The drag manager isn't
guaranteed to exist in headless test boot, so the registration is
gated on `getDragManager()`.

Verified: tsc, eslint, vitest 1288/1288, plugin build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User flagged: "show the label 'CANT DROP HERE' as we show in other
cases." The reject-claimant from 82692f7 already triggers the
framework's default reject chip text — proven now by an explicit
regression so a future edit can't silently regress the contract.

The test fakes the My WordPress window shape
(`.desktop-mode-window` wrapping `.desktop-mode-window__body`),
registers the same `accept: () => false` claimant the production
code wires up, starts a desktop-file drag, and asserts after the
hover:

- The hint chip is mounted on `document.body`.
- It carries `desktop-mode-drag-hint--reject` for the red styling.
- Its text is exactly "Can't drop here" (with the framework's
  curly apostrophe).

Note for testers: the chip is a runtime ghost element appended to
`document.body` — only visible AFTER `npm run build` has produced
the latest bundles. If the chip doesn't appear when dragging onto
My WordPress in a manual session, rebuild and reload before
reporting the bug.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: "i can still drag something over the MY WORDPRESS app
and still showing the green badge, i want the Can't drop here. Not
the drop here to move..."

I previously read this as the OPEN My WordPress WINDOW (and added a
reject claimant there in 82692f7 — which works, locked by
caff08c). But the user meant the My WordPress TILE on the
wallpaper grid. Different surface, same problem shape.

Root cause
----------
The wallpaper canvas drop target lives on `#desktop-mode-area` and
accepts every `desktop-file` / `shortcut` payload. Each tile sits
INSIDE that area. When the user drags a tile over another tile, the
manager's hit-test walks UP from the cursor's element looking for a
registered drop target, stopping at `.desktop-mode-window`. For:

  - Folder tiles: `registerFolderDropTarget` claims with
    `accept: payload-type-aware`. ✓
  - Recycle Bin: `recycle-bin-targets.ts` claims with the trash
    handler. ✓
  - **Everything else** (My WordPress, dock promotions, plugin-
    registered icons, post/page shortcuts): NO claimant. The walk
    falls through to the canvas drop target → `accept: true` →
    chip says "Drop here to move" (green).

But the drop math uses `snapToEmptyCell` on the cursor coordinates
with the visual-occupied set (now including pinned slots after
ddd4163). The occupied cell pushes the snap to the next free
cell — so the drop visually lands in a different position than
the chip implied. Green chip, divergent outcome.

Fix
---
Layer.ts now registers a per-tile reject claimant for every
non-folder, non-bin tile. New helper `shouldRejectTileDrops`
gates the registration:

  - `placement.file.type === 'folder'` → false (folder claim
    already handles it).
  - `placement.file.ref === 'desktop-mode-recycle-bin'` → false
    (the registry keys by element; a reject claimant here would
    overwrite the bin's trash target and silently break the bin).
  - Everything else → true.

The reject claimant sits in a new `tileRejectDeregisters: Map<
number, () => void >` so the wholesale-rebuild cleanup loop AND
the layer-level `dispose()` both tear it down idempotently. The
fast-path repaint preserves tile DOM identity, so a moved tile's
claimant survives without re-registration.

Registration happens in both the pinned-branch and the regular
branch of the tile-build loop, so My WordPress (pinned) AND
plugin-registered icons (non-pinned shortcuts) both pick up the
rejection.

Extension hook for plugins
--------------------------
Plugin authors who register an icon via `desktop_mode_register_icon`
and WANT to accept drops can install their own drop target on the
tile element from a `desktop-mode.files.tile-rendered` listener.
The registry overwrites by element, so their target wins over the
layer's default reject claim.

Tests
-----
Two new vitest cases in `desktop-files-drag.test.ts`:

  - "hovering a non-folder tile during a drag flips the chip to
    reject" — mounts a layer with My WordPress (pinned) + Icon B,
    walks the manager's debug target list, asserts the
    `desktop-mode-files-tile-100-reject` target is registered on
    the My WordPress tile element and that its `accept()` returns
    false for a desktop-file payload.
  - "recycle-bin tile is NOT reject-claimed — its trash target
    survives" — mirror of the above, asserting the layer does NOT
    install a reject claimant on the bin so
    `recycle-bin-targets.ts`'s trash-accepting registration wins.

Suite: vitest 1291/1291, tsc + eslint + plugin build clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

@claude one last round? ;)

@claude

claude Bot commented May 16, 2026

Copy link
Copy Markdown

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


PR Review — Round 2

Tasks:

  • Gather context and understand the request
  • Check if previous round's 3 issues were addressed (commit 16bc3f0)
  • Fresh scan of the two new commits for any remaining concerns
  • Post final review

Round 1 follow-up — all three items addressed

Commit 16bc3f0 ("Address PR review: ribbon docs, info.discovered guard, describe rename") closes every issue from round 1, plus two bonus items that weren't requested:

Item Status
describeformatError rename ✅ Done; all three call sites updated
max(0, …) on info.discovered ✅ Done with a clear explanatory comment
<wpd-ribbon> in layout-primitives.md ✅ Done — placement matrix, full CSS-variable table, ::part(banner) extension point, accessibility note
Shared-transient caveat on desktop_mode_plugins_featured_slugs ✅ Bonus — good addition to the hooks-reference
card.prepend(ribbon) coupling comment ✅ Bonus — future editors are warned not to drop the __card--featured class

New commit review — bdd0d8f

The second new commit lands two structural-integrity bug fixes for the desktop-files layer:

Recycle bin self-trash guard (recycle-bin-targets.ts:119): Clean single-line early-return in accept() that short-circuits when the dragged placement's file.ref equals RECYCLE_BIN_WINDOW_ID. RECYCLE_BIN_WINDOW_ID is a module-level constant — not a hardcoded string at the call site — so if the id ever changes there's a single place to update. The JS test covers all the right assertions: no data-desktop-mode-trash-drop-active, no onCommit, no DELETE, placement still in store.

Folder cycle detection: Client-side wouldCreateFolderCycle in layer.ts mirrors the server-side desktop_mode_files_would_create_folder_cycle in store.php. Both walk the parentage chain upward with a 256-depth hard cap and a visited-set guard against pre-existing cycles. Both return false immediately when targetParentId <= 0 (desktop root can never form a cycle). The PHP function runs up to 256 individual get_var queries in the worst case — fine for real trees (depth rarely exceeds 5–6) and the authoritative server-side rejection means a slow client can't commit damage even if it somehow skips the preflight.

Tests: 6 PHPUnit cases cover self-target, descendant target, unrelated target, move-to-root, desktop_mode_files_move 409 rejection, and a "folder into itself" variant. 1 Vitest case for the self-trash guard. Coverage matches the scope of the changes.

One minor observation (nothing to block merge): the PHP sanitize_key normalizes uppercase/spaces in dockPromotedPositions map keys (e.g. 'Edit Php''editphp'), while the TS sanitizeDockPromotedPositions only type-checks and rejects empty strings — no case normalization. In practice this is harmless because the TS reads a value already sanitized by PHP, and dock item IDs are always slug-shaped. Worth being aware of if someone ever writes a getPositions() caller that sources IDs from free-form user input.


Summary

Everything from round 1 is cleanly resolved. The two new bug fixes are well-implemented, correctly tested, and follow the existing patterns. The documentation is thorough. This is ready to merge.

@AllTerrainDeveloper

Copy link
Copy Markdown
Collaborator Author

@claude mistakenly I left code not pushed in my local, please review from 61f49ef

@claude

claude Bot commented May 16, 2026

Copy link
Copy Markdown

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


PR Review — Round 3 (from 61f49ef)

Tasks:

  • Gather context and understand the request
  • Check git log from 61f49ef onwards — 5 new commits
  • Read new/changed files (layer.ts, my-wordpress/index.ts, test files)
  • Post comprehensive review

Five commits to review: fd7cd3f, ddd4163, 82692f7, caff08c, 56b6e9e. They form a coherent cluster fixing drag UX: stale-closure placement → wrong If-Match on PATCH; pinned slot missing from occupied set → drops landing on pinned cells; missing reject claimants → green chip where the drop would silently snap away.


fd7cd3f — drag reads live placement from store, not stale closure

The fix is correct and the regression description in the commit/test is precise: fast-path repaints reuse tile DOM without re-attaching drag handlers, so the closure-captured placement drifts away from the store's truth after the first heartbeat bump. The ?? placement fallback for a deleted-in-flight placement is the right safety net. No issues.


ddd4163 — include pinned tile slots in drop occupancy

buildVisualOccupiedSet correctly mirrors the repaint's sorting logic (pinned-first, stable within group) and allocates (0, pinnedIdx) rather than the stored (x, y). The stable-sort comment is accurate. The test pins myWp at stored coords (9999, 9999) to prove the layer ignores those — this is exactly the right way to exercise the bug.

One observation: buildVisualOccupiedSet hardcodes column 0 for all pinned tiles. This is consistent with the repaint invariant documented in the JSDoc, but the function would produce wrong results silently if that invariant ever changes. The JSDoc explains it clearly enough. No action needed.


82692f7 + caff08c — My WordPress window body reject claimant

Clean integration with the existing state.teardown array. The test correctly pins the chip text ("Can't drop here") as a behavioral contract of the reject claim.


56b6e9e — reject-claim non-folder tiles

One bug in the documented escape hatch.

The shouldRejectTileDrops JSDoc says:

Plugins that register icons via desktop_mode_register_icon and WANT to accept drops can register their own drop target on the tile element from a desktop-mode.files.tile-rendered listener; the registry overwrites by element, so their target wins.

But desktop-mode.files.tile-rendered fires inside buildTile (file-tile.ts:168) — before buildTile returns. Back in layer.ts, the reject claimant is registered after buildTile returns (line 327 / line 382). Since registerDropTarget overwrites by element and the reject claimant is last, the reject claimant wins, not the plugin's target. The escape hatch is documented but broken.

Fix options (in order of preference):

  1. Correct the comment to remove the false claim. Note that tile-rendered fires too early. If the escape hatch is a real requirement, add a desktop-mode.files.tile-drop-registered action AFTER the reject claimant registers, so plugins can listen there.
  2. Register the reject claimant BEFORE tile-rendered fires, so plugins can overwrite it — but this is tricky since you need the tile reference (returned by buildTile) to register.

Since no plugin currently uses this path and the feature is 0.20.0 / Experimental, fixing the comment is the lowest-risk option.

Fix the comment →


Minor — RECYCLE_BIN_REF duplication.

layer.ts:1024 defines const RECYCLE_BIN_REF = 'desktop-mode-recycle-bin' while recycle-bin-targets.ts:41 defines const RECYCLE_BIN_WINDOW_ID = 'desktop-mode-recycle-bin' — same string, two constants. Exporting across those files risks the side-effect-import problem called out in AGENTS.md (the recycle-bin module carries a poller, leader, badge). A shared leaf-level src/desktop-files/constants.ts would be cleaner, or at minimum a cross-reference comment so if one ever changes the editor knows to update the other. Not a blocker.


Tests

All four scenarios are covered with regression-focused test names and explicit comment blocks explaining what the bug was and how to reproduce it. The live-placement test using vi.spyOn(manager, 'start') and asserting updatedAtMs === 9999 without needing to complete the drag is the right scope for a unit test.


Summary

Commit Verdict
fd7cd3f — stale closure fix ✅ Correct
ddd4163 — pinned-slot occupancy ✅ Correct
82692f7/caff08c — My WordPress reject ✅ Correct
56b6e9e — per-tile reject claimants ⚠️ One doc bug: escape hatch in JSDoc is incorrect (ordering)

One thing worth fixing before merge: the shouldRejectTileDrops JSDoc documents an escape hatch (tile-rendered listener wins) that doesn't work due to registration ordering. The fix is a one-line comment correction. Everything else is solid.

…Drops

Reviewer caught a doc lie. The JSDoc said plugins could win by
registering their own target from a `desktop-mode.files.tile-rendered`
listener, "since the registry overwrites by element." That's
technically true of the registry but the registration ORDER is
wrong: `tile-rendered` fires from inside `buildTile()`
(file-tile.ts:168) BEFORE `buildTile` returns. Back in layer.ts the
reject claimant gets registered AFTER `buildTile` returns. So the
claimant always wins, not the plugin's target. The hatch was
documented but broken.

Going with the lowest-risk fix (the reviewer's preferred option):
just correct the comment. No in-tree caller depends on the hatch
today and the surface is 0.20.0 / Experimental, so I'd rather not
add a public action speculatively. The new wording:

  - States the actual order: tile-rendered → buildTile returns →
    layer reject claimant.
  - Names the right fix if a real plugin needs the hatch: a new
    action (e.g. `desktop-mode.files.tile-drop-registered`) fired
    AFTER the layer's registration so plugins can install their
    own target last.
  - Says explicitly we're not adding that action speculatively.

No code change, no test change — pure comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AllTerrainDeveloper AllTerrainDeveloper enabled auto-merge (squash) May 16, 2026 12:01
@AllTerrainDeveloper AllTerrainDeveloper merged commit 3f0c284 into trunk May 16, 2026
5 checks passed
@AllTerrainDeveloper AllTerrainDeveloper deleted the features-plugin branch May 16, 2026 12:04
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