Skip to content

Add Taskbar Countdown Timer mod - #5381

Open
richilp wants to merge 14 commits into
ramensoftware:mainfrom
richilp:main
Open

Add Taskbar Countdown Timer mod#5381
richilp wants to merge 14 commits into
ramensoftware:mainfrom
richilp:main

Conversation

@richilp

@richilp richilp commented Sep 6, 2026

Copy link
Copy Markdown

Adds a lightweight countdown timer directly to the Windows 11 taskbar.

Features:

  • Custom reminder text
  • Live countdown displayed in the taskbar
  • Cancel active timer
  • Snooze for a custom number of minutes
  • Completion popup with sound
  • Modern Windows 11-style popup UI
  • No external application required

Tested on Windows 11 25H2 build 26200.9168.

Changelog

Not applicable — this pull request introduces a new mod.

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 windhawk-reviewer Bot added the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Sep 6, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.

@richilp

richilp commented Sep 6, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 6, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.


Nice, self-contained idea, and the taskbar.dll / GetTaskbarXamlRoot plumbing follows the established pattern. The problems are concentrated in the lifecycle: startup, unload, and the dedicated popup thread.

1. The mod never loads after a reboot or an Explorer restart. Wh_ModInit looks for Shell_TrayWnd and returns FALSE if it isn't there yet:

HWND taskbar = FindCurrentProcessTaskbarWnd();
if (!taskbar) {
    Wh_Log(L"ERROR: Taskbar not found");
    return FALSE;
}

Wh_ModInit runs before the target process starts executing, so on a normal boot / explorer.exe restart the taskbar window does not exist yet and this always fails. Returning FALSE is fine in principle because Windhawk retries after a settings change — but this mod has no settings block at all, so there is no retry trigger and the button only ever appears if the user toggles the mod by hand mid-session. That's the "works when enabled manually, gone after reboot" case that can't be merged.

Move the UI work to Wh_ModAfterInit and handle the taskbar appearing later. taskbar-vd-switcher does exactly this for the same SystemTrayFrameGrid insertion — it hooks the system-tray module symbols (with a LoadLibraryExW-in-kernelbase hook for the not-yet-loaded case) and additionally runs a short retry loop:

void Wh_ModAfterInit() {
    ...
    g_retryStopEvent = CreateEventW(nullptr, TRUE, FALSE, nullptr);
    g_retryThread = CreateThread(nullptr, 0, [](void*) -> DWORD {
        for (int i = 0; i < 5 && !g_unloading; i++) {
            if (WaitForSingleObject(g_retryStopEvent, 2000) != WAIT_TIMEOUT) break;
            if (g_buttonGrid || g_unloading) break;
            ApplyAllSettingsOnWindowThread();
        }
        return 0;
    }, nullptr, 0, nullptr);
}

The same mechanism also covers the second half of the problem: the button is currently added exactly once, so it does not come back if the taskbar XAML tree is rebuilt during the session.

2. The window classes are never unregistered — the next load runs a dangling WndProc. Both ShowTimerPopup and ShowFinishedPopup do:

if (!RegisterClassW(&wc) && GetLastError() != ERROR_CLASS_ALREADY_EXISTS) { ... return; }
classRegistered = true;

wc.hInstance is GetModuleHandleW(nullptr) (i.e. explorer.exe, not the mod DLL), so Windows will not auto-unregister the class when the mod is unloaded, and there is no UnregisterClass in Wh_ModUninit. After disabling and re-enabling the mod (or after any mod update), classRegistered is false again, RegisterClassW fails with ERROR_CLASS_ALREADY_EXISTS, the code continues, and CreateWindowExW happily creates a window from the stale class whose lpfnWndProc still points into the previous, now-unmapped mod image. Clicking the Timer button then crashes explorer.exe (or worse).

Swallowing ERROR_CLASS_ALREADY_EXISTS is precisely the workaround that turns a silent failure into a dangling-pointer call. Register fresh on every load and unregister in Wh_ModUninit (after the popup thread has exited). taskbar-ai-quota shows the recovery form:

if (!RegisterClassExW(&cls)) {
    if (GetLastError() != ERROR_CLASS_ALREADY_EXISTS ||
        !UnregisterClassW(kClass, instance) || !RegisterClassExW(&cls)) {
        return 0;
    }
}
...
UnregisterClassW(kClass, instance);  // on teardown

3. The popup thread can outlive the mod image. Wh_ModUninit does:

if (g_timerPopup) { PostMessageW(g_timerPopup, WM_APP_TIMER_SHUTDOWN, 0, 0); }
if (g_timerPopupThread) { WaitForSingleObject(g_timerPopupThread, 2000); CloseHandle(...); }

Windhawk unloads the mod with a single FreeLibrary right after Wh_ModUninit returns, so the thread must be gone by then — its instruction pointer and return address live in the mod image. Three ways this currently isn't guaranteed:

  • The wait is bounded at 2000 ms. On timeout the mod unloads with the thread still running → crash.
  • If the thread was created but hasn't set g_timerPopup yet, no shutdown message is posted at all, so the thread enters its message loop and runs forever; the 2 s wait then always times out.
  • If the user left the MessageBoxW validation dialog open, the popup thread is in a nested modal loop.

Also, the GDI objects (g_popupFont, g_popupEditBrush, g_popupBackgroundBrush) are deleted while that thread may still be painting with them.

Make the shutdown deterministic and then wait INFINITE: keep the thread id from CreateThread, set an unloading flag, PostThreadMessageW(threadId, WM_QUIT, 0, 0) (works even before the window exists), have the thread proc bail out if unloading is already set before creating the window, and only delete the GDI objects / unregister the classes after the join. Replacing MessageBoxW with inline validation (or tracking and closing it on shutdown) removes the last blocking point. See taskbar-ai-quota for the PostMessage + WaitForSingleObject(..., INFINITE) shape.

4. The global XAML objects need [[clang::no_destroy]].

static Button g_timerButton{nullptr};
static TextBlock g_timerText{nullptr};
static DispatcherTimer g_countdownTimer{nullptr};

Wh_ModUninit is not called when explorer.exe itself terminates (restart, sign-out, reboot). In that path the CRT still runs these globals' destructors, on the shutdown thread, after the other threads are gone — releasing strong XAML references off the UI thread once the XAML core has been torn down. Since these are nullable WinRT projected types, the bare attribute is the right form (no std::optional wrapper needed):

[[clang::no_destroy]] Button g_timerButton{nullptr};
[[clang::no_destroy]] TextBlock g_timerText{nullptr};
[[clang::no_destroy]] DispatcherTimer g_countdownTimer{nullptr};

The existing explicit cleanup in RemoveTimerButton (assigning nullptr on the taskbar UI thread) must stay — that's what actually releases them on a normal unload. Background and the case-by-case rules: Global objects and process shutdown. See island-media-controls for the same pattern on WinRT globals.

5. The popups are laid out in raw pixels, so they break at non-100% DPI. Window sizes (380 × 235, 380 × 220), every control rect, and the font (CreateFontW(-16, ...)) are hard-coded physical pixels. Explorer is per-monitor-DPI aware, so on a 150%/200% display the popup is roughly half size with clipped text. Scale everything by GetDpiForWindow(hWnd) / 96.0 (and re-scale on WM_DPICHANGED), or at minimum derive the font height with -MulDiv(12, dpi, 72).

Positioning has the same problem: x = taskbarRect.right - width - 20; y = taskbarRect.top - height - 10; assumes a bottom-edge taskbar and does no clamping, so the popup lands off-screen for a top/auto-hidden taskbar and can end up on the wrong monitor. Compute from the taskbar's monitor work area (MonitorFromWindow + GetMonitorInfo) and clamp.

6. GetTaskbarXamlRoot never works on ARM64. The TaskbarHost::FrameHeight prologue check is x64-specific, but here a mismatch is fatal:

if (code[0] == 0x48 && ... ) { offset = code[7]; }
else { /* log + return nullptr */ }

@architecture x86-64 also covers ARM64 devices (natively for shell processes), where the ARM64 prologue never matches these bytes, so the mod silently does nothing there. The canonical implementation guards the pattern match and falls back to the default offset — see taskbar-notification-icon-spacing:

size_t taskbarElementIUnknownOffset = 0x48;
#if defined(_M_X64)
    ... pattern match, else Wh_Log(L"Unsupported TaskbarHost::FrameHeight");
#elif defined(_M_ARM64)
    // Just use the default offset which will hopefully work in most cases.
#endif

Note the default there is 0x48, not the 0x10 used here (which is currently dead code, since the mismatch path returns early).

Optional improvements

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

  • g_secondsRemaining, g_timerPopup and g_timerPopupThread are read and written from the taskbar UI thread, the popup thread and (for the latter two) the arbitrary Wh_ModUninit thread with no synchronization. std::atomic<int> / std::atomic<HWND> is enough here and is cheaper than a mutex.
  • g_timerPopupThreadId is assigned and cleared but never read — dead. (It becomes useful if you adopt the PostThreadMessageW(WM_QUIT) shutdown from item 3.)
  • The hook proc inside RunFromWindowThread calls RegisterWindowMessageW on every dispatched message. A capture-less lambda can still reference the enclosing function's static const UINT message directly, as the canonical snippet does.
  • The comment in RemoveTimerButton refers to "v0.7", but @version is 1.1 — stale.
  • In ShowFinishedPopup, the reminder STATIC is created with a nullptr control id, so it isn't in the ApplyPopupFont id list and keeps the default system font while every other control gets Segoe UI.
  • WM_CTLCOLORBTN isn't handled, so the buttons render with the default light chrome on the dark popup background.
  • The message loop calls TranslateMessage/DispatchMessageW directly, so WS_TABSTOP, Enter (default button) and Esc do nothing. Adding if (!IsDialogMessageW(g_timerPopup, &msg)) around the dispatch would make the popups keyboard-navigable.
  • If FindCurrentProcessTaskbarWnd() returns nullptr, ShowFinishedPopup(nullptr) calls GetWindowRect(nullptr, &taskbarRect), which fails and leaves the rect zeroed, placing the popup at negative coordinates.
  • Worth double-checking whether -lole32 -loleaut32 are actually needed — the mod only does a QueryInterface and C++/WinRT projection calls; -lruntimeobject covers the latter.
  • Consider exposing the defaults as settings (initial minutes, default snooze minutes, button label) — that also gives the mod a settings block, which is what makes a Wh_ModInit FALSE return recoverable.

Functionality notes

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

  • The countdown drifts. The tick handler decrements g_secondsRemaining by 1 per DispatcherTimer tick. DispatcherTimer ticks are dispatched on the taskbar UI thread and are coalesced/dropped when that thread is busy, and they stop entirely across sleep/hibernate — so a 20-minute timer can fire noticeably late, and a timer started before the machine sleeps effectively pauses. Store a deadline instead (GetTickCount64() + seconds * 1000 or std::chrono::steady_clock::now() + ...) and compute the remaining time on each tick; the timer then stays correct regardless of tick jitter, and you can keep the 1 s interval purely for the display refresh.
  • Only the primary Shell_TrayWnd gets the button; secondary taskbars on additional monitors don't. taskbar-vd-switcher handles both if you want parity.
  • A running timer is lost on mod reload / Explorer restart. If you want it to survive, the deadline could be persisted with Wh_SetStringValue / Wh_SetIntValue (Windhawk's own storage — no registry writes).
  • The popups are created with Shell_TrayWnd as their owner, which is a window on a different thread. It works, but it ties popup activation to the taskbar thread's input queue; an unowned WS_EX_TOOLWINDOW | WS_EX_TOPMOST window positioned near the taskbar avoids that coupling.
  • Minutes-only input with a 1440 cap means no sub-minute timers ("2:30") and no multi-day ones; an optional seconds field or an HH:MM:SS parse would widen it. Also, when a timer is running the popup re-displays the remaining time rounded up to whole minutes, so reopening it shows e.g. 5 for 4:02 remaining.


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 windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 6, 2026
@richilp

richilp commented Sep 7, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 7, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.


Nice, self-contained idea, and no existing mod overlaps with it — the closest neighbours (taskbar-clock-customization, taskbar-vd-switcher) do something else entirely. The XAML-injection boilerplate (GetTaskbarXamlRoot, RunFromWindowThread, FindCurrentProcessTaskbarWnd) matches the established mods, and the [[clang::no_destroy]] on the WinRT globals is the right call. The items below are mostly about the retry loop, the popup's GDI handling, and unload safety.

1. The retry thread polls the taskbar UI thread forever, every 5 seconds. RetryThreadProc (line ~2947) loops until unload, and each iteration runs EnumWindows over every top-level window on the desktop, then a blocking SendMessage into the taskbar UI thread that walks the CTaskBand vtable chain, calls GetTaskbarHost, QueryInterfaces the XAML root and does two recursive visual-tree searches (FindChildRecursive, depth 20) — even in the steady state where the button is already there and the only outcome is the if (existing) return; early exit. That's permanent overhead on the shell's UI thread for the whole session, which is the single most commonly rejected pattern in this repo.

Bound the retry and drive the re-add from an event instead. taskbar-vd-switcher does exactly this — at most 5 attempts, 2 s apart, and it stops as soon as the element exists:

g_retryThread = CreateThread(nullptr, 0, [](void*) -> DWORD {
    for (int i = 0; i < 5 && !g_unloading; i++) {
        if (WaitForSingleObject(g_retryStopEvent, 2000) != WAIT_TIMEOUT) break;
        if (g_buttonGrid || g_unloading) break;
        ApplyAllSettingsOnWindowThread();
    }
    return 0;
}, nullptr, 0, nullptr);

For re-adding the button after the taskbar's XAML tree is rebuilt, hook a tray (re)construction point rather than polling — taskbar-vd-switcher hooks winrt::SystemTray::implementation::IconView::IconView in SystemTray.dll and re-injects from there.

2. The popup font is deleted while it is still selected into live controls. FinishedPopupWndProc calls EnsurePopupResources(96) at the top of every message (line ~1257), and EnsurePopupResources deletes and recreates g_popupFont whenever the requested DPI differs from g_popupFontDpi. On any display that isn't 100 %:

  • LayoutFinishedPopup(hWnd, dpi) creates the font at, say, 144 DPI and WM_SETFONTs it onto all six children;
  • the very next message the window receives (WM_WINDOWPOSCHANGED from PositionPopupNearTaskbar, then WM_ERASEBKGND, WM_CTLCOLOR*, …) calls EnsurePopupResources(96)DeleteObject(g_popupFont);
  • the children now hold a dangling HFONT, so the popup renders in the default system font instead of scaled Segoe UI. The same happens again after every WM_DPICHANGED.

TimerPopupWndProc has no such call, so this looks like a leftover. Drop the per-message EnsurePopupResources from FinishedPopupWndProc, and more generally don't DeleteObject a font that live windows still reference — either keep the font keyed to the window's own DPI, or re-apply WM_SETFONT to every child right after recreating it.

3. On unload, the XAML button and DispatcherTimer can be left behind — that crashes Explorer. Wh_ModUninit (line ~3101) only tears them down if FindCurrentProcessTaskbarWnd() returns a window and RunFromWindowThread succeeds, and the return value is discarded:

HWND taskbar = FindCurrentProcessTaskbarWnd();
if (taskbar) {
    RunFromWindowThread(taskbar, RemoveTimerButton, nullptr);
}

If either fails (SetWindowsHookExW can fail; the taskbar window can be missing during an Explorer restart), the Button stays in the tray's visual tree with a Click delegate — and the DispatcherTimer keeps ticking — with both callbacks pointing into the mod image that Windhawk FreeLibrarys the moment Wh_ModUninit returns. The next click or tick then executes unmapped code. It also leaks the no_destroy globals permanently, since the assignment to nullptr is the only release they ever get.

Check the result, and don't give up silently — retry a few times, and log an error if removal genuinely couldn't happen. See https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown (section #4 XAML and other UI-thread objects) for why the explicit release on the owning thread is the only correct teardown here.

4. @architecture amd64 excludes ARM64 users. amd64 means 64-bit Intel/AMD only; ARM64 Windows 11 devices will never load the mod. Every comparable taskbar mod uses x86-64, which also covers ARM64. The blocker is the #error in GetTaskbarXamlRoot — add the ARM64 branch for the TaskbarHost::FrameHeight offset, which is already written for you in adaptive-microphone-icon-visibility.wh.cpp:

#elif defined(_M_ARM64)
    const DWORD* p = (const DWORD*)TaskbarHost_FrameHeight_Original;
    if (p[0] == 0xD503237F && (p[1] & 0xFFC07FFF) == 0xA9807BFD &&
        p[2] == 0x910003FD && (p[3] & 0xFFF00FE0) == 0xF8400C00) {
        offset = (p[3] >> 12) & 0xFF;
    } else {
        Wh_Log(L"Unsupported TaskbarHost::FrameHeight");
    }
#endif

5. Don't continue with a guessed offset when the FrameHeight prologue doesn't match. When the byte pattern fails, the code logs and falls through with the hard-coded offset = 0x48, then dereferences taskbarHostSharedPtr[0] + 0x48 as an IUnknown* and makes a virtual call on it. On a future build where the layout moved, that is an arbitrary pointer dereference and a virtual dispatch through garbage, inside explorer.exe. Return nullptr from GetTaskbarXamlRoot in the else branch instead — losing the button is much better than taking down the shell.

6. Column bookkeeping breaks if another mod also inserts into SystemTrayFrameGrid. AddTimerButton inserts a ColumnDefinition at index 0 and shifts every child by +1; RemoveTimerButton unconditionally does columns.RemoveAt(0) and decrements every child with column > 0. If another mod (taskbar-vd-switcher inserts into the same grid) added its column afterwards, yours is no longer at index 0 — removal then deletes their column and mis-shifts their element. Keep the ColumnDefinition you created in a global, find it with columns.IndexOf(...) on removal, and only shift children whose column is greater than that index.

7. Cross-thread globals are unsynchronized. g_timerPopup, g_finishedPopup, g_timerPopupThread, g_timerPopupThreadId and g_reminder (lines 90–96) are plain globals written on the popup thread and read on the taskbar UI thread (the DispatcherTimer tick reads g_timerPopup every second and posts WM_APP_TIMER_FINISHED to it) and on the Windhawk thread in Wh_ModUninit. Make the HWNDs std::atomic<HWND> and hand the reminder text to the popup thread through the message instead of reading the shared buffer. Note also that IsWindow() on a possibly-stale handle isn't a safe validity check (The Old New Thing) — a recycled HWND would receive your private WM_APP + 2.

8. Popup-thread handshake: closed-handle race and unbounded waits on the shell UI thread. Three related problems around g_popupThreadReadyEvent:

  • OpenTimerPopup runs on the taskbar UI thread and ends with WaitForSingleObject(g_popupThreadReadyEvent, INFINITE) — an unbounded wait in a click handler on the shell's UI thread.
  • The same function CloseHandles and recreates that event (lines ~2420–2435), while Wh_ModUninit may concurrently be inside WaitForSingleObject(g_popupThreadReadyEvent, INFINITE) on it. Waiting on a closed — and possibly recycled — handle can either fail immediately or block forever, hanging the unload.
  • Both joins (g_retryThread, g_timerPopupThread) use a plain WaitForSingleObject(..., INFINITE) while those threads can be blocked in a cross-thread SendMessage via RunFromWindowThread. taskbar-vd-switcher uses the message-pumping form for exactly this reason:
DWORD result;
do {
    result = MsgWaitForMultipleObjects(1, &g_retryThread, FALSE, INFINITE, QS_SENDMESSAGE);
    if (result == WAIT_OBJECT_0 + 1) {
        MSG message;
        PeekMessageW(&message, nullptr, 0, 0, PM_NOREMOVE);
    }
} while (result == WAIT_OBJECT_0 + 1);

Create the ready event once (in Wh_ModAfterInit) rather than recycling it per popup, give the waits a timeout, and use the pumping join.

Optional improvements

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

  • No ==WindhawkModSettings== block at all. Everything is hard-coded: the 20-minute default duration, the 5-minute snooze default, the ⏱ Timer button label, the popup colors and the 380×235 / 380×220 popup sizes. Exposing at least the default duration, the default snooze and the button label as settings would fit how mods in this repo are normally configured.
  • ApplyModernWindowStyle contains two calls that do nothing here. DWMWA_USE_IMMERSIVE_DARK_MODE only affects the non-client caption, which a WS_POPUP | WS_BORDER window doesn't have, and DwmExtendFrameIntoClientArea({1,1,1,1}) is immediately painted over by the opaque WM_ERASEBKGND fill. Only the corner-preference call has a visible effect. Is the rest intentional, or leftover from the AI-assisted draft?
  • The _v12 suffix on the window class names (TaskbarCountdownTimerPopup_v12) looks like a workaround for a stale-class problem that the code already handles properly — UnregisterPopupClasses runs on the popup thread before it exits, and Wh_ModUninit joins that thread, so a plain unversioned name is fine. The ERROR_CLASS_ALREADY_EXISTS → unregister → re-register fallback in RegisterOnePopupClass is a reasonable safety net; keep that.
  • EnsurePopupResources is called with a hard-coded 96 from RegisterOnePopupClass as well (line ~1499). Once finding 2 is fixed this is harmless, but it's simpler to create the brushes eagerly and only ever build the font from a real window DPI.
  • Missing includes for symbols you use. swprintf_s / wcsncpy_s come from <cwchar> and _wtoi from <cstdlib> (the latter is included); add <cwchar> rather than relying on transitive includes.
  • FindCurrentProcessTaskbarWnd() is called on every click, start, cancel and snooze, each time enumerating every top-level window on the desktop. Caching the taskbar HWND (invalidating it when the tree is rebuilt) would avoid that; it matters much less once finding 1 removes the 5-second cadence.

Functionality notes

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

  • The popups won't look like Windows 11. WM_CTLCOLORBTN is ignored by themed push buttons (Explorer runs with the comctl32 v6 manifest), so Start / Cancel timer / Snooze / Dismiss will render as standard light-themed buttons on your dark background, while the labels and edits are dark. If you want the "modern Windows 11-style popup UI" the README promises, the popups would need owner-drawn (BS_OWNERDRAW) buttons — or, since you're already in a XAML-capable process, a XAML flyout anchored to the button.
  • Keyboard handling is a bit unfinished. IsDialogMessageW translates Enter into WM_COMMAND/IDOK and Esc into IDCANCEL, and neither wndproc handles them, so Enter doesn't start the timer and Esc doesn't dismiss the popup. Giving the Start button BS_DEFPUSHBUTTON and handling IDOK/IDCANCEL in TimerPopupWndProc would make it behave like a normal dialog. Also, when both popups exist, IsDialogMessageW is only ever called with g_finishedPopup, so tab navigation in the timer popup stops working while a finished popup is up.
  • The countdown depends on the XAML DispatcherTimer running on the taskbar UI thread. That's fine for displaying the remaining time, but it makes the alarm itself dependent on the taskbar's XAML tree being alive and pumping. A waitable timer or a WaitForSingleObject timeout on the popup thread as the authoritative deadline (with the DispatcherTimer purely for the text) would be more robust.
  • Timer state is lost on an Explorer restart or a mod reload. Persisting the deadline with Wh_SetIntValue / Wh_GetIntValue and restoring it in Wh_ModAfterInit would let a running timer survive both.
  • Duration granularity is whole minutes only. A "5 seconds" test or a 90-second timer isn't expressible, and ResetPopupForNewTimer has to round the remaining time up to the next minute to display it. Accepting mm:ss or a seconds field would remove that rounding.
  • The finished popup is WS_EX_TOPMOST + SetForegroundWindow. It will pop over fullscreen content; since it's also WS_EX_TOOLWINDOW it won't show in Alt-Tab, so if SetForegroundWindow is refused (which it often is from a background thread) the user gets a topmost window they can't reach through the task switcher. Worth a quick check on a couple of foreground scenarios.


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 windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 7, 2026
@richilp

richilp commented Sep 7, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 7, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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 overall structure is solid — the XAML injection follows the established pattern from mods/taskbar-vd-switcher.wh.cpp and mods/adaptive-microphone-icon-visibility.wh.cpp, teardown joins both worker threads and unregisters the window classes, and the [[clang::no_destroy]] globals are handled deliberately. A few real problems, though:

1. The custom reminder text never reaches the completion popup. This is the mod's headline feature, and it's broken on both paths:

  • The normal expiry path posts the message with an empty payload:

    PostMessageW(g_timerPopup.load(), WM_APP_TIMER_FINISHED, 0, 0);

    WM_APP_TIMER_FINISHED's handler reads lParam as a std::wstring*, so it always falls back to L"Timer finished". The reminder is sitting right there in g_reminder, and the other call site already does the right thing — use PostStringMessage(g_timerPopup.load(), WM_APP_TIMER_FINISHED, g_reminder).

  • The snooze handler reads a control that is never created:

    GetWindowTextW(GetDlgItem(hWnd, IDC_FINISHED_REMINDER), finishedReminder, ARRAYSIZE(finishedReminder));

    IDC_FINISHED_REMINDER (1099) doesn't exist — ShowFinishedPopup creates the reminder static with IDC_FINISHED_TEXT (1105). GetDlgItem returns NULL, GetWindowTextW writes nothing, and the snoozed timer is renamed to "Timer finished". Read IDC_FINISHED_TEXT instead (or keep the text in a variable rather than round-tripping it through a control).

2. The timer popup is sized and positioned once, at 96 DPI, on the primary monitor. TimerPopupThreadProc calls ShowTimerPopup(nullptr), so GetPopupDpi(nullptr) returns 96 and the window is created 380×235 physical pixels with children laid out at 96 DPI. PositionPopupNearTaskbar(hWnd, nullptr, ...) then falls back to MonitorFromWindow(hWnd) — and since CW_USEDEFAULT is ignored for WS_POPUP windows, the window is at (0, 0), i.e. always the primary monitor.

When the popup is later shown, WM_APP_TIMER_SHOW re-lays out the children at the real DPI but calls SetWindowPos with SWP_NOMOVE | SWP_NOSIZE, so the frame stays at its 96-DPI size. On a 150% display that puts the Start/Cancel buttons at x≈292 with width≈247 inside a 380px-wide client area — the buttons are clipped. WM_DPICHANGED doesn't save it either, because the window never crosses a monitor boundary. And if the taskbar is on a secondary monitor, the popup opens on the primary one.

Do the sizing/positioning at show time, against the taskbar window:

case WM_APP_TIMER_SHOW: {
    HWND taskbar = g_taskbarWnd.load();
    UINT dpi = GetPopupDpi(taskbar);
    ResetPopupForNewTimer(hWnd, ...);
    LayoutTimerPopup(hWnd, dpi);
    PositionPopupNearTaskbar(hWnd, taskbar, 380, 235, dpi);
    ...
}

Related: PositionPopupNearTaskbar passes SWP_SHOWWINDOW, so creating the popup at mod load briefly flashes an empty window in the corner of the screen before ShowWindow(hWnd, SW_HIDE) runs. Either drop SWP_SHOWWINDOW from that helper and show explicitly at the call sites that want it, or don't position the window at creation time at all.

3. HandleLoadedModuleIfSystemTray has a check-then-set race, and there's no Wh_ModAfterInit fallback.

if (g_systemTrayModuleHooked.load() || GetSystemTrayModuleHandle() != module) {
    return;
}
if (HookSystemTraySymbols(module)) {
    g_systemTrayModuleHooked.store(true);
    Wh_ApplyHookOperations();
}

LoadLibraryExW can run concurrently on several threads, so two of them can both pass the load() check and both call WindhawkUtils::HookSymbols for the same module — which invalidates the symbol cache and forces a re-resolution, on top of setting the same function hook twice. Use the atomic form, as in adaptive-microphone-icon-visibility.wh.cpp#L663:

if (!g_systemTrayModuleHooked && GetSystemTrayModuleHandle() == module &&
    !g_systemTrayModuleHooked.exchange(true)) {
    if (HookSystemTraySymbols(module)) {
        Wh_ApplyHookOperations();
    }
}

Also, the LoadLibraryExW hook only becomes active when Wh_ModInit returns, so a system tray module that loads in that window is missed and the tray-rebuild hook silently never installs. The same reference mod covers this with a re-check in Wh_ModAfterInit (#L968) — worth copying.

Optional improvements

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

  • [[clang::no_destroy]] std::list<FrameworkElement::Loaded_revoker> g_loadedRevokers; — the suppression itself is justified (revoking XAML events off the UI thread at process shutdown is exactly the hazard), but the bare attribute is only meant for nullable/handle-like types (WinRT projections, smart pointers, raw handles). For a container the documented shape is the std::optional wrapper, so the explicit release is an unambiguous full destruction:

    [[clang::no_destroy]] static std::optional<
        std::list<FrameworkElement::Loaded_revoker>> g_loadedRevokers;

    with g_loadedRevokers.emplace() on first use and g_loadedRevokers.reset() in RemoveTimerButton instead of .clear(). See Global objects and process shutdown. The other no_destroy globals (g_timerButton, g_timerText, g_countdownTimer, g_timerColumn) are WinRT projected types, so the bare attribute is correct there.

  • Dead code: WM_APP_TIMER_SHUTDOWN is defined and handled but never posted, and IDC_FINISHED_REMINDER is only used in the broken GetDlgItem call from item 1. #include <winrt/Windows.UI.Xaml.Controls.Primitives.h> also appears unused.

  • RegisterOnePopupClass's ERROR_CLASS_ALREADY_EXISTS recovery can't actually work across mod reloads: UnregisterClassW(className, g_modInstance) requires the hInstance that registered the class, and a leftover registration would carry the previous module's handle, so the unregister fails and the whole popup thread bails out. The real fix is already in place (the thread unregisters both classes on exit, so the stale-class case shouldn't arise) — the fallback just gives a false sense of safety and could be dropped, or reduced to a log line.

  • PostThreadMessageW(popupThreadId, WM_QUIT, 0, 0) in Wh_ModUninit ignores its return value and is followed by PumpWaitForThread(popupThread) with an INFINITE timeout. In practice the ready event guarantees the queue exists by then, so this shouldn't hang — but checking the return value (and falling back to PostMessageW(g_timerPopup, WM_APP_TIMER_SHUTDOWN, 0, 0), which is presumably what that message was for) would make the teardown robust by construction.

  • FindCurrentProcessTaskbarWnd() runs a full EnumWindows sweep on every button click, start, cancel and snooze. g_taskbarWnd is already cached — use it and fall back to the enumeration only when IsWindow fails, the way Wh_ModUninit does.

  • The RetryThreadProc polling loop (5 attempts, 2 s apart) largely overlaps with the IconView::IconView hook, which already re-runs the injection when the tray rebuilds its XAML tree. Worth checking whether the retry thread is still needed once the hook is installed reliably (item 3).

  • The mod has no ==WindhawkModSettings== block, so the default duration (20 min), the default snooze (5 min), the button label and the completion sound are all hard-coded. Exposing at least the two durations as settings would be a cheap win.

Functionality notes

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

  • The popup colors are hard-coded dark (RGB(32, 32, 32)) and DWMWA_USE_IMMERSIVE_DARK_MODE is forced to TRUE unconditionally. On a light-themed system the popup will look out of place. Reading AppsUseLightTheme (or at least offering a setting) would fit better.

  • The taskbar button uses the default XAML Button style, which draws a filled, bordered chip next to the otherwise flat tray icons. Applying the tray's own button style (or clearing the background/border) would blend in better — see how taskbar-vd-switcher.wh.cpp styles its injected buttons.

  • Only the primary taskbar gets the button — FindCurrentProcessTaskbarWnd matches Shell_TrayWnd only, so Shell_SecondaryTrayWnd monitors have none. That may well be intentional; worth stating in the README if so.

  • Timer state lives only in memory, so a running timer is silently lost on an Explorer restart or a mod reload. If you want it to survive, persist the deadline with Wh_SetIntValue/Wh_GetIntValue (that's the sanctioned way to store mod state — no registry writes of your own).

  • Keyboard handling: the popups are dispatched through IsDialogMessageW, but neither window proc handles the IDOK/IDCANCEL commands it generates, so Enter doesn't start the timer and Esc doesn't close the popup. Handling those two would make the dialogs feel native.

  • If a second timer finishes while the completion popup from a previous one is still open, ShowFinishedPopup just calls SetForegroundWindow on the existing window — the stale reminder text stays on screen. Updating IDC_FINISHED_TEXT on that path would fix it.

  • StartCountdownOnTaskbarThread returns silently when g_timerText/g_countdownTimer are null (e.g. the tray rebuilt and the button hasn't been re-injected yet). The snooze popup is destroyed regardless, so the user gets no feedback that the snooze didn't take.


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 windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 7, 2026
@richilp

richilp commented Sep 7, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 7, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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-09-07 19:07 UTC (in 11 hours) to get another one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 7, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.


Nice mod overall — the taskbar integration is done with the established symbol/XAML approach, the teardown is thought through (PumpWaitForThread to avoid the classic uninit deadlock is a good touch), settings and code are in sync, and state is persisted through Windhawk's own storage rather than the registry. A few things need attention before merge:

1. @license MIT is incompatible with the GPL-3.0 code this mod reuses

A large part of the infrastructure here is a near-verbatim (reformatted) copy of two of m417z's mods, both published under GPL-3.0:

  • GetTaskbarXamlRoot (line 377), the TaskbarHost::FrameHeight prologue scan used to find the XAML element offset — including the identical x64 byte checks and ARM64 instruction masks (lines 470-519) — and RunFromWindowThread (line 567) come from taskbar-multirow.wh.cpp (license header).
  • HookTaskbarDllSymbols and its symbol array (line 322), GetModuleVersionInfo (line 3168), GetSystemTrayModuleHandle with the 2604 version check (line 3218), HandleLoadedModuleIfSystemTray (line 3347), LoadLibraryExW_Hook (line 3379) and the IconView::IconView hook with the Loaded_revoker list (line 3257) come from taskbar-notification-icon-spacing.wh.cpp (license header).

Please change @license to GPL-3.0 and add an attribution comment naming the source mods, the way taskbar-system-info.wh.cpp does for the same borrowed code.

2. The completion-alert thread can outlive Wh_ModUninit and crash Explorer

In Wh_ModUninit the alert thread is joined at line 3597, but the timer worker thread — which is what creates alert threads — is only joined afterwards, at line 3616. HandleTimerExpiredFromWorker checks g_unloading only on entry (line 2214), so the worker can already be past that check and go on to call ShowFinishedAlert (line 2099), which creates a new alert thread and stores its handle at line 2165 — after g_finishedAlertThread.exchange(nullptr) at line 3598 has already run. That handle is never joined or closed, so the thread is still executing mod code (its message loop, DestroyWindow, UnregisterClassW, its own return path) when Windhawk FreeLibrarys the image → crash in explorer.exe. The same race can also leave the alert thread waiting on g_finishedAlertStopEvent after line 3675 closed it.

Fix: signal and join the timer worker (and the retry thread) first, then deal with the alert thread; and make ShowFinishedAlert refuse to start a thread when g_unloading is set:

static bool ShowFinishedAlert(const wchar_t* reminder) {
    if (g_unloading.load()) {
        return false;
    }
    ...
}

Everything that can still run mod code must be joined before Wh_ModUninit returns — see Global objects and process shutdown for the surrounding rules.

3. existing == g_timerButton doesn't test XAML object identity

Line 2702 compares a FrameworkElement (from children.GetAt(i).try_as<FrameworkElement>()) with a Button. C++/WinRT's operator== compares the default interface ABI pointers, which differ between IFrameworkElement and IButton even for one and the same object — so this branch never matches its own button. The consequence is that every tray IconView Loaded event (line 3290 sets g_buttonInjected = false and re-runs ApplyTimerButtonIfAvailable) treats the mod's own button as "stale from a previous mod instance", removes it, tears down the flyout and the DispatcherTimer via ReleaseOwnedXamlForRebuild, and rebuilds everything — closing an open flyout and churning the tray grid's column definitions.

Compare COM identity explicitly, as taskbar-numberer.wh.cpp does:

auto sameObject = [](auto const& a, auto const& b) {
    return a && b &&
           winrt::get_abi(a.template as<winrt::Windows::Foundation::IUnknown>()) ==
           winrt::get_abi(b.template as<winrt::Windows::Foundation::IUnknown>());
};

if (existing && g_timerButton && sameObject(existing, g_timerButton)) { ... }

4. The flyout's Start / Cancel timer click handlers are never revoked

RemoveTimerButtonImpl carefully revokes the taskbar button's Click and the DispatcherTimer's Tick — its own comment (line 2936) says nothing is allowed to leave a live delegate behind — but the two flyout buttons register Click at lines 1193 and 1238 without keeping the tokens, and HideAndReleaseFlyouts (line 912) only nulls the globals. Flyout::Hide() is asynchronous, so the popup can still hold the flyout, its panel and both buttons, with delegates pointing into the mod image, past Wh_ModUninit.

Store the tokens next to g_timerButtonClickToken and revoke them in HideAndReleaseFlyouts before releasing the elements.

5. Alert window class: don't continue on ERROR_CLASS_ALREADY_EXISTS, and don't unregister a class you didn't register

Lines 1972-1982 swallow ERROR_CLASS_ALREADY_EXISTS and then create a window of that class. If the class ever survives a previous mod image (an unregister that failed because a window still existed, an early return, a load that raced), its lpfnWndProc points into unmapped memory and CreateWindowExW dispatches straight into it. And when CreateWindowExW fails on that path, line 2013 calls UnregisterClassW on a class this instance never registered, tearing it out from under whoever owns it.

Register the class once in Wh_ModInit (with the mod's own HINSTANCE) and UnregisterClass it in Wh_ModUninit, and treat ERROR_CLASS_ALREADY_EXISTS as a hard failure rather than something to work around.

Optional improvements

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

  • Dead state. g_secondsRemaining (line 128) and g_pendingFinished (line 130) are written in nine places each and never read anywhere. They look like leftovers from an earlier design; removing them drops ~18 lines of bookkeeping and two atomics.
  • Wh_GetStringSetting never returns NULL — it returns L"" when unset or on error, so the label && check at line 714 is redundant. WindhawkUtils::StringSetting (RAII) is also preferable to the raw get + Wh_FreeStringSetting pair:
    WindhawkUtils::StringSetting label = WindhawkUtils::StringSetting::make(L"buttonLabel");
    if (*label) {
        updated.buttonLabel = label.get();
    }
  • Clamp inconsistency in LoadSettings. maximumMinutes is clamped to 10080 (line 693), but defaultMinutes / defaultSnoozeMinutes are clamped to a hard-coded 1440 (lines 680, 688), so a user who raises the maximum past a day still can't set a matching default. Clamp the defaults against updated.maximumMinutes instead of a literal.
  • EnsureTimerWorkerStarted (line 2431) is a check-then-act on g_timerWorkerStarted. Today every first call reaches it from the taskbar thread, so it isn't a live bug, but if that ever changes two worker threads get created and g_timerWorkerThread is overwritten — the orphaned one is never joined. std::call_once, or a compare_exchange_strong on a tri-state, closes it.
  • EnsureFinishedAlertResources (line 1388) deletes the old font and brushes before anything stops using them. On WM_DPICHANGED the children still have the old font selected when DeleteObject runs (line 1425), so the call fails and the object leaks; the same applies to the brushes returned from WM_CTLCOLOR*. Create the new objects, install them (WM_SETFONT / repaint), then delete the old ones.
  • The teardown retry loop in Wh_ModUninit (lines 3635-3646) doesn't measure what it thinks it does. TryRemoveTimerXaml returns whether RunFromWindowThread delivered the call, not whether removal succeeded — and since Wh_ModBeforeUninit already ran the same teardown, the ten Sleep(50) retries mostly just add up to half a second of unload latency. One attempt (or a real success flag set inside RemoveTimerButtonImpl) would be clearer.
  • size_t offset = 0x48; (line 468) is a dead initializer — both architecture branches either assign offset or return.
  • WM_ERASEBKGND (line 1787) passes g_finishedAlertBackgroundBrush to FillRect without a null check; it's created before the window is shown, but a null brush there would be a silent no-op that's hard to diagnose.
  • Injection retry cost. Every tray IconView Loaded runs the full path: SetWindowsHookEx + SendMessage (RunFromWindowThread), GetTaskbarXamlRoot, and a depth-20 recursive FindChildRecursive walk. That's per tray icon, and tray icons load fairly often. Once item 3 above is fixed the work is at least idempotent, but a cheaper pre-check (e.g. bail out if VisualTreeHelper::GetParent(g_timerButton) is still the tray grid) would avoid most of it.
  • BOOL Wh_ModSettingsChanged isn't used — good, the void form is already the right choice here. Nothing to change; just noting it in case a future edit reaches for the bReload variant.

Functionality notes

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

  • Restoring a long-expired timer fires the alert immediately. RestorePersistedTimer (line 2402) pops the completion alert for any stored deadline that's already in the past, with no staleness bound. Re-enabling the mod after a week — or an Explorer crash the user never noticed — brings up a "Timer finished" window for something that expired days ago. A cutoff (say, ignore deadlines more than a few hours old and just clear them) would make the restore behavior less surprising.
  • The alert doesn't follow a theme change while it's open. g_finishedAlertDark is sampled at WM_CREATE / WM_DPICHANGED only. Handling WM_SETTINGCHANGE with lParam == L"ImmersiveColorSet" and re-running ApplyFinishedAlertTheme would keep a long-lived alert consistent.
  • Primary taskbar only. The README says so, which is fine — but note the secondary taskbars run on the same Explorer UI thread, so extending this later is mostly a matter of enumerating Shell_SecondaryTrayWnd and using GetSecondaryTaskbarXamlRoot; see taskbar-multirow.wh.cpp for that variant.
  • A second completion is silently dropped. ShowFinishedAlert returns true when a previous alert thread is still running (line 2113) but its window hasn't appeared yet, so that completion produces no UI at all. With one timer at a time this is hard to hit; worth a Wh_Log at minimum so it isn't invisible if it ever does.
  • Snooze validation writes the error into the label. Rewriting IDC_ALERT_SNOOZE_LABEL to Snooze minutes (1-%d): (line 1891) works, but the label never reverts once the user corrects the value. A MessageBeep plus reselecting the text (both of which you already do) is arguably enough on its own.


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 windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 10, 2026
@richilp

richilp commented Sep 10, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 10, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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-09-10 09:32 UTC (in 11 minutes) to get another one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 10, 2026
@richilp

richilp commented Sep 10, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 10, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.


Overall this is a solid submission — the teardown ordering in particular is careful and correct (threads joined before the handles/GDI objects they use are closed, XAML released on the taskbar thread, [[clang::no_destroy]] used correctly on the WinRT projected types with explicit nullptr releases, window class registered/unregistered per load). Two things about how the button gets injected are worth changing.

1. Injection and re-injection rely on polling plus an incidental IconView hook — use TrayUI::StartTaskbar instead

Right now the button is injected once from Wh_ModAfterInit, and if the taskbar isn't ready yet, RetryThreadProc (line 3196) polls 5 times at 2 s intervals and then gives up permanently. Re-injection after the tray tree is rebuilt is driven solely by the winrt::SystemTray::implementation::IconView::IconView hook and its Loaded handler. Both signals are indirect, and each has a failure mode users will hit:

  • On a cold boot the taskbar's XAML island can take longer than ~10 s to come up on a slow machine or one with a lot of startup apps — after that the retry thread is gone and the button never appears until Explorer is restarted.
  • If the tray tree is rebuilt without a new IconView being constructed, the button disappears for good.
  • If HookSystemTraySymbols fails on a future build (symbol drift), the IconView signal is gone entirely and the retry thread is the only thing left.

taskbar.dll exposes the canonical "the taskbar is up" hook, and you already resolve symbols from that module — just add it to the existing taskbarDllHooks array (line 347):

using TrayUI_StartTaskbar_t = void (WINAPI*)(void* pThis);
static TrayUI_StartTaskbar_t TrayUI_StartTaskbar_Original;

void WINAPI TrayUI_StartTaskbar_Hook(void* pThis) {
    TrayUI_StartTaskbar_Original(pThis);
    if (!g_unloading.load()) {
        ApplyTimerButtonIfAvailable();
    }
}

// ...
{
    {LR"(public: virtual void __cdecl TrayUI::StartTaskbar(void))"},
    &TrayUI_StartTaskbar_Original,
    TrayUI_StartTaskbar_Hook,
},

This is the pattern used by taskbar-folder-menus, taskbar-multi-tray and mutealert. With a deterministic signal in place, RetryThreadProc, g_retryStopEvent, g_retryThread and the corresponding join in Wh_ModUninit can all be deleted — that's a whole thread's worth of lifetime management removed along with the polling.

2. Every tray icon load re-resolves the entire taskbar XAML root on Explorer's UI thread

The Loaded handler at line 3386 does:

g_buttonInjected.store(false);
ApplyTimerButtonIfAvailable();

unconditionally, so AddTimerButtonImpl runs GetTaskbarXamlRoot()GetProp + up to 20 vftable probes + CTaskBand::GetTaskbarHost + instruction-byte parsing of TaskbarHost::FrameHeight + _Decref — followed by FindChildRecursive(content, ..., maxDepth = 20) over the whole taskbar visual tree, only to discover at line 2785 that the button is already the one it created and return. That happens for every IconView construction on the taskbar; opening the notification-area overflow constructs a batch of them at once, all on Explorer's UI thread.

An early bail-out at the top of AddTimerButtonImpl (right after the g_unloading check) covers all callers:

if (g_timerButton && VisualTreeHelper::GetParent(g_timerButton)) {
    // Still attached to a live tree, nothing to rebuild.
    g_buttonInjected.store(true);
    return;
}
Optional improvements

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

  • Missing #include <memory>. std::unique_ptr (line 2023) and std::make_unique (line 2187) are used but only reached transitively through other headers; include the header for the symbols you actually use.
  • Use WindhawkUtils::StringSetting instead of Wh_GetStringSetting + Wh_FreeStringSetting in LoadSettings (line 712) — it's RAII and can't leak on an early return:
    auto label = WindhawkUtils::StringSetting::make(L"buttonLabel");
    if (*label) {
        updated.buttonLabel = label.get();
    }
  • Create the synchronization objects once in Wh_ModInit rather than lazily from whichever thread happens to call first. Today EnsureTimerWorkerStarted (line 2491) is not itself thread-safe — the g_timerWorkerStarted check and the CreateThread aren't atomic — and g_timerStopEvent / g_timerRearmEvent / g_timerWaitable / g_timerWorkerThread / g_finishedAlertStopEvent are plain (non-atomic) globals written on one thread and read on others. The current call ordering happens to serialize all of it (the alert thread only exists once the worker is already running), so I don't think it can actually go wrong today, but it's fragile for no benefit. Creating the handles up front and starting the worker unconditionally in Wh_ModInit removes the whole class of question.
  • The LoadLibraryExW hook is installed even when it can never do anything. In Wh_ModInit (line 3562) the hook is installed whenever systemTrayHookedAtInit is false — including the case where the module was loaded but HookSystemTraySymbols failed. In that case HandleLoadedModuleIfSystemTray always returns at the previous == module check, so the hook just adds GetSystemTrayModuleHandle() (up to three GetModuleHandleW calls, plus a version-resource parse) to every LoadLibraryExW in Explorer for nothing. Install it only when GetSystemTrayModuleHandle() returned null at init.
  • EnsureFinishedAlertResources deletes the old HFONT before the controls stop referencing it (line 1429). On WM_DPICHANGED the old font is destroyed, then DwmSetWindowAttribute runs, and only after that does EnumChildWindows send the new WM_SETFONT; in between the child controls hold a deleted GDI handle. Create the new font first, re-point the children, then DeleteObject the old one.
  • maximumMinutes is silently clamped to 10080 in LoadSettings (line 688) but the $description doesn't mention a cap — worth saying "up to 10080 (7 days)" so a user who types a larger number understands what happened.

Functionality notes

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

  • The completion alert steals focus, and Snooze is the default button. ShowWindow(hWnd, SW_SHOWNORMAL) followed by SetWindowPos(HWND_TOPMOST, ..., SWP_SHOWWINDOW) (lines 2071-2078) will pull the foreground away from whatever the user is doing — Explorer is the shell, so it's allowed to. Combined with BS_DEFPUSHBUTTON on Snooze and IsDialogMessageW in the loop, an Enter keypress that was already in flight lands on the alert and silently snoozes the timer (and Esc dismisses it). SW_SHOWNOACTIVATE plus the FlashWindow you already call would be less intrusive; alternatively drop BS_DEFPUSHBUTTON so a stray keystroke does nothing.
  • The alert doesn't follow a live theme switch. AppsUseLightTheme (line 1370) is only read from WM_CREATE and WM_DPICHANGED, so an alert that's open when the user toggles light/dark keeps the old colors. Handling WM_SETTINGCHANGE with lParam equal to L"ImmersiveColorSet" and re-running ApplyFinishedAlertTheme would cover it.
  • An early/spurious waitable-timer fire permanently disarms the timer. In HandleTimerExpiredFromWorker (line 2275), if GetUtcFileTimeNow() < expected the function returns without doing anything. The waitable timer is auto-reset and one-shot (CreateWaitableTimerW(nullptr, FALSE, nullptr) + lPeriod = 0), so it has already been consumed and nothing re-arms it — the timer then never fires, even though g_deadlineUtc100ns is still set and the taskbar keeps counting down. Calling SignalTimerWorker() before returning from that branch would re-arm it. This should only be reachable on a backwards system-clock adjustment, but it's a cheap guard.
  • Countdown text width churns the tray layout once per second. The ⏱ 1:00:00⏱ 59:59 transitions (and any digit-count change) resize the button and re-lay-out SystemTrayFrameGrid every second. A fixed-width format, or minute resolution above an hour, would keep the tray stable.
  • buttonLabel only applies while idle — the running countdown always uses the hardcoded prefix from FormatCountdown (line 831). If the point of the setting is letting users pick their own glyph (or drop it), it'd be more consistent to apply it to the countdown text too.
  • Primary-taskbar-only and one-timer-at-a-time are both stated in the README, which is the right call — just noting them so they're not mistaken for oversights.


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 windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 10, 2026
@richilp

richilp commented Sep 10, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Sep 10, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.


Overall this is a well-structured mod — the taskbar XAML plumbing is faithfully adapted from taskbar-notification-icon-spacing (symbol strings match exactly, the IconView rebuild detection is the right pattern), taskbar.dll is loaded with LOAD_LIBRARY_SEARCH_SYSTEM32, LoadLibraryExW is hooked in kernelbase, the window class is unregistered in Wh_ModUninit, the [[clang::no_destroy]] usage is correct (bare attribute for the WinRT projected types, std::optional wrapper for the revoker list), and all five settings are declared and read with matching types. Two things need fixing:

1. The authoritative timer is started from the XAML injection path, which makes the timer depend on UI success and opens unload races.

EnsureTimerWorkerStarted() (line 2521) — which also performs RestorePersistedTimer() (line 2612) — is only reached from AddTimerButtonImpl (line 3003) and ArmTimer (line 1023). Three consequences:

  • A persisted timer silently never fires if button injection bails out early — GetTaskbarXamlRoot returning null, SystemTrayFrameGrid not found, or the "unsupported layout class" path (line 2946). The worker never starts, so the countdown that was armed before the Explorer restart is neither restored nor alerted, even though the README advertises "Timer state survives Explorer restarts and mod reloads".
  • A thread can outlive Wh_ModUninit → crash in Explorer. The g_unloading check at line 2527 is a TOCTOU: the taskbar thread can pass it, and Wh_ModUninit can then complete PumpWaitForThread + CloseHandle + g_timerWorkerThread = nullptr (lines 3649-3658) before g_timerWorkerThread = thread at line 2607 runs. The result is a worker thread still executing mod code when Windhawk FreeLibrarys the image. The same window exists for the alert thread: RestorePersistedTimerShowFinishedAlertCreateThread on the taskbar thread, after Wh_ModUninit already did g_finishedAlertThread.exchange(nullptr) (line 3682). Separately, g_timerWorkerThread / g_finishedAlertStopEvent are plain non-atomic globals written on the taskbar (or alert) thread and read unsynchronized from Wh_ModUninit, and two concurrent EnsureTimerWorkerStarted calls would create two workers and leak the first one's handles.
  • A stale "Timer finished" alert can pop up when arming a new timer: if the worker isn't up yet, ArmTimerEnsureTimerWorkerStartedRestorePersistedTimer can show the completion alert for an old expired deadline before ArmTimer stores the new one.

All three go away with the same change: create the events, the waitable timer and the worker thread unconditionally in Wh_ModInit, restore the persisted state once in Wh_ModAfterInit, and delete EnsureTimerWorkerStarted / its call sites. The timer is the mod's core function; it shouldn't be gated on the XAML button appearing.

2. Wh_ModInit skips the LoadLibraryExW hook when the symbol hook failed, not just when it succeeded.

if (!systemTrayModuleLoadedAtInit) {   // line 3572
    // ... install the kernelbase LoadLibraryExW hook
}

The gate is "was some candidate module already loaded", but the thing that matters is "did we actually hook IconView::IconView". GetSystemTrayModuleHandle() (line 3302) falls back to Taskbar.View.dll and then ExplorerExtensions.dll, so on a build where one of those is loaded but SystemTray.dll (the real home of the symbol) hasn't loaded yet, HookSystemTraySymbols fails, systemTrayModuleLoadedAtInit is nevertheless true, and the loader hook is never installed — so SystemTray.dll loading later is never noticed. Wh_ModAfterInit doesn't recover either, because g_systemTrayModuleAttempted was already set to that module at line 3551, so HandleLoadedModuleIfSystemTray returns early. The user-visible effect is that tray-rebuild detection quietly stops working: the button vanishes on the next tray rebuild and never comes back until Explorer restarts.

Fix — gate on the hook result, as taskbar-icon-size.wh.cpp#L3169 does:

if (!systemTrayHookedAtInit) {
    // ... install the kernelbase LoadLibraryExW hook
}
Optional improvements

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

  • g_buttonInjected (line 169) is write-only: it's stored in four places and never read. Dead state, remove it.
  • -lole32 -loleaut32 in @compilerOptions (line 10) appear unused — there's no CoCreateInstance/BSTR/VARIANT anywhere; C++/WinRT only needs -lruntimeobject. Worth trimming.
  • The completion alert sizes its window to 380x220 and then lays children out in client coordinates down to y=181 (lines 1622-1627, 2078-2079). It fits, but only because WS_CAPTION happens to leave just enough room. AdjustWindowRectExForDpi on the desired client rect would make the layout independent of the frame metrics.
  • After a bad snooze value, the label is rewritten to "Snooze minutes (1-%d):" (line 1929) and never restored to "Snooze minutes:", so the hint sticks around for the rest of the alert's lifetime (and at 155px the longer text may clip).
  • Wh_ModBeforeUninit and Wh_ModUninit both call TryRemoveTimerXaml(); the second is a no-op in the normal case. That's a deliberate retry, but a comment saying the first call is expected to do the work would save the next reader a pass.

Functionality notes

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

  • ARM64 FrameHeight offset. Unlike the reference mod, which falls back to the known-good 0x48 offset when the prologue doesn't match (taskbar-notification-icon-spacing.wh.cpp#L826 and #L841), this mod returns nullptr on both architectures (lines 521-551), so the button silently never appears on any build whose prologue changes. The new ARM64 instruction pattern (lines 536-541) is also presumably untested — you mention testing on x64 26200 — and it insists on Rn == x0 with pre-index addressing, which is worth double-checking against a real ARM64 Taskbar.dll. Matching the reference's "log and use the default offset" behavior would degrade more gracefully.
  • The alert steals activation. ShowWindow(hWnd, SW_SHOWNORMAL) followed by SetWindowPos(..., SWP_SHOWWINDOW) (lines 2098-2105) activates the popup, and Snooze is BS_DEFPUSHBUTTON — so an Enter keypress the user intended for whatever they were typing in snoozes the timer. SW_SHOWNOACTIVATE + FlashWindow would be less intrusive; the same applies to the SetForegroundWindow at line 2186.
  • Alert colors are hardcoded. RGB(32,32,32) / RGB(249,249,249) etc. (lines 1440-1448, 1838-1886) ignore the accent color and, more importantly, high-contrast themes, where the result can be unreadable. GetSysColor(COLOR_WINDOW)/COLOR_WINDOWTEXT (or GetThemeColor) when SystemParametersInfo(SPI_GETHIGHCONTRAST) reports high contrast would cover that. Related: the alert doesn't react to WM_SETTINGCHANGE/WM_THEMECHANGED, so a theme switch while it's open leaves it in the old colors. Given this plus the caption bar, "Modern Windows 11-style popup UI" in the README is a bit generous — consider softening the wording.
  • Taskbar button styling. The button is a default XAML Button with only BorderThickness/Padding/MinWidth overridden (lines 2847-2868) — no Background, so it renders with the standard button chrome rather than the transparent-with-hover look of the neighbouring tray buttons. Worth comparing side by side with ControlCenterButton.
  • Wall-clock dependence. The deadline is an absolute FILETIME and SetWaitableTimer is given an absolute due time (lines 2375-2387), so an NTP correction or a manual clock change shifts the countdown. That's the right trade-off for surviving restarts — just noting it as a known behavior (and CREATE_WAITABLE_TIMER_HIGH_RESOLUTION isn't needed here).
  • Primary taskbar only. FindCurrentProcessTaskbarWnd matches Shell_TrayWnd only (line 246), as the README says. If you later want the button on secondary monitors, Shell_SecondaryTrayWnd windows live on the same Explorer UI thread, so no extra synchronization would be needed.
  • Flyout UX while a timer runs. ShowTimerFlyout disables the reminder and minutes boxes (lines 1345-1347), so the only way to change a running timer's text or duration is cancel-then-restart. Allowing an in-place edit (or a "Restart" button) would be a small quality win.


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 windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Sep 10, 2026
@richilp

richilp commented Sep 11, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-ai-review An AI review was requested and is being prepared. label Sep 11, 2026
@windhawk-reviewer

Copy link
Copy Markdown

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.


Overall this is in good shape. The taskbar XAML integration is a faithful adaptation of taskbar-notification-icon-spacing (symbol strings, GetTaskbarXamlRoot, RunFromWindowThread, the IconView revoker list all match), the settings block and the code agree in both directions, and — importantly — the unload path is genuinely correct: both worker threads are signalled and joined inside Wh_ModUninit, the mod never takes an extra reference on its own module, the window class is unregistered, and the [[clang::no_destroy]] usage on the XAML globals (bare attribute on nullable WinRT types, std::optional wrapper on the revoker list, explicit release on the UI thread) is exactly the right shape. I also couldn't find an existing mod that overlaps with this feature. Two things to fix:

1. The timer button is never re-added after an in-process tray rebuild. AddTimerButtonImpl starts with:

// mods/taskbar-countdown-timer.wh.cpp:2718
if (g_timerButton &&
    VisualTreeHelper::GetParent(
        g_timerButton))
{
    return;
}

When the system tray rebuilds its XAML tree in-process — which is exactly the case the IconView::IconViewLoaded hook (line 3296) and the TrayUI::StartTaskbar hook (line 334) exist to catch — the old SystemTrayFrameGrid is detached but the old button is still a child of it, so VisualTreeHelper::GetParent keeps returning non-null and this guard bails out. The button then never appears in the new tree until Explorer restarts. (taskbar-icon-separators documents this case explicitly: "A repeated TrayUI::StartTaskbar is an in-process reconstruction".)

The code just below already handles "already present" correctly and safely — it looks up TaskbarCountdownTimerButton among the current tray's children and compares identity with SameWinrtObject (lines 2768–2795). So the simplest fix is to drop the early return entirely and let that path decide. If you want to keep a fast path, make it tree-aware, e.g. compare g_timerButton.XamlRoot() against the live xamlRoot, or check that walking GetParent from the button reaches xamlRoot.Content().

2. RemoveStaleNamedButton will most likely crash Explorer in the one case it exists for. A "stale" button named TaskbarCountdownTimerButton that this instance doesn't own can only come from a previous DLL instance whose teardown didn't run — i.e. RunFromWindowThread in Wh_ModBeforeUninit/Wh_ModUninit returned false and RemoveTimerButtonImpl never executed. In that case the button still carries a live Click delegate (and the DispatcherTimer still holds a live Tick delegate) whose implementation and vtable live in the now-unmapped previous mod image. children.RemoveAt(staleIndex) (line 2661) drops the last reference to that button, which releases the delegate → virtual call into freed memory → access violation in explorer.exe. The try/catch (...) around AddTimerButtonImpl won't help, since an AV isn't a C++ exception.

Worth clarifying: did you actually observe stale buttons, or was this added defensively (the PR notes AI assistance, and this reads like a speculative safety net)? If it was defensive, I'd drop the path — leaving a foreign-instance element alone is strictly safer than touching it. If you did observe it, the real bug is the teardown that failed, and that's what needs fixing; removing the corpse afterwards can't be made safe from the new instance.

Optional improvements

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

  • HookSymbols can run twice against the same module. If HookSystemTraySymbols fails in Wh_ModInit (line 3557), g_systemTrayModuleAttempted is reset to nullptr (line 3568), so Wh_ModAfterInitHandleLoadedModuleIfSystemTray (line 3385) resolves the same module a second time. HookSymbols caches resolved symbols per module, and a repeat call invalidates that cache and forces a full re-resolution (slow). The reference mod sets its g_systemTrayModuleHooked flag before checking the result, precisely so the module is only ever attempted once — see taskbar-notification-icon-spacing.wh.cpp#L1066.

  • Handle leak when StartTimerWorkerFromInit fails partway (line 2513). If any of the four CreateEventW/CreateWaitableTimerW calls fails, or CreateThread fails, the function returns false without closing the handles that did succeed, and Wh_ModInit returns FALSE. Since a FALSE return means Windhawk retries the mod after every settings change, each retry leaks up to four kernel handles into explorer.exe. Cheapest fix is a small CleanupTimerObjects() helper called from both the failure path and Wh_ModUninit.

  • Duplicated callUnregisterFinishedAlertClass(); appears twice in a row at lines 3539–3540. Harmless (the second returns early) but clearly a copy/paste artifact.

  • HandleLoadedModuleIfSystemTray doesn't check g_unloading. The LoadLibraryExW hook (line 3417) stays active through Wh_ModBeforeUninit, so a DLL load during teardown can still reach HookSystemTraySymbols + Wh_ApplyHookOperations (line 3407) and install a fresh hook set on a mod that's on its way out. Add the same g_unloading guard the other hooks use.

  • The second TryRemoveTimerXaml() in Wh_ModUninit (line 3704) is a no-op in the normal case — Wh_ModBeforeUninit already nulled g_timerButton — but it still costs a cross-thread SendMessage on every unload. Gating it on g_timerButton being non-null would skip it.

  • A failed RegisterFinishedAlertClass() kills the whole mod. Wh_ModInit returns FALSE (line 3487) if the class can't be registered, so an alert-window detail takes the taskbar button down with it. Refusing to reuse an existing class is the right call (don't change that), but you could register the class lazily from ShowFinishedAlert instead, so the countdown still works even if the alert can't be created.

  • Formatting. The repo ships a .clang-format (BasedOnStyle: Chromium, IndentWidth: 4) and every merged mod follows it. The one-argument-per-line style here inflates the file to ~3,800 lines for what is closer to ~1,200 lines of code, which makes diffs on future updates harder to read. Running clang-format with the repo config would bring it in line.

Functionality notes

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

  • The completion alert's layout is a few pixels from clipping. The window is created at 380 × 220 scaled (line 2075), but that's the outer size — WS_CAPTION | WS_SYSMENU eats the caption height plus borders from the client area, while LayoutFinishedAlert (line 1605) places the Snooze/Dismiss buttons at y = 145..181 and x = 200..360 as if the full 380×220 were client space. On a standard caption that leaves only a few pixels of headroom, and a larger caption (different DPI/theme/accessibility settings) will clip the buttons. Size the window from the desired client rect with AdjustWindowRectExForDpi, or lay the children out against GetClientRect instead of constants.

  • Secondary taskbars aren't handled. FindCurrentProcessTaskbarWnd (line 223) only looks for Shell_TrayWnd, so on multi-monitor setups the button only appears on the primary taskbar. The README does say this, so it's fine as a documented limitation — but note all taskbars share the same Explorer UI thread, so extending to Shell_SecondaryTrayWnd doesn't introduce any threading complexity. taskbar-vd-switcher.wh.cpp inserts into SystemTrayFrameGrid on both and is a close reference for the pattern (it's also worth a look for its tray-column insertion helper and its thread-join helper, which is nearly identical to your PumpWaitForThread).

  • The countdown text changes width as it counts down, which shifts every tray element to its right — most visibly when crossing the 1-hour boundary (1:00:0059:59), but also between ⏱ 09:59 and e.g. ⏱ 9:59 on narrower fonts. Setting a MinWidth on g_timerButton sized for the initial duration, or using tabular figures, would stop the tray from jittering.

  • FormatCountdown hardcodes the prefix (lines 871/883) while the idle label is user-configurable via buttonLabel. A user who sets a plain text label still gets the emoji back the moment the countdown starts. Either derive the running prefix from the setting, or split the icon out into its own setting.

  • Long durations render awkwardly. maximumMinutes goes up to 10080 (7 days), which FormatCountdown renders as ⏱ 168:00:00 — a very wide taskbar button. Consider a Nd HH:MM form above 24 hours, or dropping the seconds once the remaining time is over an hour.

  • The alert takes foreground. ShowWindow(SW_SHOWNORMAL) + HWND_TOPMOST + FlashWindow (lines 2095–2104) will interrupt a full-screen game or a typing session. That's arguably what a timer should do, but SW_SHOWNOACTIVATE + flash (letting the user click into it) would be a gentler default, and is easy to make a setting.

  • A running timer can't be adjusted, only cancelled (ShowTimerFlyout disables the fields, line 1334), and when ParseMinutes rejects the input the flyout just re-selects the text with no explanation of the valid range — unlike the alert's snooze field, which does show Snooze minutes (1-N):. Showing the same hint in the flyout would be more consistent.

  • Clock-jump edge case in the worker. If the system clock moves backwards after a timer is armed, the waitable timer fires, HandleTimerExpiredFromWorker sees GetUtcFileTimeNow() < expected and calls SignalTimerWorker() (line 2299), which re-arms with the same absolute due time — which is still in the past from the timer's perspective, so it fires again immediately. That's a busy loop until the clock catches up. Adding a small minimum re-arm delay would bound it.


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.

@richilp

richilp commented Sep 11, 2026

Copy link
Copy Markdown
Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

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-09-11 17:34 UTC (in 8 hours) to get another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-author The author's turn: request an AI review, or respond to one that was posted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant