Add Featured Plugins View and Ribbon Component#225
Conversation
- 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.
|
@claude review this PR please =) |
|
Claude finished @AllTerrainDeveloper's task in 3m 56s —— View job PR Review: Add Featured Plugins View and Ribbon ComponentTasks:
This is a solid, well-structured PR that follows the project's established patterns throughout. Here's a detailed walkthrough.
|
| Area | Verdict |
|---|---|
<wpd-ribbon> component |
✅ Well-structured; minor RTL concern consistent with existing codebase |
featured-view.ts |
✅ Clean; rename describe → formatError |
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:
describe→formatErrorinfeatured-view.tsmax(0, ...)guard oninfo.discoveredinajax.php<wpd-ribbon>entry indocs/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.
✅ WordPress Plugin Check Report
📊 ReportAll 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>
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>
|
@claude one last round? ;) |
|
Claude finished @AllTerrainDeveloper's task in 3m 0s —— View job PR Review — Round 2Tasks:
Round 1 follow-up — all three items addressedCommit
New commit review —
|
|
Claude finished @AllTerrainDeveloper's task in 3m 27s —— View job PR Review — Round 3 (from
|
| Commit | Verdict |
|---|---|
fd7cd3f — stale closure fix |
✅ Correct |
ddd4163 — pinned-slot occupancy |
✅ Correct |
82692f7/caff08c — My WordPress reject |
✅ Correct |
56b6e9e — per-tile reject claimants |
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>
featured-view.ts, providing a curated gallery of plugins for Desktop Mode.<wpd-ribbon>component for displaying diagonal corner banners, with customizable placement and tone.<wpd-ribbon>component inwpd-ribbon.styles.ts.<wpd-ribbon>component to ensure proper rendering and attribute handling.