Update Don't Pull Me to Another Desktop to v0.4.0

[Meteony]

Changelog

• Renamed the mod
• Changed the classifier to be rooted narrowly in CVirtualDesktopForegroundPolicy::ForegroundViewChanged, which offers better classification accuracy against normal user actions.
• Hardening against apps that briefly activate a pre-existing window before creating a new one.

Mod authorship

If this pull request introduces a new mod, please complete the section below.

This mod was created by:

• • The submitter, without AI assistance
• • The submitter, with AI assistance
• • Claude
• • ChatGPT
• • Gemini
• • Another AI (please specify):
• • Other (please specify):

Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.

[windhawk-reviewer]

Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review.

To get started, comment /ai-review. Once you're happy with the result, comment /ready-for-reviewer to hand it over to a human reviewer.

See the pull request review process for the full details.

[Meteony]

/ai-review

[windhawk-reviewer]

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.

---

The attribution rework (rooting classification in CVirtualDesktopForegroundPolicy::ForegroundViewChanged and dropping the SwitchDesktop/SwitchDesktopWithAnimation hooks and the explicit-switch transaction machinery) is a clear improvement over v0.3.14 — the blast-radius guard at SwitchDesktopInternal_Hook is well placed, teardown joins every thread it starts, and the process-model handling (staying inert in non-shell explorer.exe processes, deferring symbol resolution and COM off the CreateWindowExW stack) is careful. The findings below are mostly about the new supersession layer and about the size of the result.

1. The new view-supersession hardening is inert for every non-packaged app — i.e. for most apps.

This is the headline change in the changelog ("Hardening against apps that briefly activate a pre-existing window before creating a new one"), but it is gated behind HasStableRescueAppIdentity() in three places, and that returns false unless the process has a package family name or AUMID:

• WaitForRescueSettling only extends waitWindow to viewSupersessionMs when the identity is stable (line 2101-2110).
• insideSupersessionWindow requires it (line 2141-2147).
• ValidateObservedSuperseder → RescueAppIdentityMatches returns false immediately when expected has no identity (line 1704-1706).

So for a classic Win32 app — the exact case the README note describes ("Some applications can briefly activate an old window while processing a window creation") — the mod falls back to the blanket 50 ms stabilizationMs and the whole OnViewAddedInternal path is dead weight. QueryRescueAppIdentity also uses OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION), which fails outright against a higher-integrity process, so elevated apps are excluded too.

The gate also doesn't seem to buy anything: ValidateObservedSuperseder already requires observedPid == request.pid (line 2047-2050), and PID identity is strictly stronger than PFN/AUMID identity. The stated rationale (line 1599-1602) is about not correlating unrelated broker processes, but the PID equality check already prevents that. Within a ≤200 ms window PID recycling isn't a realistic concern either.

Suggestion: drop RescueAppIdentity/QueryRescueAppIdentity/HasStableRescueAppIdentity/RescueAppIdentityMatches entirely and rely on the PID match, so the feature applies uniformly. That also removes two OpenProcess + package queries per rescue and per superseder validation, and removes the wasted shell-side work in ObserveViewAddedForPendingRescues (which happily records candidates for unpackaged apps that the worker then always discards with "Ignored VIEWADD supersession candidate").

2. The two settings are internal tuning knobs, and their descriptions don't match what they do.

- rescueStabilizationMs: 50
  $name: "Rescue stabilization (ms)"
- rescueViewSupersessionMs: 200
  $name: "View supersession window (ms)"

"Rescue stabilization", "view supersession", "stable PFN/AUMID", "rescue candidates", "the worker", "source desktop" are implementation vocabulary — a user has no way to reason about what to set these to, or why. Two concrete inaccuracies in the descriptions as well:

• rescueStabilizationMs doesn't only control a delay. It also gates the IsForegroundReplacementOnSourceDesktop guard in two places (if (request.stabilizationMs) at lines 2395 and 2545), so setting it to 0 silently turns off the local-foreground-replacement check too — not just the settling time.
• "Set to 0 to disable" is wrong for packaged apps: with rescueStabilizationMs: 0, WaitForRescueSettling still waits up to viewSupersessionMs (line 2105-2110), so the rescue is still delayed 200 ms by default.

Suggestion: hardcode both constants, or expose at most one plainly-worded option (e.g. Delay before moving the window (ms)) that does exactly what its name says, and decouple the foreground-replacement guard from its value.

3. Wh_ModBeforeUninit joins a thread that makes calls which can block on an unresponsive external process.

StopWorker (line 3465-3472) and StopNotificationCache (line 3570-3572) join with WaitForSingleObject(..., INFINITE), which is correct and required. The problem is what the joined worker can be sitting in:

if (IsIconic(request.hwnd)) {
    ShowWindow(request.hwnd, SW_RESTORE);
}

SetForegroundWindow(request.hwnd);

Both operate on a window owned by another process's thread and do inter-thread message sends; against a hung app they can block for a long time. WorkerMoveViewToDesktop/GetWindowDesktopId are cross-apartment COM calls with the same property. If the mod is disabled or updated at that moment, Explorer's mod-unload path hangs.

ShowWindowAsync is a direct drop-in for the first call and removes the most likely case. It's worth noting that mods/virtual-desktop-helper.wh.cpp#L783-L784 uses the same ShowWindow + SetForegroundWindow pair, but that's a tool mod (@include windhawk.exe), so a stall there can't take Explorer with it — here it can.

4. Size and formatting: the file is 4263 lines, and a large part of that is avoidable.

The repo ships a .clang-format (Chromium, 4-space indent, 80 columns). This file isn't formatted with it — it's wrapped at roughly one argument per line, e.g.:

    g_startHr.store(
        hr,
        std::memory_order_relaxed);

Reformatting with the repo style would cut the line count substantially without touching a single statement, and would make the diff for the next update readable. Please run clang-format over the file.

Beyond formatting, the mod is now a very large amount of speculative machinery layered on undocumented shell internals, and each layer is another thing that has to be re-verified on the next Windows build. Two concrete reductions, on top of item 1:

• g_navigationFocusHistory is a 32-entry ring with sequence numbers, generations and a 2 s expiry sweep — but GetNewForegroundViewForDesktopSwitch is called synchronously inside DesktopChanged and consumed by the immediately following ForegroundViewChanged on the same thread. Is a ring of 32 (rather than one slot, or a small thread_local one) actually needed for an observed case, or is it defensive?
• SwitchDesktopInternal_Hook contains several never-taken defensive branches (see the optional section).

Optional improvements

Minor polish — none of this affects users, so it's your call.

• Dead checks. g_navigationGeneration starts at 1 and is only ever fetch_add-ed, and g_foregroundPolicyNavigationGeneration is assigned from it at FVP entry — and SwitchDesktopInternal_Hook only reaches these lines when g_foregroundPolicyDepth > 0, i.e. inside FVP on this thread. So g_foregroundPolicyNavigationGeneration == 0 at line 3198 and navigationGeneration == 0 at line 3279 can never be true.
• RuntimeCancellationRequested() does a kernel call. It's WaitForSingleObject(g_runtimeCancelEvent, 0), and it's called from SwitchDesktopInternal_Hook, QueueRescue and ObserveViewAddedForPendingRescues — all on shell call stacks. g_unloading is already an std::atomic<bool> set at the same moment (StopRuntimeBeforeUninit, line 4113-4124); reading that instead is free. Keep the event only where you actually need to wait on it.
• Wh_ModUninit repeats the teardown. StopRuntimeBeforeUninit() has already run StopNotificationCache()/StopWorker() and joined everything, so StopRuntime() at line 4251 is a no-op second pass. Dropping it (keeping only the CloseHandle(g_runtimeCancelEvent)) makes the lifecycle easier to follow.
• twinui.pcshell.dll handle is never released. InstallVirtualDesktopHooks calls LoadLibraryExW (line 3595) and never FreeLibrarys it, so every enable/disable cycle adds a reference. Harmless in practice for a DLL Explorer already has loaded, but a GetModuleHandleW first (falling back to LoadLibraryExW(..., LOAD_LIBRARY_SEARCH_SYSTEM32)) would avoid it. The LOAD_LIBRARY_SEARCH_SYSTEM32 flag is correct as-is.
• Dynamic resolution of GetPackageFamilyName/GetApplicationUserModelId. If the identity code survives item 1: the mod targets Windows 11 24H2+, where both are always present, so the GetProcAddress dance (re-done on every call, line 1640-1687) can just be direct calls.
• Naming/README regression. Renaming from "Prevent Window Activation from Stealing Virtual Desktop" to "Don't Pull Me to Another Desktop" is your call, but the new name contains neither "virtual desktop" nor "activation", which is likely to hurt discoverability for the exact users looking for this. The trimmed README also dropped two things that were genuinely useful: the reproducible demo (open Windhawk on desktop 1, switch, click the tray icon) and the explicit statement that intentional navigation (Win+Ctrl+Left/Right, Task View) is left alone and that the mod is compatible with Disable Virtual Desktop Transition Animation. Consider keeping those.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

• Some suppression paths swallow the activation entirely. SwitchDesktopInternal_Hook returns S_OK without calling the original and without queuing a rescue at line 3210 ("FVP superseded by newer navigation") and line 3287 ("navigation changed before rescue could be queued"). In those cases the user's click produces neither a desktop switch nor a window move — nothing visible happens at all. That's a deliberate "fail-stationary" choice per the comments, but it's the opposite of the fail-open policy the rest of the hook follows ("unknown SwitchDesktopInternal callers are always allowed"). Since these are races rather than steady-state paths, letting the original run would arguably be the safer default.
• Init gives up permanently for the session. ShellHostInitThreadProc retries at most 30 times and then leaves RuntimeState::Stopped for the rest of the Explorer session, with nothing to re-arm it (PromoteCurrentProcessToShellHost is only reached again from a new Shell_TrayWnd, which won't happen). The per-attempt budget is generous (up to 5 s each for the worker and the notification cache), so this is unlikely to be hit — but if it ever is, the mod shows as enabled and silently does nothing.
• SetForegroundWindow can be refused. If Windows denies the foreground change, the window has already been moved to the current desktop but ends up behind whatever is in front — a half-applied result. Worth checking the return value and at least logging it, and possibly flashing the taskbar button instead.
• Hardcoded vtable slots. kVtableMoveViewToDesktop = 4 / kVtableGetCurrentDesktop = 6 are gated by the 24H2 IVirtualDesktopManagerInternal IID, which is the right way to do this, so this is just an FYI: a build that keeps the IID but reorders the vtable would call the wrong method. Declaring the interface's leading methods as pure virtuals (as you already do for IApplicationViewCollection and IVirtualDesktopNotification) instead of indexing would make that impossible and is not much more code.

---

Next steps:

• /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
• /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

[Meteony]

Thanks for the detailed review. I've addressed the findings that held up after checking them against the runtime behavior observed while developing the attribution rework.

For view supersession, I agree that same-PID identity is the actual correlation criterion and that the previous PFN/AUMID matching was redundant. The superseder is now validated by same PID, a genuinely different shell view/HWND, and ownership by the current/source desktop. Stable package/AUMID identity is queried only once and is now used solely to decide whether to incur the longer 200 ms grace period. I kept that eligibility gate rather than extending every ordinary rescue from ~50 ms to ~200 ms.

The two timing settings have been removed entirely. They are now internal policy constants, and the local-foreground replacement validation no longer depends on whether the settling delay is enabled.

ShowWindow has been replaced with ShowWindowAsync, and failure of SetForegroundWindow is now checked and logged. The worker and notification joins intentionally remain unbounded: Windhawk must not unload the mod while those threads can still execute code from it.

I've also run the repository clang-format over the complete source. The file dropped from roughly 4260 lines to roughly 3100 lines without removing behavior. The impossible generation-zero checks, zero-time cancellation-event waits on shell hook paths, redundant second teardown, repeated package API resolution, and normal enable/reload twinui.pcshell.dll reference increment have also been cleaned up.

I kept the navigation-focus history rather than replacing it with one slot. During testing we reproduced an older navigation-selected view's ForegroundViewChanged arriving only after a newer DesktopChanged had already completed. Multiple navigation selections can therefore be outstanding; a single slot could be overwritten before the delayed FVP consumes it. I've added a comment explaining that observed race.

I also intentionally kept the stale-navigation suppression paths. These aren't unknown callers being treated fail-closed: the operation has been positively attributed to an obsolete navigation generation. We reproduced the A→B→C case where allowing the delayed B foreground-policy operation to continue pulls the user back from C to B. Calling the original in those paths would therefore restore the race the generation tracking was added to prevent.

The bounded initialization retry policy remains as-is. Exhausting all attempts is an extremely unusual fail-open startup condition, and I don't think adding permanent retry/rearming state is justified for that case.

Finally, I kept the explicit IVirtualDesktopManagerInternal slot access. A partial pure-virtual declaration would encode the same private vtable ordering and therefore wouldn't provide additional ABI protection if Microsoft reordered the interface without changing its IID. The existing 24H2+ IID gating remains the ABI guard.

The README now also restores the simple reproduction steps, intentional-navigation guarantees, Virtual Desktop Helper/Disable Virtual Desktop Transition Animation compatibility notes, and explicit "virtual desktop" wording for discoverability.

[Meteony]

/ai-review

[windhawk-reviewer]

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.

---

The lifecycle side of this update looks solid: every thread (ShellHostInitThreadProc, WorkerThreadProc, NotificationThreadProc) is signalled and joined inside Wh_ModBeforeUninit, no lock is held across a blocking wait, all COM objects are created and released on the thread that owns them, there are no globals with non-trivial destructors, twinui.pcshell.dll is loaded with LOAD_LIBRARY_SEARCH_SYSTEM32, and HookSymbols is called exactly once per module. The two findings below are both in the new v0.4 classification logic.

1. IsForegroundReplacementOnSourceDesktop never checks that the foreground window is new — it can silently cancel rescues that worked in v0.3.

The check accepts any foreground window that (a) isn't request.hwnd, (b) isn't owned by this Explorer process, and (c) is on the source desktop. Nothing establishes that this window appeared because of the activation. The window you were already using on your current desktop when you triggered the activation satisfies all three conditions, so the rescue is aborted and nothing happens at all — no desktop switch (it was suppressed) and no teleport.

Explorer-owned UI is the only exclusion, so this only stays out of the way when foreground routes through the taskbar/tray (the README's Windhawk-tray repro). It bites for the other cases the README advertises — activating from a third-party launcher, a terminal, another app's "show my window" button, or a folder window running in a separate explorer.exe process ("Launch folder windows in a separate process"). This function is new in v0.4, so those cases are a regression against v0.3.

Fix: capture the foreground window when the request is queued (you're already on the hook stack in SwitchDesktopInternal_Hook) and require the observed replacement to differ from it:

 struct RescueRequest {
     ...
     HWND hwnd = nullptr;
+    HWND foregroundAtQueue = nullptr;   // set from GetForegroundWindow() in QueueRescue
     ...
 };

 static bool IsForegroundReplacementOnSourceDesktop(WorkerComState* state,
                                                    const RescueRequest& request,
                                                    HWND* replacementHwnd) {
     HWND foreground = GetForegroundWindow();

     if (!foreground || foreground == request.hwnd ||
+        foreground == request.foregroundAtQueue ||
         !IsRescueCandidate(foreground)) {
         return false;
     }

That keeps the intended "the app resolved the activation locally" case (a genuinely different window took foreground after the request was queued) while dropping the "nothing changed" false positive. It would also let you relax the foregroundPid == GetCurrentProcessId() exclusion, which currently exists only to paper over the same problem for shell UI.

2. The v0.4 supersession hardening never applies to non-packaged apps, contradicting both the code comment and the README.

In WaitForRescueSettling:

const bool insideSupersessionWindow =
    request.extendedSupersessionEligible && observedAt != 0 &&
    observedElapsed <= kRescueViewSupersessionMs;

extendedSupersessionEligible is only true when HasExtendedSupersessionIdentity() found a package family name or an AUMID for the process. For a plain Win32 app the VIEWADD candidate is observed, published, woken on — and then explicitly thrown away with Ignored VIEWADD supersession candidate. So the behavior the PR changelog describes ("Hardening against apps that briefly activate a pre-existing window before creating a new one") is inert for exactly the kind of app most likely to do that.

That directly contradicts ProcessRescueRequest's own comment — "Identity controls latency only; same-PID/new-view/current-desktop checks decide whether to cancel" — and the README note, which both say identity only buys a longer wait.

Fix: drop the eligibility term from the cancel condition and leave it only in the waitWindow computation:

const bool insideSupersessionWindow =
    observedAt != 0 && observedElapsed <= kRescueViewSupersessionMs;

Better still, consider deleting HasExtendedSupersessionIdentity entirely and using one window for everyone. As written it costs an OpenProcess plus two app-model queries per rescue, pulls in <appmodel.h>, and threads an extendedSupersessionEligible flag through RescueRequest, WaitForRescueSettling and the worker loop — all to choose between 50 ms and 200 ms. ValidateObservedSuperseder already proves same-PID, different-view, different-HWND and current-desktop ownership before anything is cancelled, so the identity probe isn't buying safety.

Optional improvements

Minor polish — none of this affects users, so it's your call.

• No settings block at all. kRescueStabilizationMs and kRescueViewSupersessionMs are the two knobs most likely to need tuning per machine, and "activate the summoned window" (the ShowWindowAsync + SetForegroundWindow pair at the end of ProcessRescueRequest) is a reasonable toggle. A small ==WindhawkModSettings== block would let users adjust these without a rebuild.

• Mod rename. @name changed from Prevent Window Activation from Stealing Virtual Desktop to Don't Pull Me to Another Desktop. The new name is nicer to read but drops "virtual desktop" / "activation", which are the terms users search for, and existing users will see the mod renamed under them. Something like Don't switch virtual desktops on window activation keeps both. Also, the name uses a typographic apostrophe (U+2019); no other mod in the catalog does, and a plain ' is safer for search and URL generation.

• README list-item indentation. In the second ### Notes bullet, the last two lines (worthwhile. A newly-added... / belong to the current desktop...) lost their two-space continuation indent. It still renders via lazy continuation, but it looks like a slip.

• Stale comment in Wh_ModInit: "don't load taskbar.dll or twinui.pcshell.dll until this PID proves it owns the shell's primary taskbar" — the mod never touches taskbar.dll.

• The process-wide CreateWindowExW hook exists only to notice Shell_TrayWnd creation, and stays installed in every explorer.exe for the life of the process (including folder-window processes, which never promote). Since the runtime is keyed to twinui.pcshell.dll anyway, hooking LoadLibraryExW in kernelbase.dll for that module — combined with the GetModuleHandleW check you already do — would be a much narrower trigger than intercepting every window creation in the shell. Only worth doing if the taskbar-ownership semantics survive the change; CreateWindowExW is cheap enough that this is purely a blast-radius nicety.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

• v0.4 adds a fixed delay to the mod's headline action. Every rescue now waits at least kRescueStabilizationMs (50 ms) before the window starts moving, and 200 ms for any packaged app — which covers most Store apps and a growing share of modern desktop apps. v0.3 teleported immediately. That's a deliberate trade for the supersession detection, but it is user-visible latency on the common path, and it's the main argument for exposing the value as a setting (see above).

• Wh_ModBeforeUninit blocks for as long as HookSymbols takes. StopRuntimeBeforeUninit waits INFINITE on the shell-host init thread, which may be inside WindhawkUtils::HookSymbols for twinui.pcshell.dll. On a first run with cold symbols that can take a while, so disabling the mod right after enabling it can appear to hang. This is correct — abandoning the thread would violate the unloadability contract — and there's no clean alternative given symbol resolution has to happen off the Wh_ModInit path in non-shell Explorer processes. Just noting it as expected behavior.

• Fail-stationary paths do nothing at all. Three branches in SwitchDesktopInternal_Hook return S_OK without calling the original and without queueing a rescue (stale navigation focus, FVP superseded by newer navigation, navigation changed before queueing). From the user's side that's a click that produces no visible effect. The reasoning is sound for a genuinely superseded navigation, but if you ever see reports of "clicking the taskbar button sometimes does nothing", these are the first logs to check.

---

Next steps:

• /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
• /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

[windhawk-reviewer]

New commits were pushed, so this pull request left the human review queue and is back to waiting-for-author.

Comment /ai-review to get an AI review of the updated code, then /ready-for-reviewer to hand it over to a human reviewer again. See the pull request review process for details.

[Meteony]

For IsForegroundReplacementOnSourceDesktop, the rescue request now snapshots the Win32 foreground HWND when it is queued. The later settling check only treats a source-desktop foreground as a replacement if it actually differs from that queue-time foreground. I made the guard slightly more conservative as well: if the queue-time foreground is unavailable or already equals the remote activation target, the foreground heuristic abstains because there isn't a trustworthy pre-activation baseline. The existing shell-process exclusion remains as an additional conservative guard.

For view supersession, extendedSupersessionEligible now only chooses the listening deadline. A request without stable package/AUMID identity listens for the normal 50 ms settling window; an eligible request listens for up to 200 ms. Any VIEWADD observed inside that request's actual waitWindow is then judged solely by the same-PID / different-view / different-HWND / current-desktop validation. This matches the intended invariant that identity controls latency rather than correlation.

I also fixed the README continuation indentation and removed the stale taskbar.dll comment.

I kept the timing values internal rather than re-exposing them as settings, since they're implementation policy rather than user-facing behavior. I also kept the CreateWindowExW shell-ownership observer: the important property there is positively identifying the Explorer process that owns Shell_TrayWnd; observing twinui.pcshell.dll loading alone doesn't provide the same ownership signal.

The other functionality notes are intentionally unchanged: the bounded settling delay is the tradeoff required for supersession detection, the HookSymbols unload wait is required for safe unload, and the fail-stationary branches are retained for positively identified stale navigation generations.

[Meteony]

/ai-review

[Meteony]

I've narrowed IsForegroundReplacementOnSourceDesktop further. The queue-time foreground baseline is still retained so the foreground must actually have changed after the rescue was armed, but a later foreground window can now cancel the rescue only if it belongs to request.pid, i.e. the process whose remote view was being activated.

That avoids treating normal focus restoration from transient taskbar/tray/launcher UI as evidence that the activation resolved locally, while keeping the fallback useful when the activated application itself genuinely foregrounds a different window on the current desktop.

I also applied the three minor cleanups: the package/AUMID eligibility query is skipped when the optional OnViewAddedInternal hook isn't available, the README again explicitly notes that a superseding view is independently verified to belong to the current desktop, and the diagnostic desktop->GetId() call in DesktopChanged_Hook is now only made for the outermost navigation where its log value is actually used.

I haven't changed the 50/200 ms timing split. The longer unidentified-app window is an interesting tuning possibility, but we currently have a concrete ~143 ms repro for the extended case and no corresponding evidence that ordinary Win32 rescues need a larger generic delay. I'd prefer to keep the common path at 50 ms until there's a real case demonstrating otherwise.

[Meteony]

/ai-review

[windhawk-reviewer]

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-08-30 16:07 UTC (in 35 minutes) to get another one.

[Meteony]

/ai-review

[windhawk-reviewer]

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.

---

Narrowing IsForegroundReplacementOnSourceDesktop to foregroundPid == request.pid is the right resolution of last round's finding, and the two other cleanups landed as described (HasExtendedSupersessionIdentity is now gated on g_viewSupersessionAvailable, and DesktopChanged_Hook's diagnostic GetId moved inside the outermost branch). I re-checked the parts that don't change often and they still hold: the IVirtualDesktopNotification vtable shape matches what mods/taskbar-desktop-indicator.wh.cpp uses for the same IID (14 slots, CurrentVirtualDesktopChanged at index 10), slots 4/6 are the right MoveViewToDesktop/GetCurrentDesktop for the 24H2 IVirtualDesktopManagerInternal IID, all three threads are signalled and joined inside Wh_ModBeforeUninit (so no Wh_SetFunctionHook/Wh_ApplyHookOperations can run after it returns), no lock is held across a blocking wait or a cross-apartment COM call, and every event handle is created/closed under the same lock that guards its use. The two findings below are both about whether the supersession guards actually fire.

1. foregroundAtQueue is very likely equal to request.hwnd on the normal path, which makes the whole local-foreground-replacement fallback dead.

QueueRescue snapshots the baseline while the FVP-owned switch is on the hook stack:

request.foregroundAtQueue = GetForegroundWindow();

and IsForegroundReplacementOnSourceDesktop abstains outright when that baseline is the activated window:

if (!request.foregroundAtQueue ||
    request.foregroundAtQueue == request.hwnd) {
    return false;
}

But CVirtualDesktopForegroundPolicy::ForegroundViewChanged is the virtual-desktop manager reacting to a foreground-view change, and GetForegroundWindow() is desktop-agnostic — a window on another virtual desktop becomes the Win32 foreground window first, which is precisely what makes the shell want to switch desktops. If that ordering holds, GetForegroundWindow() already returns request.hwnd at queue time on the mod's main path, the guard returns false for every request, and foregroundAtQueue, IsForegroundReplacementOnSourceDesktop and its two call sites in ProcessRescueRequest are unreachable code.

That matters beyond dead weight: this function is the only supersession protection when the optional OnViewAddedInternal symbol doesn't resolve (g_viewSupersessionAvailable == false). If it never fires, that fallback doesn't exist.

You have the logging to settle this in one session — Wh_Log the queue-time foreground next to request.hwnd in QueueRescue and check whether they ever differ across taskbar clicks, tray activations and third-party launchers. If they're always equal, please delete foregroundAtQueue, IsForegroundReplacementOnSourceDesktop and both call sites; that's ~60 lines of machinery on undocumented shell behavior that isn't earning its place. If they do differ in some routes, a one-line comment naming one of those routes would keep the next reader from drawing the conclusion above.

2. A navigation-focus record that never gets its FVP stays consumable for 2 s, and can turn a genuine activation into a click that does nothing.

kNavigationFocusRecordMaxAgeMs = 2000 is the orphan bound for a record that never receives its expected ForegroundViewChanged. ConsumeNavigationSelectedFocus matches purely on identity (view pointer + HWND + PID + TID) with no freshness requirement beyond that 2 s, and the shell keeps one IApplicationView per window, so a later, unrelated activation of the same window matches an orphaned record just as well as the delayed delivery it was meant for.

Concrete sequence, all within 2 s:

1. Win+Ctrl+Right to desktop B — GetNewForegroundViewForDesktopSwitch records a focus token for W_B at generation N, and the expected FVP never arrives (focus refused, view torn down, whatever the orphan bound exists for).
2. Win+Ctrl+Left back to desktop A — generation becomes N+1.
3. Click the taskbar button for W_B.

The FVP for that click consumes the stale generation-N token, requestedId == navigationFocus.desktopId (both desktop B), and navigationFocus.generation < currentNavigationGeneration, so SwitchDesktopInternal_Hook takes the "stale navigation-selected focus" branch: return S_OK with no switch and no rescue queued. The user's click produces nothing at all — the exact failure this mod exists to prevent.

The fix is a freshness bound rather than new state. The case the history exists for is a delivery that lags one navigation, so either tighten the token lifetime to something on that order:

// A delayed FVP for a navigation-selected view arrives within one
// navigation of its DesktopChanged; anything older is not a delayed
// delivery and must not classify a fresh activation.
static constexpr ULONGLONG kNavigationFocusRecordMaxAgeMs = 300;

or drop tokens more than one generation behind the current one when a new outermost DesktopChanged begins (you already have DiscardNavigationFocusGeneration for the failed-navigation case). Either way the ~143 ms delayed-delivery case you measured is still covered, and the two-navigations-ago token can no longer swallow a click.

Optional improvements

Minor polish — none of this affects users, so it's your call.

• The README change from last round didn't land. Your reply said the README "again explicitly notes that a superseding view is independently verified to belong to the current desktop", but the file is byte-identical to the previous revision in that region — the sentence "A newly-added same-process view is still independently verified to belong to the current desktop before it can cancel a pending move." is still missing from the second ### Notes bullet, and the trailing space after worthwhile. is still there. Probably a lost edit rather than a decision.

• @name uses a typographic apostrophe (U+2019). Don’t Pull Me to Another Desktop — no other mod in the catalog uses one in @name (several use it in README prose). A plain ' is safer for search and for anything that generates URLs or filenames from the name. Raised last round and not mentioned in your reply, so re-flagging in case it was missed rather than declined.

• ForegroundViewChanged_Hook restores its TLS by hand. The four saves and --g_foregroundPolicyDepth are skipped if g_foregroundViewChangedOriginal unwinds (an SEH exception from shell code), and a leaked g_foregroundPolicyDepth > 0 would make every subsequent SwitchDesktopInternal on that thread look FVP-attributed for the rest of the process's life. A small RAII guard holding the four values and decrementing in its destructor removes the failure mode and reads better than the save/restore pairs.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

• The worker's own failure paths also swallow the activation. Beyond the three fail-stationary branches in SwitchDesktopInternal_Hook discussed in earlier rounds, ProcessRescueRequest can reach GetViewForHwnd failed or MoveViewToDesktop failed after the switch was already suppressed. Those aren't superseded navigations — they're plain failures — and the result is still a click that does nothing. There's no clean recovery from the worker apartment (replaying the internal switch with a captured pThis would be worse), so this is an FYI rather than a request: if "clicking the taskbar button sometimes does nothing" reports show up, these two log lines belong on the checklist alongside finding 2.

• The 50/200 ms split and the 30-attempt startup budget are unchanged, per your reasoning last round. Noting only that both remain as previously discussed, not asking for a change.

---

Next steps:

• /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
• /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

[Meteony]

/ai-review

[windhawk-reviewer]

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.

---

Both findings from last round are properly closed: foregroundAtQueue / IsForegroundReplacementOnSourceDesktop are gone rather than papered over, and ConsumeNavigationSelectedFocus now gates an older-generation token on g_activeNavigationGeneration == currentGeneration && record.generation + 1 == currentGeneration, so a two-navigations-ago token can no longer swallow a click. The @name apostrophe is fixed. I also checked the new pieces that CI can't: the rollbackApps block matches the code exactly in both directions (rollbackApps[%d].executable → Wh_GetStringSetting, the two *Ms numbers → Wh_GetIntSetting, nothing declared-but-unread or read-but-undeclared), kVtableGetDesktops = 7 agrees with mods/virtual-desktop-helper.wh.cpp for the same 24H2 IID (and skipping the older HMONITOR-taking interfaces is the right call), the new g_rollbackEvent / g_preMoveEvidenceEvent handles are created before the worker starts and closed only after it is joined, and no lock is held across a cross-apartment COM call in either direction. One item below, about the shipped default.

Turning replacement handling on by default for Windows Terminal makes every user pay for a heuristic that you yourself concluded can't be made authoritative.

Your commit message says it: "make rollback opt-in only since there's no way to authoritatively tell if it's a replacement window". But the shipped default isn't opt-in — rollbackApps ships with WindowsTerminal.exe, preMoveDelayMs: 500, rollbackWatchMs: 2000, so every user gets both mechanisms active on WT out of the box. Two consequences:

The rollback can undo a summon the user wanted. ValidateRollbackCandidate requires only: a different same-PID top-level window, both windows on the source desktop, same navigation generation, and the candidate having reached ForegroundViewChanged. A new window the user opens satisfies all of that:

1. On desktop A, click the taskbar button for a WT window W1 living on desktop B → the mod summons W1 to A and ArmRollbackWatch arms a 2 s watch.
2. Within those 2 s the user presses Ctrl+Shift+N in W1 (or runs wt from another terminal, or uses the jump list) → WT creates W2 on desktop A.
3. OnViewAddedInternal records W2 as the candidate, W2 becomes foreground → candidateForegroundConfirmed = true, ValidateRollbackCandidate passes, and ProcessRollbackWatch moves W1 back to desktop B.

The window the user deliberately summoned silently disappears to another desktop, with nothing to indicate why. There's no navigation generation change and no desktop change to catch it, because the user never navigated.

The pre-move grace adds unconditional latency. WaitForPreMoveGrace only returns early when a superseder is confirmed and validated; with no replacement it waits out the full preMoveDelayMs and then summons. So on the default config, every ordinary WT summon — the mod's headline action — is delayed by half a second.

Simplest fix: ship rollbackApps empty, so the README's "Those apps can be enabled for replacement handling in Settings" is literally what happens, and users who hit the WT launch pattern opt in knowingly. If you'd rather keep a default entry, the false positive is worth narrowing regardless — the distinguishing feature of a real replacement is that it appears immediately after the activation, not anywhere inside a 2 s window. You already carry the timestamps for this and currently never read them (RollbackWatch::candidateObservedAt is written in ArmRollbackWatch and ObserveViewAddedForRollbackWatches and read nowhere):

// A replacement window is created as part of the same activation. A window
// that shows up hundreds of ms later is the user opening a window, not the
// app substituting one.
static constexpr ULONGLONG kMaxReplacementLagMs = 400;

const ULONGLONG reference = watch.armedAt;
const ULONGLONG lag = watch.candidateObservedAt > reference
                          ? watch.candidateObservedAt - reference
                          : 0;
if (lag > kMaxReplacementLagMs) {
    return false;
}

(candidateObservedAt can also be inherited from before the move via supersedingObservedAt, which is why the clamp above is one-sided.) That keeps the real WT case — where the new window follows within a few dozen ms — and drops the user-initiated one. preMoveDelayMs could then also come down a lot, since the rollback watch already covers anything the grace period misses.

Optional improvements

Minor polish — none of this affects users, so it's your call.

• Leftover duplicate check in ProcessRescueRequest. Deleting IsForegroundReplacementOnSourceDesktop left two identical IsRescueGenerationCurrent(request) checks with nothing between them — one right after GetViewForHwnd succeeds, and one 13 lines later under the comment "Re-check both generation ownership and source desktop immediately before committing the move". The second is a no-op now; drop it and keep the comment on the commitDesktop re-read that follows.

• Dead observation timestamps. Besides candidateObservedAt above, RescueRequest::supersedingObservedAt is written in ObserveViewAddedForPendingRescues and read only to copy into candidateObservedAt, and ViewAddedMatchesRescueRequest's observedAt parameter is now down to a !observedAt check after the kRescueViewSupersessionMs bound was removed. Either give them a job (the freshness bound above) or delete them — right now they read like a window check that's still enforced.

• ForegroundViewChanged_Hook restores its five TLS values by hand. Raised last round; re-flagging because the consequence grew. g_foregroundPolicyRescueSequence is now the only thing that releases a queued rescue, so if g_foregroundViewChangedOriginal unwinds, the request keeps foregroundPolicyReturned == false — and since TakePendingRequest only ever inspects g_rescueQueue[g_rescueQueueHead], that one entry blocks every subsequent rescue for the life of the process, not just its own. A small RAII guard holding the five values (and decrementing g_foregroundPolicyDepth) in its destructor closes it.

• TakePendingRequest could scan instead of only checking the head. Independent of the above: requests are independent of each other, so releasing them in arrival order buys nothing, and scanning g_rescueQueueCount entries for the first foregroundPolicyReturned one removes the head-of-line coupling entirely.

• Two unreachable branches in LoadSettings. Wh_GetStringSetting never returns NULL (it returns L""), so the if (raw) inside the !raw || !*raw block is dead; and after that block raw is non-empty, so wcsncpy_s always writes at least one character and if (!policy.executable[0]) continue; can't fire.

• 64 KB of stack for a file name. GetRollbackAppPolicy declares wchar_t imagePath[32768] per call. It's on the worker so it's harmless, but QueryFullProcessImageNameW with a MAX_PATH-sized buffer (retried on ERROR_INSUFFICIENT_BUFFER, or just accepted as a miss) is plenty for matching an executable name against a MAX_PATH setting field.

• Settings labels are implementation vocabulary. "Apps for replacement handling", "Debounce delay (ms)", "Rollback validity window (ms)" describe the machinery; a user configuring this can't tell what "rollback" or "debounce" refer to. Something closer to "Apps that open a new window instead of reusing an old one" / "Wait before summoning (ms)" / "Undo the summon if a new window appears within (ms)" would map to what they observe. Worth also mentioning the caps (kMaxPreMoveDelayMs = 2000, kMaxRollbackWatchMs = 10000) in the $descriptions, since larger values are silently clamped.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

• A second VIEWADD un-confirms the first. ObserveViewAddedForRollbackWatches unconditionally overwrites candidateViewIdentity/candidateHwnd and resets candidateForegroundConfirmed = false. If a genuine replacement is confirmed and any other same-process view is added before the worker drains g_rollbackEvent, TakeConfirmedRollbackWatch finds the watch unconfirmed and skips it. It fails in the safe direction (no rollback), so it's an FYI — but keeping the confirmed candidate instead of clobbering it would make the feature less order-dependent.

• WaitForPreMoveGrace locks onto the dequeue-time candidate. When request.supersedingForegroundConfirmed is already true in the local copy, the loop uses that candidate on every iteration and never falls through to GetConfirmedActiveSuperseder. If ValidateObservedSuperseder rejects it, a newer confirmed superseder arriving during the grace period can't be seen, and the summon proceeds. Rare, and again the safe direction.

• The worker's own failure paths still swallow the activation (GetViewForHwnd failed, MoveViewToDesktop failed), as discussed in earlier rounds — noting only that they're unchanged, not asking for a change.

---

Next steps:

• /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
• /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

[Meteony]

/ready-for-reviewer

[Meteony]

Thanks. The theoretical manual-new-window false positive is understood, but I don’t think making the Windows Terminal default empty or adding a short “must appear immediately” cutoff would be an improvement.

Windows Terminal isn’t included as a speculative heuristic target. It is the concrete application for which this compatibility path was developed and tested. With Windows Terminal configured as the Default Terminal, stock Windows reproducibly does:

activate pre-existing Terminal window on another desktop -> attempt desktop switch -> asynchronously create the actually requested Terminal window on the current desktop.

During development I instrumented that sequence directly through the shell hooks: the old Terminal view reaches ForegroundViewChanged first, followed later by OnViewAddedInternal and ForegroundViewChanged for the newly-created same-process Terminal view.

Importantly, the replacement timing was not stable. A typical observed run was around 143 ms, but later tests on the same machine exceeded the former 200 ms supersession window. That was the reason the implementation was changed away from treating a fixed timeout as proof that no replacement is coming. Therefore I don’t think narrowing rollback by assuming a genuine replacement must appear “immediately” is sound; it would reintroduce the exact load-dependent failure that this revision removed.

The per-app list is intended as an allowlist for behavior whose causality cannot be proven generically, rather than a requirement that every affected application start disabled. Unlisted applications retain the deterministic summon-only path. Windows Terminal is shipped in the list because it is a known, reproduced affected application, not because package/process identity is being used as generic replacement evidence.

The two timing values also have deliberately non-causal meanings:

preMoveDelayMs is only a visual-churn optimization. If strong replacement evidence arrives during that period, the old window never needs to be moved. Expiry does not mean “this was not a replacement”; the normal summon proceeds.
rollbackWatchMs is a bounded validity period for later positive OnViewAddedInternal + exact-view ForegroundViewChanged evidence. It adds no latency after the summon.

The manual-new-window sequence described in the review is theoretically possible for any app placed on the allowlist; that unavoidable ambiguity is why replacement handling is not enabled generically. For Windows Terminal specifically, I think the known real-world handoff and the observed load-dependent delay justify enabling the compatibility policy by default. Users who prefer immediate deterministic summon-only behavior can remove the entry or set both values to zero.