Skip to content

Fix placement, gear/Help, and dock⇄desktop visibility bugs#300

Merged
AllTerrainDeveloper merged 2 commits into
trunkfrom
fix/placement-gearhelp-visibility-bugs
Jun 7, 2026
Merged

Fix placement, gear/Help, and dock⇄desktop visibility bugs#300
AllTerrainDeveloper merged 2 commits into
trunkfrom
fix/placement-gearhelp-visibility-bugs

Conversation

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator

@AllTerrainDeveloper AllTerrainDeveloper commented Jun 7, 2026

Summary

Fixes a cluster of app-placement, screen-meta, and item-visibility bugs surfaced by a behavioral audit. Two were user-reported (My WordPress missing from the wallpaper; gear/Help buttons showing with nothing behind them); the rest are adjacent correctness issues found while tracing those paths. Every finding was reproduced in code and adversarially verified before fixing.

No public-API breakage. The only documented surface touched is the desktop-mode-screen-meta bridge message, whose change is backwards-compatible (an empty panels: [] was already a valid payload) — docs/javascript-reference.md is updated in the same PR.

Why

  • My WordPress vanished from the desktop on some installs. Root cause: the Spatial desktop layout silently dropped all server-registered desktop icons. The only recovery was OS Settings → Apps → move it to the dock.
  • Gear / Help title-bar buttons appeared with nothing to show. Two causes: the buttons were never cleared when an iframe navigated in place to a page with no screen meta, and detection keyed on element presence rather than panel content.
  • "Hide" on a dock/desktop tile didn't hide it for single-rail items — it teleported the tile to the other rail.

Changes

🔴 High

  • Spatial layout no longer drops the pinned My WordPress iconsrc/desktop-layout.ts
    repaintIcons()'s spatial branch now keeps server icons that are either pinned (framework-owned, e.g. My WordPress) or carry an explicit desktop/both placement override. Non-pinned plugin icons with no override stay suppressed (the deliberate "core surface" design). This also fixes the OS Settings "On the desktop" picker silently no-opping in Spatial.

  • Right-click "Hide" stops teleporting itemssrc/item-visibility-menu.ts
    "Hide from dock/desktop" now derives the target from the item's current placement via a new computeHideTarget() (both → the other rail; single-rail → hidden) instead of unconditionally writing the opposite rail. Native rail is derived from the rail-synthesis id prefix.

🟠 Medium

  • Stale gear/Help cleared on navigationsrc/iframe-bridge-standalone.ts, includes/render/chromeless-bridge.php
    Both bridges now always announce desktop-mode-screen-meta (including an empty panels: []) so the parent removes stale buttons when a page exposes no screen meta after an in-place same-slug navigation.

  • Visibility menu is state-awaresrc/item-visibility-menu.ts
    "Also show on <rail>" is hidden when the item is already on both (it was a no-op re-write).

  • OS Settings → Apps & Icons repaints livesrc/settings/sections/apps-icons.ts
    The tab subscribes to wp.desktop.subscribeOsSettings and repaints on external writes (e.g. a right-click visibility change), self-unsubscribing once the panel is torn down. Previously the row dropdown showed stale placement until the panel was rebuilt.

  • Reset no longer corrupts the DEFAULTS singletonsrc/settings/state.ts, src/settings/panel.ts
    structuredDefaults() now deep-clones the collection fields (itemVisibility, dockOrder, dockPromotedPositions), and onReset uses it instead of a shallow { ...DEFAULTS }. Fixes the custom-gradient editor mutating the module-level DEFAULTS for the rest of the session after a Reset.

  • dockOrder survives the PHP round-tripincludes/os-settings.php
    Cross-rail tiles persist with a rail-synthesis prefix (desktop:<id> / dock:<id>). sanitize_key() stripped the colon, silently resetting the user's order (and risking an id collision) on reload. The dockOrder sanitizer now preserves the colon while still rejecting anything outside the JS id charset. (itemVisibility / dockPromotedPositions keys are canonical and unaffected.)

🟡 Low

  • Content-less Screen Options / Help no longer surface a dead button — both bridges
    A panel is announced only when its toggle link is present and the panel has renderable content (form controls for Screen Options; non-empty tab/sidebar text for Help).

  • Contract drift correctedsrc/settings/types.ts, docs/javascript-reference.md
    The itemVisibility docblock wrongly claimed 'both' is never persisted; it is (PHP whitelists it, the menu stores it). Doc updated, plus the empty-array screen-meta semantics.

  • dockPromotedPositions no longer leakssrc/settings/desktop-shortcuts-sync.ts
    When a promoted item is demoted/hidden, its persisted drag position is pruned (reentrancy-safe), so stale coordinates can't accumulate toward the 256-entry cap or silently resurrect on a future re-promote.

Investigated and dismissed (no change): the "first-run seeding / absent key resolves to hidden" theory. resolvePlacement correctly falls back to the native rail for an unset key, so fresh and existing accounts behave identically — the real My WordPress cause was the Spatial branch above.

Testing

  • npm run typecheck
  • npm run lint
  • npm run test:js ✓ — 1548 passed (added spatial-layout coverage: pinned icon kept, explicit promotion kept, non-pinned/no-override still dropped)
  • npm run build
  • php -l ✓ on both changed PHP files
  • npm run test:php ✓ — 926 tests / 2175 assertions (added dockOrder colon-preservation + normalization tests)

Notes for reviewers

  • Spatial design decision (Uploading demo video  #1): I kept the deliberate suppression of non-pinned, no-override plugin desktop icons in Spatial — only pinned framework icons and explicitly-promoted icons survive — to avoid unilaterally changing the documented "core surface" behavior. If we'd rather Spatial mirror Classic/Unified and show all desktop-resolved server icons, it's a one-line change to the filter.
  • The screen-meta fix is applied to both the standalone TS bridge and the inline PHP chromeless bridge so they stay in sync.

🤖 Generated with Claude Code

Open WordPress Playground Preview

@AllTerrainDeveloper
Copy link
Copy Markdown
Collaborator Author

@claude can you review this pull request?

@claude
Copy link
Copy Markdown

claude Bot commented Jun 7, 2026

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


PR Review: Fix placement, gear/Help, and dock⇄desktop visibility bugs

Todo List

  • Load tools
  • Read changed files and diff
  • Review TypeScript changes
  • Review PHP changes
  • Review tests
  • Post final review

This is a well-structured bug-fix PR. Each fix is correctly targeted at the root cause, the test coverage is solid, and the documentation is kept in sync. Below are my findings, grouped by severity.


✅ What's correct and well-done

computeHideTarget reads live state at pick time (src/item-visibility-menu.ts:109–119): By calling getApi()?.getOsSettings?.() lazily inside onPick rather than capturing the value at menu-open time, the function produces the correct result even if another consumer writes itemVisibility between the right-click and the pick. The currentPlacement captured at menu-open time (for showing/hiding the "Also show on" entry) can be stale in that edge case, but the actual write is always correct. This is the right design.

keptServerIcons data-source isolation (src/desktop-layout.ts:363–369): The filter operates on serverIcons (desktop-registered icons), while explicitlyPromoted iterates items (dock-registered items). The two sources are mutually exclusive, so there is no risk of duplicates in the rendered list.

Empty-array postMessage always fires (src/iframe-bridge-standalone.ts, includes/render/chromeless-bridge.php): Moving the postMessage before the panels.length === 0 early-return is the minimal correct fix. The parent's addScreenMetaButtons() clears and re-populates on every call, so [] is a safe "remove everything" signal.

PHP regex for dockOrder (includes/os-settings.php:481): preg_replace('/[^a-z0-9_:-]+/', '', strtolower($id)) is correctly minimal — it folds case and preserves the colon without opening up any meaningful injection surface, since the charset is still far tighter than the unrestricted $_POST input. The dedup guard underneath catches any resulting collisions.

structuredDefaults comment (src/settings/state.ts:542–549): The explanation for why the clones are needed (gradient editor aliasing the singleton) is concise and exactly the kind of non-obvious invariant the comment rule asks for.


🟡 Minor observations

1. dockPromotedPositions inner objects are not deep-cloned (src/settings/state.ts:549)

dockPromotedPositions: { ...DEFAULTS.dockPromotedPositions },

A spread creates a shallow copy. If DEFAULTS.dockPromotedPositions were ever initialised with entries, the inner { x, y } objects would be shared between the singleton and the clone. In practice DEFAULTS starts with {} so this is currently safe — but the comment says "Clone the collection fields too" without qualifying the depth, which may mislead a future reader. Consider either a note in the comment or a one-liner like Object.fromEntries(Object.entries(DEFAULTS.dockPromotedPositions).map(([k,v]) => [k,{...v}])). Not a current bug, but latent.

2. subscribeOsSettings callback typed more narrowly than the real API (src/settings/sections/apps-icons.ts:171–180)

The inline type written here —

cb: ( snapshot: { itemVisibility: Record<string, ItemVisibility> } ) => void

— is structurally compatible with the real OsSettingsSnapshot (which does include itemVisibility), but if a future refactor renames the field or changes the type, TypeScript will still accept the cast and the narrowed callback will silently receive a different shape. Using the actual exported type OsSettingsSnapshot from src/settings/registry.ts (or at minimum import type { OsSettingsSnapshot }) would make this an explicit, checked dependency. Fix this →

3. wp.desktop type assertion repeated across files

The same inline shape for window.wp?.desktop is duplicated between src/settings/sections/apps-icons.ts and src/settings/desktop-shortcuts-sync.ts. Neither file can import the public TS API type directly (different bundle), but a small shared internal helper (e.g. src/shell-api.ts exporting getShellApi()) would eliminate the drift risk. Not blocking — both assertions are currently correct — but worth noting for a follow-up.

4. No unit tests for computeHideTarget / railFromId / prunePromotedPositions

The new spatial-layout tests in desktop-layout.test.ts are good regression guards. The item-visibility-menu helpers (railFromId, computeHideTarget) and the pruning helper in desktop-shortcuts-sync.ts are not directly unit-tested — coverage happens only through integration paths. Given that computeHideTarget is the core correctness fix for the "teleporting tile" bug, a few unit tests (both → demote to other rail, single-rail → hidden) would be a cheap safety net against future regressions. Not a blocker.

5. hasScreenOptionsContent selector includes button (src/iframe-bridge-standalone.ts:862, includes/render/chromeless-bridge.php)

wrap.querySelector('input, select, textarea, button, fieldset')

A Screen Options wrap that contains only a "Close" / "Apply" button and nothing else would still trigger the gear. In practice WordPress always renders inputs alongside submit buttons, so this is not a real regression risk — but the intent is "has renderable content," and button-only is a degenerate case. Adding a note that the button is there because WP always co-locates it with actual form fields (and so its presence is a reliable proxy) would help a future reader not trim it and reintroduce a gap.


🔴 Nothing blocking

No correctness bugs, no public-API breaks, no test failures. The one PHP change that deserves a second look — the preg_replace sanitizer — correctly mirrors the JS id charset and is validated by the new PHPUnit tests.


Summary

All five bugs described in the PR body have correct, minimal fixes. Tests cover the two most load-bearing new behaviours (spatial-layout pinned icon, dockOrder colon preservation). The observations above are minor: one latent aliasing risk in structuredDefaults, one type-safety improvement for the subscribeOsSettings callback, and missing unit tests for the new helpers. None of these block the merge.

@AllTerrainDeveloper AllTerrainDeveloper enabled auto-merge (squash) June 7, 2026 10:46
@AllTerrainDeveloper AllTerrainDeveloper merged commit 8a205a9 into trunk Jun 7, 2026
5 checks passed
@AllTerrainDeveloper AllTerrainDeveloper deleted the fix/placement-gearhelp-visibility-bugs branch June 7, 2026 10:48
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