@claude can you review this?

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

PR Review

Tasks:

• Gather context and understand the request
• Read changed files: src/window/index.ts, src/window-manager/arrange.ts, tests/vitest/window-state-transitions.test.ts, docs/examples/window-lifecycle.md
• Analyze changes for correctness, bugs, and design issues
• Post review feedback

The core motivation is solid — restore() unconditionally snapping back to 'normal' while leaving --maximized on the element was a real state/class desync bug, and the cascade reorder is the correct fix for that context. The fullscreen cross-state class-stripping is also clearly right. A few issues need attention before merge though.

Bugs

1. tile() not updated — minimized-while-maximized windows won't unmaximize after tile

arrange.ts:cascade() was fixed to run the minimized → restore pass first, so the subsequent maximized → toggleMaximize check sees the restored state. But tile() (lines 131–141) still uses the old order (fullscreen → maximized → minimized). With the new restore() behaviour, a window minimized while maximized will have restore() called last — after the maximized check has already been skipped — and it'll stay maximized inside the tile grid.

// tile() — old order, now broken for minimized-from-maximized:
if ( w.state === 'fullscreen' ) { w.toggleFullscreen(); }
if ( w.state === 'maximized'  ) { w.toggleMaximize();  }
if ( w.state === 'minimized'  ) { w.restore();          } // lands maximized, never unwound

Fix this →

2. Geometry overwrite in toggleFullscreen exit-to-maximized path

When exiting fullscreen with _savedFullscreenState.state === 'maximized', the code does:

// src/window/index.ts ~line 1615
this.state = 'normal';   // ← trick to bypass idempotency guard
this.maximize();

But inside maximize(), the guard if ( this.state === 'normal' ) now fires and re-saves _savedGeometry from the element's current offsetLeft/offsetTop/offsetWidth/offsetHeight — which at that point hold the maximized inline values (left: 0px, top: 0px, width: parentW, height: parentH) because those were set when the window was first maximized. The user's real floating geometry ({x:40, y:40, w:800, h:600}) is silently overwritten.

After this, toggleMaximize() → normal will restore to {x:0, y:0, w:1200, h:800} instead of the original position.

The test for this case ('maximize → fullscreen → toggle-off fullscreen → toggle-off maximize') passes only because JSDOM returns 0 for all offsetLeft/Top/Width/Height regardless of inline styles — so startLeft === 0 === overwritten value. It's a vacuous assertion.

A minimal fix: instead of the state = 'normal' trick, save and restore _savedGeometry around the maximize() call:

const geo = this._savedGeometry;
this.state = 'normal';
this.maximize();
this._savedGeometry = geo; // don't let maximize() re-capture

Or skip the trick altogether and implement a private _enterMaximized() that doesn't have the idempotency guard, calling it from both maximize() and here.

Fix this →

3. Double _emitChange('state') on toggleFullscreen exit paths

When exiting fullscreen to 'maximized' or a snapped state, maximize() / applySnap() each call _emitChange('state') internally. toggleFullscreen's common tail (line ~1669) then calls it again. State-change subscribers fire twice per transition for these paths. This was fine before because those helpers were only called from a clean initial state; now they're called from within toggleFullscreen's own _emitChange scope.

4. TypeScript private field access in test

tests/vitest/window-state-transitions.test.ts:284:

expect( handle.win._savedGeometry ).toEqual( { ... } );

_savedGeometry is declared private in Window. If the test files are included in the tsc --noEmit project config, this will fail type-checking. Either cast (handle.win as any)._savedGeometry or widen the field to @internal (remove private, add a JSDoc note).

Hook ordering inconsistency (minor)

toggleMaximize() fullscreen → maximize path: fires WINDOW_FULLSCREEN_EXITED first, then WINDOW_MAXIMIZED (via maximize()).

toggleFullscreen() exit-to-maximized path: calls maximize() first (fires WINDOW_MAXIMIZED), then the common tail fires WINDOW_FULLSCREEN_EXITED.

Plugin authors subscribing to both hooks will observe different ordering depending on which code path triggered the transition. Not a blocker, but worth aligning.

Positives

• The _stateBeforeMinimize null-guard on double-minimize is clean and the test coverage for it is good.
• Class-stripping in maximize(), applySnap(), and toggleFullscreen() enter is the right approach — "strip all, add one" is much safer than "only add."
• The cascade reorder + comment is clear and correct.
• The --fullscreen addition to applySnap()'s strip list is the right defensive fix.
• Doc update accurately captures the new restore semantics.
• Test infrastructure (activeStateClasses helper, mountWindow wrapper) is well-designed and reusable.

✅ 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

@claude second round?

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

PR Review — Round 2

Tasks:

• Gather context and understand the request
• Read current state of changed files on branch
• Check each issue from round 1: resolved or still present
• Look for new issues in the fix commit
• Post updated review feedback

All five issues from round 1 are resolved. Here's the scorecard plus two new observations from the fix commit.

Round 1 Issues — Status

1. tile() order ✅ Fixed — arrange.ts:127–147 now runs the minimize check first in tile(), matching the cascade fix from the original commit.

2. Geometry overwrite in toggleFullscreen exit-to-maximized ✅ Fixed — _applyMaximizeVisuals() (private, visuals-only) is called directly instead of this.maximize(), so the if ( this.state === 'normal' ) geometry-save guard in maximize() is never triggered from a non-normal state. The test 'maximize → fullscreen → toggle-off fullscreen → toggle-off maximize: lands on original geometry' now uses the stubbed offset values (40, 60, 800×600) and would actually fail if the geometry were clobbered, unlike the previous vacuous version.

3. Double _emitChange on toggleFullscreen exit paths ✅ Fixed — both toggleFullscreen() exit paths and toggleMaximize()'s fullscreen-to-maximized branch each call _emitChange exactly once. The 'fullscreen exit-to-maximized emits state-change exactly once' test guards the regression.

4. TypeScript private field access in tests ✅ Fixed — tests now assert element.style.left/top/width/height rather than reaching into _savedGeometry directly.

5. Hook ordering inconsistency ✅ Fixed — both toggleFullscreen() exit-to-maximized and toggleMaximize() from-fullscreen now fire FULLSCREEN_EXITED before MAXIMIZED. The two hook-order tests ('fullscreen → maximize via Maximize button fires FULLSCREEN_EXITED then MAXIMIZED, in that order' and the symmetric 'fullscreen → exit-to-maximized via Focus button fires same hook sequence') lock this in.

New Observations

A. restore() to fullscreen skips updateFocusButtonState() — low severity

restore() at line 1475 sets this.state = this._stateBeforeMinimize ?? 'normal' but never calls updateFocusButtonState() or updateFullscreenBodyClass(). For the common path — focus button was already showing "Exit fullscreen" before minimize and nothing touched it during the hidden period — the state persists correctly through minimize/restore. But if the button is re-rendered during the minimized window's lifetime (tab re-mount, accessibility refresh), it would come back showing the wrong label. The test for 'fullscreen → minimize → restore: returns to fullscreen' doesn't cover focus button or body-class state, so this gap is invisible to the suite.

A targeted fix: call updateFocusButtonState() and updateFullscreenBodyClass() inside restore() when the restored state is 'fullscreen':

if ( wasMinimized ) {
    this.state = this._stateBeforeMinimize ?? 'normal';
    this._stateBeforeMinimize = null;
    if ( this.state === 'fullscreen' ) {
        updateFullscreenBodyClass();
        this.updateFocusButtonState();
    }
}

Fix this →

B. restore() fires WINDOW_RESTORED but not WINDOW_MAXIMIZED/WINDOW_FULLSCREEN_ENTERED when restoring to those states

When a maximized-then-minimized window is restored, WINDOW_RESTORED fires, but WINDOW_MAXIMIZED does not. This is arguably the correct semantic — the window never left maximized state, it was just hidden — but it can surprise plugin authors who listen to WINDOW_MAXIMIZED expecting it to correlate with the element becoming visible and state-active. The docs/examples/react-to-window-events.md example only covers the simple minimize→restore path, so this corner case is undocumented. Worth a sentence in the doc or a comment in the hook-order tests to make the semantic explicit. Not a blocker.

Summary

The architecture is solid — _applyMaximizeVisuals / _applySnapVisuals as visuals-only primitives elegantly dissolved the three interlocked problems (double-emit, geometry clobber, idempotency-guard trap) without complicating the existing surfaces. Observation A is the only change worth making before merge; B is documentation polish. The hook-ordering and geometry round-trip tests are well-designed and will catch future regressions.

@claude one more?

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

PR Review — Round 3

Tasks:

• Gather context from previous review rounds
• Read current state of changed files on branch
• Check Round 2 observations A and B: resolved or still present
• Deep-read for any remaining issues
• Post updated review feedback

Both Round 2 observations are resolved. One minor cleanup item worth noting, and two pre-existing behaviors that are worth documenting context on but neither is introduced by this PR.

Round 2 Observations — Status

A. restore() to fullscreen missing UI refresh ✅ Fixed — updateFullscreenBodyClass() and updateFocusButtonState() are called inside restore() when this.state === 'fullscreen'. Three targeted tests cover the fix: body-class rebuild after external mutation during minimize, focus-button aria-pressed/--active class refresh, and confirmation that the tests would actually catch a regression (the body-class test manually strips the class before restore to simulate the race, making it non-vacuous).

B. Undocumented WINDOW_RESTORED-only hook semantics ✅ Documented — the new "Restoring from minimized — what fires" section in window-lifecycle.md is accurate and the recipe (win?.isMaximized() inside a restored handler) uses the real isMaximized() method that exists on Window. Two new tests ('restore() to maximized fires WINDOW_RESTORED but NOT WINDOW_MAXIMIZED' / 'restore() to fullscreen fires WINDOW_RESTORED but NOT WINDOW_FULLSCREEN_ENTERED') lock the semantic in as a deliberate contract rather than an oversight.

One cleanup item (non-blocking)

Redundant first assertion in 'fullscreen → minimize → restore: returns to fullscreen'

Lines 232–235 of the test file:

expect( activeStateClasses( handle.win.element ) ).toEqual( [
    'desktop-mode-window--fullscreen',
    'desktop-mode-window--minimized',
].filter( ( c ) => handle.win.element.classList.contains( c ) ) );

activeStateClasses(el) already filters from a hardcoded list of known classes against el.classList. The RHS does the same filter against the same element. The assertion would only fail if an extra class outside the RHS list (e.g. --maximized) leaked onto the element — but the immediate second assertion toEqual(['desktop-mode-window--fullscreen']) is strictly stronger and already catches that. The first assertion adds noise without adding coverage. Safe to drop.

Fix this →

Pre-existing behaviors (context, not blocking)

cascade() / tile() don't reset snapped-* state

Neither arrange function strips --snapped-left / --snapped-right from windows after laying them out. Since those classes carry only box-shadow: none and a border-radius tweak (no !important position/size rules), the inline geometry cascade/tile writes takes visual effect correctly. But window.state stays snapped-left / snapped-right after the arrangement, leaving state and position out of sync until the user next interacts with the window. This was true before this PR and isn't worsened by it — just worth tracking separately.

Minimized fullscreen windows hold the body class

updateFullscreenBodyClass() queries all .desktop-mode-window--fullscreen elements in the DOM. Since minimize() never strips --fullscreen from the element (only adds --minimized), a minimized fullscreen window keeps desktop-mode-has-fullscreen-window on <body>, hiding the admin bar while the window is invisible. The new restore() semantics make this intentional: minimize is a visibility pause, not a state exit. Correct UX — just documenting it explicitly since the updated restore() logic makes the contract more visible.

Summary

The PR is in good shape. Round 2's two observations are fully addressed — the UI-state refresh is correctly guarded, well-commented, and has non-vacuous tests. The hook-ordering contract is now explicit in both docs and tests. The only actionable item before merge is the redundant assertion cleanup, which is cosmetic. Architecture is clean throughout.