🐞 Don't let no-op actions clobber the cached base frame (grow-from-zero)

[bit-saver]

Summary

During a preview-only chord that starts with a no-op action (e.g. trigger + a "do nothing" action bound to a key, then an arrow to grow), the green preview collapses to a tiny zero-size box in the screen center, and the subsequent grow/shrink resizes from that box instead of the window's real frame.

Root cause

ResizeContext is reused across a chord. recomputeTargetFrame unconditionally caches the frame returned by WindowFrameResolver.getFrame. For no-frame actions (.noAction/.noSelection/.cycle/.minimize/.hide and window-focus actions), getFrame returns a zero-size centered sentinel — which then overwrites cachedTargetFrame.raw. When the user next presses grow/shrink/move, WindowFrameResolver.calculateTargetFrame reads context.lastAppliedFrame ?? context.cachedTargetFrame.raw as its base. In a preview-only chord lastAppliedFrame is still nil (it's only set on an actual apply), so it falls through to the polluted cachedTargetFrame.raw = the zero box, and grows from zero.

Pre-refactor (1.4.2) never cached the sentinel: its no-op early-return sat before the lastTargetFrame store, so the grow base stayed the real window frame. The refactor into recomputeTargetFrame lost that invariant.

Fix

Skip caching in recomputeTargetFrame for no-op / no-frame / window-focus actions (mirroring the guard already in WindowFrameResolver.getFrame), preserving the previously-cached real frame. Incremental grow during preview still works because real actions keep updating cachedTargetFrame.raw.

Testing

Built (Release) and verified: trigger + do-nothing + Up now grows from the window's real frame; repeated grow/shrink still steps correctly; normal single actions unaffected.

[bit-saver]

Rebased onto the latest develop — this clears the merge conflict that appeared once #1113 landed (it touched the same recomputeTargetFrame). Also trimmed the comment down. Ready for review whenever you have a chance 🙂

[mrkai77]

Thanks for this PR! I took a look through the logic, and the fix for the original .noAction case makes sense, but I think there are still a couple of cases this doesn't account for.

The first is window focus actions. The new guard preserves the cached frame, but once the focus action is applied, setWindow(to:) changes the target window without updating cachedTargetFrame. This means a sequence like window A > focusRight to window B > grow would grow window B using window A's frame. I think setWindow(to:) would also need to reset the cached frame using the new window's actual frame.

The noFrameActions list also seems incomplete. .minimizeOthers and the space-switching actions don't calculate a target frame either, so they currently fall through calculateTargetFrame and return .zero. A sequence like minimizeOthers > grow could therefore still run into the same grow-from-zero issue. Screen-switching actions technically don't produce a frame either, although LoopManager handles those separately right now.

It may make sense to centralize this as something like WindowDirection.hasTargetFrame, then use that in both ResizeContext and WindowFrameResolver so the two lists can't drift apart. Could you update the PR to account for these cases?

Edit: also, you can ignore the swiftformat errors that are unrelated to this PR :)

[bit-saver]

Thanks for the thorough review — all three addressed:

1. Focus actions: setWindow(to:) now resets cachedTargetFrame to the new window's actual frame, so A → focusRight to B → grow grows B from B's own frame rather than A's.
2. Incomplete list: replaced the two hardcoded lists with a single WindowDirection.hasTargetFrame, which returns false for no-op, minimize/hide/minimizeOthers, cycle, and focus/space/screen-switching. So minimizeOthers → grow (and the space/screen cases) no longer grow from zero.
3. Centralized: hasTargetFrame lives on WindowDirection and is used by both WindowFrameResolver.getFrame and ResizeContext.recomputeTargetFrame, so the two can't drift apart.

Left the unrelated swiftformat errors alone per your note. Thanks!

[mrkai77]

Thanks for addressing those! I took another look, and the previous issues are fixed, but I noticed one remaining behavior change caused by the new early return :)

When hasTargetFrame is false, recomputeTargetFrame() returns without changing the cache, so getTargetFrame() gives the preview the previous cached frame. Since PreviewViewModel determines visibility using the frame's area, a sequence like leftHalf > noAction would leave the preview visible at the previous leftHalf frame instead of hiding it. In the latest release, frameless actions hide the preview without overwriting lastTargetFrame, so this would be a behavioral regression.

I'm thinking that PreviewViewModel should also check context.action.direction.hasTargetFrame when determining shouldBecomeVisible. That should preserve the cache fix while restoring the old preview behavior!

[bit-saver]

Good catch! Fixed — PreviewViewModel.shouldBecomeVisible now also checks context.action.direction.hasTargetFrame, so frameless actions hide the preview (e.g. leftHalf → noAction) instead of leaving the previous cached frame on screen, while grow/shrink/move still resize from the cached frame. Thanks!