Skip to content

PivotLink: Browser Router 1.0#4699

Open
gauthumj wants to merge 2 commits into
ramensoftware:mainfrom
gauthumj:main
Open

PivotLink: Browser Router 1.0#4699
gauthumj wants to merge 2 commits into
ramensoftware:mainfrom
gauthumj:main

Conversation

@gauthumj

@gauthumj gauthumj commented Jul 6, 2026

Copy link
Copy Markdown

Changelog

If this pull request updates an existing mod, describe the changes below:

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.

@m417z

m417z commented Jul 6, 2026

Copy link
Copy Markdown
Member

Submission review

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

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


A few of these are functional blockers — the mod currently fails for its own advertised use case (Discord/Slack), and one hook reports failure to the caller even after a successful redirect.

(HINSTANCE)32 / hInstApp = 32 signals failure, not success. ShellExecute/ShellExecuteEx treat any value ≤ 32 as an error (32 == SE_ERR_DLLNOTFOUND); success is strictly > 32. So after ShellExecuteW_Hook redirects a link, it returns (HINSTANCE)32 (pivotlink-browser-router.cpp:288) and ShellExecuteExW_Hook sets hInstApp = (HINSTANCE)32 (line 276) — both read as "the launch failed". Callers that check the return (if ((INT_PTR)ShellExecuteW(...) <= 32) { error } is the standard idiom — see mods/auto-theme-switcher.wh.cpp#L363) will show an error dialog or fall back to another launch path, even though the browser did open. Return a value > 32 (33 is the convention — mods/f1-blocker.wh.cpp#L82, mods/classic-taskbar-properties.wh.cpp#L96).

The D3D "game detection" disables the mod in the exact apps it targets. IsCurrentProcessGame() (line 156) returns true for any process with d3d9/11/12.dll or vulkan-1.dll loaded, and the only exclusions are the configured browsers. But Electron/Chromium apps — including Discord and Slack, the README's own motivating examples — routinely load D3D (ANGLE), as do WPF/UWP apps, media players, Office, etc. In all of those, RouteLinkIfNecessary bails out at line 234 and the link is never redirected, so the mod silently does nothing in its primary use case. Recommend dropping the game-detection feature entirely (simplest, and removes the false positives) — disabling redirection specifically in games is a niche need that isn't worth this cost. If you keep it, it needs a far more reliable signal than "loaded a graphics DLL."

The CreateProcessW hook is hardcoded to zen.exe. The entire hook body only does anything when the command line or application name contains the literal string zen.exe (lines 306–307). This looks like leftover personal configuration (or an AI artifact) — it doesn't match the generic, browser-agnostic mod described in the README, and no user other than a Zen Browser user gets any CreateProcessW handling at all. On top of that, when it does fire, the synthesized PROCESS_INFORMATION is broken: hThread = NULL, dwThreadId = 0, and hProcess points at a different browser than the caller asked to launch (lines 340–345). Any caller that waits on the returned process, resumes the thread (CREATE_SUSPENDEDResumeThread(NULL)), or reads the PID will misbehave. Either generalize this to detect any configured browser being launched with a URL, or drop the CreateProcessW hook — which would also meaningfully shrink the mod's blast radius (see below).

@author You is a placeholder (line 6). It should be your real name/handle; your @github is already gauthumj.

Optional improvements

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

  • Settings-change data race. LoadSettings() does g_priorityBrowsers.clear() and repopulates it (lines 207–223) on the settings-changed thread while other threads iterate the same vector in GetHighestPriorityRunningBrowser() / IsCurrentProcessGame(). That's a data race on a std::vector that can crash on a settings change. Guard the globals with a mutex (copy out a local snapshot under the lock before use) or swap in a freshly-built vector atomically.
  • Drop the custom [PivotLink] log prefix (lines 253, 327, 366). Windhawk already prefixes every Wh_Log line with the mod name, so the bracketed prefix is redundant.
  • Dead / redundant default handling. Wh_GetStringSetting never returns NULL (it returns L"" on miss), so the if (rawBrowser)/if (rawSetting …) NULL checks, the hardcodedDefaults[] fallback branch (lines 210, 220–222), and the if (g_priorityBrowsers.empty()) fallback (lines 225–227) are all unreachable — the defaults already live in the ==WindhawkModSettings== block. This can be simplified a lot.
  • Prefer the type-safe utilities. Use WindhawkUtils::SetFunctionHook() instead of raw Wh_SetFunctionHook with (void*) casts (lines 370–372), and WindhawkUtils::StringSetting (RAII) instead of Wh_GetStringSetting + manual Wh_FreeStringSetting.
  • Call the original directly. RouteLinkIfNecessary invokes the hooked ShellExecuteExW (line 262) and relies on t_inHook to avoid recursing; calling ShellExecuteExW_Original directly is clearer and avoids the extra hook round-trip.

Functionality notes

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

  • @include * blast radius. Catching link opens from arbitrary apps inherently requires broad injection, so * is defensible for the ShellExecute* hooks — there's no cleaner alternative. But be aware it injects and installs three hooks into every user-session process; dropping the CreateProcessW hook (see above) would at least remove one of them from every process.
  • Per-call snapshot cost. GetHighestPriorityRunningBrowser() takes a full CreateToolhelp32Snapshot and runs EnumWindows per candidate PID on every intercepted link/launch. Fine given link opens are infrequent, just noting it.
  • Verb is ignored. The ShellExecute* hooks redirect any http(s) lpFile to a browser regardless of the operation/verb, so a non-open verb (e.g. edit, print) on a URL would still get rerouted. Edge case, but worth a guard if you want to be precise.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants