@claude Can you review please?

Claude finished @AllTerrainDeveloper's task in 1m 50s —— View job

Code Review

• Gather context and read changed files
• Analyze the diff and review changes
• Post review feedback

The fix is in the right place and the guard logic is correct. A few things worth looking at before merging:

What the change does

In WindowManager.focus() (src/window-manager/index.ts:938–944), when focus moves from one window to another, if the previously-focused window is in fullscreen state it is immediately exited via toggleFullscreen(). This prevents the new window from rendering silently behind the fullscreen z-layer.

Issues

1. Framework policy: automatic behavior without a hook (moderate)

AGENTS.md says:

When you're tempted to add a heuristic inside the framework — "do X automatically when Y" — stop and turn it into a hook the app can subscribe to. App owns the policy.

This change auto-exits fullscreen on focus change, which is appropriate for a "focus mode" text-editor window but could be wrong for a plugin that uses fullscreen for an immersive game, slideshow, or video player — those plugins may want fullscreen to persist until the user explicitly exits it. Consider whether this should be opt-out (e.g. a persistFullscreenOnBlur flag on the window config) or whether an action/filter should fire first so plugins can cancel the behavior before toggleFullscreen() is called. A simple applyFilters( 'desktop_mode_exit_fullscreen_on_focus_change', true, { windowId: previouslyFocused.id } ) would preserve the fix while letting plugin authors override.

2. No test coverage (minor)

The existing fullscreen test at tests/vitest/window-lifecycle-hooks.test.ts:156 only covers toggleFullscreen() directly. There is no test for the new path: "when window B is focused, window A in fullscreen exits fullscreen." A test alongside the existing lifecycle tests would prevent regressions here. Add a test →

3. Inline comment references a specific repro path (minor)

AGENTS.md says: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves."

The comment at line 935–937 includes:

// (repro: maximize Appearance → focus-mode → Theme Files Editor → functions.php
// opens a new window the user can't see).

That repro belongs in the PR description or commit message, not the source. The broader invariant ("fullscreen z-index is above all other windows") is the thing worth preserving in the comment.

Non-issues (confirmed correct)

• Guard previouslyFocused && previouslyFocused !== win && previouslyFocused.isFullscreen() is tight — correctly skips re-focus of the same window and skips when the stack is empty.
• remove() splices the window from _stack before calling focus(), so previouslyFocused === win there — the guard prevents a spurious toggle on close.
• WINDOW_FULLSCREEN_EXITED fires (inside toggleFullscreen()) before WINDOW_BLURRED/WINDOW_FOCUSED — this "lose fullscreen, lose focus, gain focus" ordering is logical.
• onMinimize calls focus() with a different window than the minimized one; previouslyFocused in that path is the topmost (now minimized) window. Since a window being minimized presumably isn't in fullscreen state (you'd typically exit fullscreen before minimizing), this shouldn't be a problem in practice.

Summary: The core logic is sound and fixes a real UX bug. The main question before merging is whether the framework should auto-exit fullscreen unconditionally or give plugins an opt-out. A test covering the new behavior would also be a good addition.

✅ 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