Feat/unfocus window effects

[AllTerrainDeveloper]

• Implemented a registry for unfocus effects allowing plugins to register their own effects via wp.desktop.registerUnfocusEffect().
• Created a server-sync module to manage the loading and unloading of unfocus effects based on server-side registrations.
• Developed an unfocus engine that applies the selected unfocus effect to all unfocused windows, responding to window lifecycle events and OS settings changes.
• Added a settings section for users to select their preferred unfocus effect, including a built-in 'darken' effect.
• Introduced unit tests for the new functionality, covering registration, synchronization, and effect application logic.

[AllTerrainDeveloper]

@claude can you review this?

[claude]

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

---

PR Review: Feat/unfocus window effects

Todo list:

• Load tools and post initial comment
• Read key source files (registry, engine, server-sync, types)
• Read PHP backend files
• Read CSS and settings files
• Read test files
• Review for bugs, security, and best practices
• Post comprehensive review feedback

---

This is a well-structured addition that follows the repo's established patterns closely (mirrors the commands/title-bar-buttons/wallpapers registry shape, uses createSharedStore correctly, dogfoods its own public API for built-ins, ships docs). I have one genuine bug to flag and a handful of smaller issues.

---

Bug: stale CSS class after an effect is unregistered while applied

File: src/effects/unfocus-engine.ts — clear() function (lines 60–78)

When a plugin deactivates mid-session, unregisterUnfocusEffectsByOwner removes the def from the registry and triggers recompute(). Inside recompute(), clear(el) is called for every window. At that point getUnfocusEffect(priorId) returns undefined (the def is already gone), so prior?.className never executes — the CSS class is not removed. The defensive listUnfocusEffects() sweep also misses it because the def has already been removed. The data-desktop-unfocus-effect attribute is removed, but the class lingers.

The code comment says "Remove the class…even if that def has since been unregistered" — but that's not what the code actually does.

// current code — className is NOT removed when def is gone
const prior = getUnfocusEffect( priorId );  // → undefined after unregister
if ( prior?.className ) {                    // → skipped
    el.classList.remove( prior.className );
}

Fix: store the applied class in a second data attribute at apply time so clear can always find it:

const EFFECT_CLASS_ATTR = 'data-desktop-unfocus-effect-class';

const apply = ( el: HTMLElement, def: UnfocusEffectDef ): void => {
    if ( def.className ) {
        el.classList.add( def.className );
        el.setAttribute( EFFECT_CLASS_ATTR, def.className );  // ← persist
    }
    el.setAttribute( EFFECT_ATTR, def.id );
    def.apply?.( el );
};

const clear = ( el: HTMLElement ): void => {
    // Stored class always removes correctly — even if def was unregistered.
    const storedClass = el.getAttribute( EFFECT_CLASS_ATTR );
    if ( storedClass ) {
        el.classList.remove( storedClass );
        el.removeAttribute( EFFECT_CLASS_ATTR );
    }
    const priorId = el.getAttribute( EFFECT_ATTR );
    if ( priorId ) {
        getUnfocusEffect( priorId )?.clear?.( el );   // callback still guarded
    }
    // Defensive sweep for any still-registered effects.
    for ( const def of listUnfocusEffects() ) {
        if ( def.className ) el.classList.remove( def.className );
    }
    el.removeAttribute( EFFECT_ATTR );
};

Fix this →

---

No double-init guard in startUnfocusEngine

File: src/effects/unfocus-engine.ts — lines 121–138

The docstring says "Idempotent per shell boot — call once" but there is no enforcement. If called twice (HMR, test bleed, future refactor), the five document.addEventListener calls, the subscribeOsSettings call, and the subscribeUnfocusEffects call all pile up — every window event fires recompute() twice. Every existing registry of this shape in the codebase (commands, wallpapers, title-bar-buttons) has the same open-loop design, but given the engine introduces window event listeners on document (which are harder to inspect than subscribers in a Set), a simple guard is worth the one-liner:

let _started = false;
export function startUnfocusEngine( deps: UnfocusEngineDeps ): void {
    if ( _started ) return;
    _started = true;
    // … rest of the function

The shared store would need to persist this flag across bundles (or it can live module-level since the engine is only imported by the main shell bundle). In tests vi.resetModules() handles cleanup.

---

buildEffectsSection subscription may leak when a parent node is removed

File: src/settings/sections/effects.ts — lines 96–109

The MutationObserver is set up on wrapper.parentNode with { childList: true, subtree: false }. This fires when a direct child of the parent is added or removed — which covers the normal "panel unmounts wrapper" case. But if the parent itself (or an ancestor) is removed from the DOM, the observer never fires and unsubscribe() is never called, leaving a dangling registry listener.

The same pattern appears in other settings sections in the codebase, so this is a systemic (and presumably accepted) limitation — worth documenting in a comment at minimum:

// Note: if a parent node higher up is removed (not `wrapper.parentNode`),
// the observer won't fire. In practice the OS Settings panel always
// removes wrapper directly, so this is safe.
observer.observe( wrapper.parentNode, { childList: true } );

---

NONE sentinel duplicated

const NONE = 'none' is defined independently in both src/effects/unfocus-engine.ts:27 and src/settings/sections/effects.ts:28. Neither file is the canonical home. A one-line re-export from src/effects/types.ts (or registry.ts) would be the single source of truth. Low priority but worth cleaning up.

---

listUnfocusEffects() (and its applyFilters call) invoked per-window inside clear()

File: src/effects/unfocus-engine.ts — lines 72–76

Each call to recompute() calls clear(el) per window, and clear calls listUnfocusEffects() — which invokes applyFilters('desktop-mode.unfocus-effects', …) — to sweep all known class names. For 3–5 windows this is negligible, but the filter call is entirely avoidable by hoisting it out of the per-window loop:

const recompute = (): void => {
    const def = currentId === NONE ? undefined : getUnfocusEffect( currentId );
    const allEffects = listUnfocusEffects();   // ← hoist — one filter call per recompute
    for ( const win of manager.getAll() ) {
        const el = win.element;
        if ( ! el ) continue;
        clear( el, allEffects );   // pass the snapshot
        // …
    }
};

---

Minor praise

• The hostsCanvas WebGL guard is the right call and the comment explaining the GPU context-loss risk is excellent documentation.
• createSharedStore is used correctly — this is the class of bug that "ate days of debugging" per AGENTS.md, and the PR nails it.
• Built-in effects are registered through the public hook, not a private shortcut — this is good API dogfooding.
• PHP sanitizer correctly lower-cases and strips non-[a-z0-9_/-] chars for unfocusEffect rather than using sanitize_key() which would drop the slash and break vendor/sub-id on round-trip.
• The window-manager/index.ts focus-transfer-on-close fix (skipping minimized/off-desktop windows) is well-reasoned and is accompanied by solid tests covering every skip condition.
• Test coverage is thorough — registry validation, engine lifecycle, server-sync idempotency, and PHP sanitization all have dedicated cases.
• CSS effects properly expose custom properties for theming and collapse transitions under prefers-reduced-motion.

---

Summary: The stale-class bug on effect unregistration is the only one that would produce a visible user-facing artefact (class stays on windows after plugin deactivation). Everything else is minor or stylistic. The overall shape of the feature is solid.