Add Windows support (WebView2 + Go build tags)#542
Open
chaoz23 wants to merge 15 commits into
Open
Conversation
Implements the Windows platform layer behind //go:build windows, leaving the macOS build unaffected. Mirrors the existing _darwin split with _windows counterparts. - URL routing: named-mutex single-instance + local socket IPC forwards URLs (and --window) from secondary launches to the resident instance, replacing macOS Apple Events (Windows uses new-process-per-URL) - Config UI: WebView2 via jchv/go-webview2 (pure Go, no cgo); local HTTP asset server replaces WKURLSchemeHandler; JS<->Go bridge via Bind(). The window is created and message-pumped on the LockOSThread main thread (Win32 delivers window messages only to the creating thread) - Browser detection/launch: StartMenuInternet registry + known install paths; config_dir_windows added to browsers.json for Chromium/Firefox profile resolution (%LOCALAPPDATA% / %APPDATA%) - Default-browser detection via the UserChoice ProgID (FinickyURL) - util: modifier keys, battery, process list via Win32; cross-platform LogDir() - Packaging/CI: Inno Setup installer with protocol-handler registration, winget manifest, cross-compile build script, and a separate, non-blocking Windows CI workflow - WINDOWS_PORT_QC.md documents the platform layer and a reusable QC checklist Cross-compiles cleanly (GOOS=windows CGO_ENABLED=0); go test ./... passes on both platforms. Refs johnste#538 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds Windows support to Finicky by separating macOS-only files with build tags, introducing Windows browser detection, launching, default-browser checks, utilities, a Windows entrypoint, and a WebView2 UI bridge. It also adds Windows build/CI, packaging scripts, manifests, and documentation updates. ChangesWindows Port
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SecondaryInstance
participant listenForURLs
participant PrimaryInstance
participant WebView
SecondaryInstance->>PrimaryInstance: sendToPrimary(url or show signal)
PrimaryInstance->>listenForURLs: accept connection, read payload
listenForURLs->>PrimaryInstance: dispatch URL or show-window request
PrimaryInstance->>WebView: requestShowWindow / launch browser
sequenceDiagram
participant UI as WebView JS
participant HandleWebViewMessage
participant BrowserModule
participant RulesModule
UI->>HandleWebViewMessage: JSON message with type
HandleWebViewMessage->>RulesModule: handleGetRules / handleSaveRules
HandleWebViewMessage->>BrowserModule: handleGetInstalledBrowsers / handleGetBrowserProfiles
HandleWebViewMessage-->>UI: SendMessageToWebView response
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (14)
.github/workflows/windows-build.yml (2)
15-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTighten the checkout token scope.
These checkout steps don't need persisted Git credentials, and the job doesn't need repo write access. Add
persist-credentials: falseand a minimalpermissions:block.Also applies to: 86-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/windows-build.yml around lines 15 - 17, Harden the Windows workflow checkout step by reducing GitHub token exposure. Update the Checkout code step that uses actions/checkout@v4 to disable persisted credentials with persist-credentials set to false, and add a minimal permissions block for the job since it does not need repo write access. Apply the same change to the other checkout step referenced in this workflow so both uses of actions/checkout are consistent and least-privileged.Source: Linters/SAST tools
27-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBump
actions/setup-goin both jobs
actions/setup-go@v4is behind the current major. Update both call sites to the latest major to keep the workflow aligned with current runner/runtime support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/windows-build.yml around lines 27 - 32, Update both instances of actions/setup-go in the workflow from v4 to the current major version. The fix should be applied in each Set up Go step, keeping the existing go-version and cache-dependency-path settings unchanged, so both jobs use the same supported setup-go action version.Source: Linters/SAST tools
scripts/installer.iss (3)
87-91: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo WebView2 Runtime presence check before/after install.
The app is built on
jchv/go-webview2per the PR objectives, but the installer neither verifies nor bootstraps the WebView2 Evergreen Runtime. On a clean Windows 10 1809 install (the declaredMinimumOSVersion), the Runtime may not be present, and the freshly-installed app launched at Line 87 (--window) could fail to render silently.Consider adding a runtime check in
[Code](e.g., checkingHKLM\SOFTWARE\WOW6432Node\Microsoft\EdgeUpdate\Clients\{F3017226-FE2A-4295-8BDF-00C3A9A7E4C5}) and prompting to download the bootstrapper if missing, similar to other WebView2-based installers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/installer.iss` around lines 87 - 91, The installer flow currently launches the app from the postinstall entry without verifying that the WebView2 Evergreen Runtime is installed. Add a runtime presence check in the installer’s [Code] section, using the WebView2 Evergreen runtime registry key or equivalent detection, and gate the postinstall launch on that check. If the runtime is missing, prompt the user to install or download the WebView2 bootstrapper before starting the app, and keep the launch behavior in the installer entries aligned with that check.
97-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CurStepChangedis a documented no-op.The comment explains why
SHChangeNotifyisn't invoked; consider calling it via an external DLL import for immediate Default Apps refresh, though this is optional polish since the registry changes will still be picked up eventually.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/installer.iss` around lines 97 - 112, CurStepChanged is currently a no-op, so Default Apps refresh is only deferred until Windows next re-reads the registry. Update the CurStepChanged handler to actually trigger SHChangeNotify during ssPostInstall, using the existing comment as the cue and a small external DLL/import helper if needed since Inno Setup does not expose it directly. Keep the change localized to CurStepChanged and preserve the registry-based fallback behavior.
89-92: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueForce-kill on uninstall may interrupt in-flight work.
taskkill /Fforcibly terminates Finicky during uninstall without giving it a chance to flush state/close the IPC socket gracefully. Given Finicky's job is transient URL routing, impact is likely minimal, but worth a quick check that no pending browser launch or config save is silently dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/installer.iss` around lines 89 - 92, The uninstall step currently uses taskkill with the force flag in the [UninstallRun] entry, which can stop Finicky before it flushes state or closes IPC cleanly. Update the uninstall termination behavior in the installer script so Finicky is asked to exit gracefully first, and only use a forced kill as a fallback if needed; keep the change localized around the UninstallRun taskkill command for {`#MyAppExeName`}.scripts/build-windows.sh (1)
27-27: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAnchor the
.envgrep to avoid false matches.
grep API_HOSTwithout an anchor will also match keys likeSOME_API_HOST_OTHER=or commented-out lines, silently embedding the wrong value into the binary.🩹 Suggested fix
-API_HOST=$(cat "$ROOT_DIR/.env" 2>/dev/null | grep API_HOST | cut -d '=' -f 2 || echo "") +API_HOST=$(grep -E '^API_HOST=' "$ROOT_DIR/.env" 2>/dev/null | cut -d '=' -f 2- || echo "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-windows.sh` at line 27, The API_HOST extraction in the build-windows.sh script is too broad and can match unrelated keys or commented lines. Update the shell pipeline around the API_HOST assignment to anchor the grep in a way that only matches the exact .env key, and keep the change localized to the API_HOST lookup logic so the binary embeds the intended value.scripts/winget/JohnSterling.Finicky.installer.yaml (1)
15-22: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider declaring the WebView2 Runtime as a package dependency.
Since Finicky's UI depends on the WebView2 Evergreen Runtime, declaring it via
Dependencies.PackageDependencieslets winget prompt/install it alongside Finicky on systems where it's missing (same root concern as the installer's lack of a Runtime check, seescripts/installer.iss).📦 Suggested addition
Installers: - Architecture: x64 InstallerUrl: https://github.com/johnste/finicky/releases/download/v4.2.2/FinickySetup-4.2.2.exe InstallerSha256: # Fill after building the installer InstallerSwitches: Silent: /VERYSILENT /SUPPRESSMSGBOXES /NORESTART SilentWithProgress: /SILENT /SUPPRESSMSGBOXES /NORESTART +Dependencies: + PackageDependencies: + - PackageIdentifier: Microsoft.EdgeWebView2Runtime ManifestType: installerSince winget dependency support has caveats (interactive-only, no version enforcement per some reports), please confirm current behavior before adding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/winget/JohnSterling.Finicky.installer.yaml` around lines 15 - 22, The winget installer manifest for JohnSterling.Finicky is missing the WebView2 Evergreen Runtime dependency, which should be declared so winget can prompt/install it when absent. Update the manifest’s top-level metadata to add a Dependencies.PackageDependencies entry for WebView2, and ensure the dependency naming matches the package used by winget. Use the existing installer manifest structure to locate the change and keep it aligned with the runtime requirement referenced by scripts/installer.iss.apps/finicky/src/util/info_windows.go (2)
159-173: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the
GetForegroundWindowproc like the others.
procGetForegroundWindowis re-created viauser32.NewProc(...)on every call instead of being declared once in the packagevar (...)block with the other procs (procGetAsyncKeyState,procGetWindowTextW, etc.). This does an avoidable lookup on every invocation.♻️ Proposed fix
var ( user32 = syscall.NewLazyDLL("user32.dll") kernel32 = syscall.NewLazyDLL("kernel32.dll") procGetAsyncKeyState = user32.NewProc("GetAsyncKeyState") procGetWindowTextW = user32.NewProc("GetWindowTextW") + procGetForegroundWindow = user32.NewProc("GetForegroundWindow") procCreateToolhelp32Snapshot = kernel32.NewProc("CreateToolhelp32Snapshot") procProcess32FirstW = kernel32.NewProc("Process32FirstW") procProcess32NextW = kernel32.NewProc("Process32NextW") procGetSystemPowerStatus = kernel32.NewProc("GetSystemPowerStatus") ) ... func GetForegroundWindowTitle() string { - procGetForegroundWindow := user32.NewProc("GetForegroundWindow") hwnd, _, _ := procGetForegroundWindow.Call()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/util/info_windows.go` around lines 159 - 173, GetForegroundWindowTitle recreates the GetForegroundWindow proc on every call instead of reusing a cached package-level proc. Move the user32.NewProc("GetForegroundWindow") lookup into the existing package var block alongside procGetAsyncKeyState and procGetWindowTextW, then update GetForegroundWindowTitle to call the shared proc variable.
175-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
%LOCALAPPDATA%instead of%APPDATA%for logs.Logs are machine-local diagnostic data and, per Windows conventions, shouldn't live in the roaming profile (
%APPDATA%), which can be synced across machines in domain/Azure AD-joined environments. This is also inconsistent with the rest of this PR:directories_windows.go'sUserCacheDir()andmain_windows.go'spipeAddr()both useos.UserCacheDir()(Local AppData) for other Finicky on-disk state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/util/info_windows.go` around lines 175 - 182, LogDir currently uses APPDATA for Finicky logs, but logs should be stored in the machine-local profile. Update LogDir in info_windows.go to use the same Local AppData source as UserCacheDir() and pipeAddr() (for example via os.UserCacheDir()), and keep the existing error handling if the local directory cannot be resolved. This keeps log storage consistent with the other Windows state paths in the PR.apps/finicky/src/browser/launcher_windows.go (3)
172-262: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRepeated JSON parsing on every call.
browsersJsonDatais unmarshalled fresh in bothresolveBrowserProfileArgsandGetProfilesForBrowseron every invocation. Consider parsing once (e.g.sync.Onceor package-level init) and reusing the slice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/browser/launcher_windows.go` around lines 172 - 262, Both resolveBrowserProfileArgs and GetProfilesForBrowser re-unmarshal browsersJsonData on every call; move the JSON parsing into a shared package-level initializer or sync.Once-backed helper and reuse the parsed []browserInfo. Keep the existing lookup logic in resolveBrowserProfileArgs and GetProfilesForBrowser, but have both functions read from the cached browser list instead of parsing locally each time.
264-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAcknowledged duplication with
launcher.go's profile-parsing logic.The comment flags this as intentional, deferred duplication. Consider extracting shared Chromium/Firefox profile-parsing helpers into a platform-agnostic file once both platforms stabilize.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/browser/launcher_windows.go` around lines 264 - 382, Acknowledge the duplicated profile-parsing helpers in launcher_windows.go and leave them as-is for now; no code change is needed for this review. If you do touch it later, extract the shared Firefox/Chromium parsing logic from readFirefoxProfileNames, parseFirefoxProfiles, chromiumInfoCache, getAllChromiumProfiles, and parseProfiles into a common helper only once both launchers are stable.
384-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pathparameter is effectively dead code.
cmd.Args(passed asargsat both call sites) always includes the program name as its first element, solen(args) == 0never triggers andpathis never actually used. Simplify the signature to just takeargs []string, or make the intent explicit by prependingpathwhen it differs fromargs[0].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/browser/launcher_windows.go` around lines 384 - 393, The formatCommand helper currently takes a path parameter that is never used because args always includes the program name, so the function signature and call sites should be simplified. Update formatCommand to either accept only args []string and build the quoted command from that slice, or explicitly prepend path when it differs from args[0] so the parameter has real effect; use formatCommand and its current call sites in launcher_windows.go to locate the changes.apps/finicky/src/browser/detect_windows.go (2)
89-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueValue type not checked before reinterpreting registry data as a string.
lpTypeis passed as0/NULL, so a non-string default value (unlikely but possible) would be blindly decoded as UTF-16, potentially returning garbage instead of falling back tosubkeyName. Low risk in practice since browserStartMenuInternetdefault values are conventionally REG_SZ, and there is a fallback path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/browser/detect_windows.go` around lines 89 - 104, The registry default value read in detectWindowsBrowser (the procRegQueryValue call and subsequent UTF16ToString conversion) assumes the value is a string without checking its type. Update this logic to request and inspect the registry value type before decoding, and only convert when it is a string-like type; otherwise fall back to subkeyName. Keep the existing size and buffer handling, but guard the string re-interpretation with the type check in detectWindowsBrowser.
13-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
golang.org/x/sys/windows/registryinstead of hand-rolledsyscall/unsafecalls.The whole file manually declares
advapi32.dllprocs and marshals buffers withunsafe.Pointer.golang.org/x/sys/windows/registrywrapsRegOpenKeyEx/RegEnumKeyEx/RegQueryValueExwithOpenKey,ReadSubKeyNames, andGetStringValue, removing the manual pointer/buffer bookkeeping and making the HKCU+HKLM merge above much simpler to implement correctly.Also applies to: 32-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/browser/detect_windows.go` around lines 13 - 24, The Windows registry access in detect_windows.go is hand-rolling advapi32.dll calls with syscall/unsafe, which is brittle and harder to maintain. Refactor the registry lookup logic in the browser detection path to use golang.org/x/sys/windows/registry instead, replacing the direct use of procRegOpenKeyEx, procRegEnumKeyEx, procRegQueryValue, and procRegCloseKey with OpenKey, ReadSubKeyNames, and GetStringValue. Keep the existing HKLM/HKCU merge behavior in the browser-detection functions, but let the registry package handle pointer/buffer management.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/windows-build.yml:
- Around line 41-47: The Windows build step is using an interactive copy command
that can hang in CI when the target asset already exists. Update the Build
config-api step in the windows workflow to make the copy non-interactive by
using a forced overwrite option such as /Y with cmd copy, or switch to a
non-interactive equivalent like Copy-Item -Force, so the pipeline stays
deterministic.
In `@apps/finicky/src/browser/detect_windows.go`:
- Around line 32-71: The browser registry scan in getBrowsersFromRegistry only
reads HKEY_LOCAL_MACHINE, so per-user installs registered under
HKEY_CURRENT_USER are missed. Update getBrowsersFromRegistry to enumerate both
HKCU and HKLM StartMenuInternet keys, merge the discovered browser names, and
deduplicate them before returning. Keep the existing Finicky filtering and reuse
getBrowserDisplayName for each registry root so GetInstalledBrowsers gets a
complete list.
In `@apps/finicky/src/browser/launcher_windows.go`:
- Around line 104-119: The pipe handling in launcher_windows.go can deadlock
because `io.ReadAll` drains `stderr` and `stdout` sequentially while
`cmd.Start()` is running. Update the `cmd.StderrPipe` / `cmd.StdoutPipe` /
`cmd.Wait` flow to read both streams concurrently, using the existing `cmd`
setup in the launcher function and preserving the collected output for error
handling. Make sure both `stderrBytes` and `stdoutBytes` are fully drained
before `cmd.Wait` is finalized.
- Around line 79-92: The fallback path in the Windows launcher is reusing
arguments incorrectly: `cmdArgs` already includes `config.URL`, so appending
`config.URL` again in the `cmd /c start` call duplicates the URL and pushes
browser-specific args into the wrong position. Update the fallback in the
`findBrowserExe` branch to build the `cmd` invocation so that `config.URL` is
passed only once and any profile/custom args remain before it or are otherwise
forwarded correctly for `cmd /c start`. Use the existing `cmdArgs`,
`config.URL`, and `exec.Command` flow to keep the browser-targeting behavior
intact.
- Around line 82-91: The fallback in launcher_windows.go uses launcher logic in
the cmd /c start path, which re-parses an untrusted URL through cmd.exe and can
allow command injection. Replace the fallback in the relevant launcher flow with
a direct Windows shell/API-based open mechanism instead of exec.Command("cmd",
...) so config.URL is never tokenized by the shell; keep the dryRun logging
behavior and update the existing fallback branch accordingly.
In `@apps/finicky/src/main_windows.go`:
- Line 70: Guard the shared globals in main_windows.go, especially vm (and also
lastError, configInfo, updateInfo), because they are read and written from
multiple goroutines including main, SaveRulesHandler, runEventLoop’s
configChange path, and checkForUpdates. Update the access pattern around
checkForUpdates and the handlers so vm is not nil-checked and dereferenced
without synchronization; use a sync.RWMutex around all reads/writes or funnel
all mutations through the single runEventLoop goroutine instead of touching
package-level state directly from callbacks.
- Around line 560-566: The listener accept loop in main_windows.go can spin
forever if listener.Accept keeps returning a persistent error, causing CPU and
log flooding. Update the accept loop around listener.Accept to detect
unrecoverable/closed listener errors and exit the goroutine instead of always
continuing. If you keep retrying, add a bounded backoff or delay inside the loop
and consider checking for a terminal error condition before the slog.Error call.
Use the existing listener.Accept and slog.Error logic as the insertion point.
- Around line 171-180: The fatal config setup path in main_windows.go still
continues after NewConfigFileWatcher fails, which can leave cfw nil and cause
setupVM to run anyway. Update the initialization flow around
NewConfigFileWatcher, handleFatalError, and setupVM so that a fatal watcher
setup error stops execution immediately, either by returning early after
handleFatalError or by making handleFatalError terminate the startup path.
- Around line 509-538: Initialize configInfo before building the WebView payload
in setupVM: GetConfigState() can return nil, and the current code only assigns
configInfo inside the cs != nil branch before later dereferencing
configInfo.Handlers, configInfo.Rewrites, configInfo.DefaultBrowser, and
configInfo.ConfigPath. Fix this by seeding configInfo with a non-nil default
ConfigInfo or by guarding the SendMessageToWebView("config", ...) payload so it
only reads fields when configInfo is present.
- Around line 593-608: The URL fallback in handOffToPrimary still shells out
through cmd.exe, which can re-parse urlFromArgs and allow metacharacters to be
interpreted. Replace the cmd.Command("cmd", "/c", "start", "", urlFromArgs) path
with a non-shell Windows launch mechanism such as ShellExecuteW, keeping the
existing default-browser check and unreachable-instance logging intact. Use
handOffToPrimary and the sendToPrimary fallback branch to locate the change, and
ensure the new launcher opens the URL directly without invoking a command shell.
In `@apps/finicky/src/window/window_windows.go`:
- Around line 334-349: The save flow in window_windows.go acknowledges success
too early: rules.Save and the "rules" WebView message happen before
SaveRulesHandler runs, so runtime failures in the handler can leave the UI out
of sync. Update the save path around SaveRulesHandler to validate/rebuild first
or make SaveRulesHandler return an error and only send the success response
after it succeeds; if it fails, emit the error message instead of the success
message.
- Around line 291-300: The handleGetRules flow is converting every rules.Load()
failure into an empty ruleset, which should only happen when the rules file is
actually missing. Update handleGetRules to distinguish a missing-file case from
parse/permission/other load errors, using rules.Load() and its error handling to
branch accordingly. On non-missing errors, keep logging the failure and surface
the error state instead of sending an empty "rules" payload to
SendMessageToWebView; only return the default browser plus empty rules when the
file truly does not exist.
- Around line 86-88: Gate the WebView2 debug setting in window_windows.go so it
is only enabled in development builds, not packaged Windows releases. Update the
webview2.NewWithOptions call in the Windows window setup to derive Debug from a
dev-only flag, env var, or build tag instead of hardcoding true, while keeping
AutoFocus unchanged. Use the WebView2 initialization path in the window creation
flow to locate the change.
In `@README.md`:
- Around line 42-43: The install instructions use names that do not match the
shipped installer and winget package, so update the README wording to match the
actual output from scripts/installer.iss and the winget manifest ID
JohnSterling.Finicky. Adjust the download filename reference and the winget
install command accordingly, using the exact shipped names so users can install
the app without confusion or failure.
---
Nitpick comments:
In @.github/workflows/windows-build.yml:
- Around line 15-17: Harden the Windows workflow checkout step by reducing
GitHub token exposure. Update the Checkout code step that uses
actions/checkout@v4 to disable persisted credentials with persist-credentials
set to false, and add a minimal permissions block for the job since it does not
need repo write access. Apply the same change to the other checkout step
referenced in this workflow so both uses of actions/checkout are consistent and
least-privileged.
- Around line 27-32: Update both instances of actions/setup-go in the workflow
from v4 to the current major version. The fix should be applied in each Set up
Go step, keeping the existing go-version and cache-dependency-path settings
unchanged, so both jobs use the same supported setup-go action version.
In `@apps/finicky/src/browser/detect_windows.go`:
- Around line 89-104: The registry default value read in detectWindowsBrowser
(the procRegQueryValue call and subsequent UTF16ToString conversion) assumes the
value is a string without checking its type. Update this logic to request and
inspect the registry value type before decoding, and only convert when it is a
string-like type; otherwise fall back to subkeyName. Keep the existing size and
buffer handling, but guard the string re-interpretation with the type check in
detectWindowsBrowser.
- Around line 13-24: The Windows registry access in detect_windows.go is
hand-rolling advapi32.dll calls with syscall/unsafe, which is brittle and harder
to maintain. Refactor the registry lookup logic in the browser detection path to
use golang.org/x/sys/windows/registry instead, replacing the direct use of
procRegOpenKeyEx, procRegEnumKeyEx, procRegQueryValue, and procRegCloseKey with
OpenKey, ReadSubKeyNames, and GetStringValue. Keep the existing HKLM/HKCU merge
behavior in the browser-detection functions, but let the registry package handle
pointer/buffer management.
In `@apps/finicky/src/browser/launcher_windows.go`:
- Around line 172-262: Both resolveBrowserProfileArgs and GetProfilesForBrowser
re-unmarshal browsersJsonData on every call; move the JSON parsing into a shared
package-level initializer or sync.Once-backed helper and reuse the parsed
[]browserInfo. Keep the existing lookup logic in resolveBrowserProfileArgs and
GetProfilesForBrowser, but have both functions read from the cached browser list
instead of parsing locally each time.
- Around line 264-382: Acknowledge the duplicated profile-parsing helpers in
launcher_windows.go and leave them as-is for now; no code change is needed for
this review. If you do touch it later, extract the shared Firefox/Chromium
parsing logic from readFirefoxProfileNames, parseFirefoxProfiles,
chromiumInfoCache, getAllChromiumProfiles, and parseProfiles into a common
helper only once both launchers are stable.
- Around line 384-393: The formatCommand helper currently takes a path parameter
that is never used because args always includes the program name, so the
function signature and call sites should be simplified. Update formatCommand to
either accept only args []string and build the quoted command from that slice,
or explicitly prepend path when it differs from args[0] so the parameter has
real effect; use formatCommand and its current call sites in launcher_windows.go
to locate the changes.
In `@apps/finicky/src/util/info_windows.go`:
- Around line 159-173: GetForegroundWindowTitle recreates the
GetForegroundWindow proc on every call instead of reusing a cached package-level
proc. Move the user32.NewProc("GetForegroundWindow") lookup into the existing
package var block alongside procGetAsyncKeyState and procGetWindowTextW, then
update GetForegroundWindowTitle to call the shared proc variable.
- Around line 175-182: LogDir currently uses APPDATA for Finicky logs, but logs
should be stored in the machine-local profile. Update LogDir in info_windows.go
to use the same Local AppData source as UserCacheDir() and pipeAddr() (for
example via os.UserCacheDir()), and keep the existing error handling if the
local directory cannot be resolved. This keeps log storage consistent with the
other Windows state paths in the PR.
In `@scripts/build-windows.sh`:
- Line 27: The API_HOST extraction in the build-windows.sh script is too broad
and can match unrelated keys or commented lines. Update the shell pipeline
around the API_HOST assignment to anchor the grep in a way that only matches the
exact .env key, and keep the change localized to the API_HOST lookup logic so
the binary embeds the intended value.
In `@scripts/installer.iss`:
- Around line 87-91: The installer flow currently launches the app from the
postinstall entry without verifying that the WebView2 Evergreen Runtime is
installed. Add a runtime presence check in the installer’s [Code] section, using
the WebView2 Evergreen runtime registry key or equivalent detection, and gate
the postinstall launch on that check. If the runtime is missing, prompt the user
to install or download the WebView2 bootstrapper before starting the app, and
keep the launch behavior in the installer entries aligned with that check.
- Around line 97-112: CurStepChanged is currently a no-op, so Default Apps
refresh is only deferred until Windows next re-reads the registry. Update the
CurStepChanged handler to actually trigger SHChangeNotify during ssPostInstall,
using the existing comment as the cue and a small external DLL/import helper if
needed since Inno Setup does not expose it directly. Keep the change localized
to CurStepChanged and preserve the registry-based fallback behavior.
- Around line 89-92: The uninstall step currently uses taskkill with the force
flag in the [UninstallRun] entry, which can stop Finicky before it flushes state
or closes IPC cleanly. Update the uninstall termination behavior in the
installer script so Finicky is asked to exit gracefully first, and only use a
forced kill as a fallback if needed; keep the change localized around the
UninstallRun taskkill command for {`#MyAppExeName`}.
In `@scripts/winget/JohnSterling.Finicky.installer.yaml`:
- Around line 15-22: The winget installer manifest for JohnSterling.Finicky is
missing the WebView2 Evergreen Runtime dependency, which should be declared so
winget can prompt/install it when absent. Update the manifest’s top-level
metadata to add a Dependencies.PackageDependencies entry for WebView2, and
ensure the dependency naming matches the package used by winget. Use the
existing installer manifest structure to locate the change and keep it aligned
with the runtime requirement referenced by scripts/installer.iss.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0aa0a09b-0eb7-45e6-9406-ef71ef3cb9a5
⛔ Files ignored due to path filters (2)
apps/finicky/assets/Resources/finicky.icois excluded by!**/*.icoapps/finicky/src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
.github/workflows/windows-build.yml.gitignoreREADME.mdWINDOWS_PORT_QC.mdapps/finicky/src/browser.goapps/finicky/src/browser/browsers.jsonapps/finicky/src/browser/detect.goapps/finicky/src/browser/detect_windows.goapps/finicky/src/browser/launcher.goapps/finicky/src/browser/launcher_windows.goapps/finicky/src/browser_windows.goapps/finicky/src/go.modapps/finicky/src/logger/logger.goapps/finicky/src/main.goapps/finicky/src/main_windows.goapps/finicky/src/util/directories.goapps/finicky/src/util/directories_windows.goapps/finicky/src/util/info.goapps/finicky/src/util/info_darwin_ext.goapps/finicky/src/util/info_windows.goapps/finicky/src/window/window.goapps/finicky/src/window/window_windows.goscripts/build-windows.shscripts/installer.issscripts/winget/JohnSterling.Finicky.installer.yamlscripts/winget/JohnSterling.Finicky.locale.en-US.yamlscripts/winget/JohnSterling.Finicky.yaml
Comment on lines
+291
to
+300
| func handleGetRules() { | ||
| rf, err := rules.Load() | ||
| if err != nil { | ||
| slog.Error("Failed to load rules", "error", err) | ||
| SendMessageToWebView("rules", map[string]interface{}{ | ||
| "defaultBrowser": "", | ||
| "rules": []interface{}{}, | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t turn a failed rules load into an empty ruleset.
For parse/permission errors, sending empty rules can make the UI look blank and a later save can overwrite the user’s existing config. Only default on “file missing”; otherwise surface the load error.
Proposed direction
rf, err := rules.Load()
if err != nil {
slog.Error("Failed to load rules", "error", err)
+ path, _ := rules.GetPath()
+ SendMessageToWebView("rulesError", map[string]interface{}{
+ "error": err.Error(),
+ "path": util.ShortenPath(path),
+ })
- SendMessageToWebView("rules", map[string]interface{}{
- "defaultBrowser": "",
- "rules": []interface{}{},
- })
return
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/finicky/src/window/window_windows.go` around lines 291 - 300, The
handleGetRules flow is converting every rules.Load() failure into an empty
ruleset, which should only happen when the rules file is actually missing.
Update handleGetRules to distinguish a missing-file case from
parse/permission/other load errors, using rules.Load() and its error handling to
branch accordingly. On non-missing errors, keep logging the failure and surface
the error state instead of sending an empty "rules" payload to
SendMessageToWebView; only return the default browser plus empty rules when the
file truly does not exist.
Comment on lines
+334
to
+349
| if err := rules.Save(rf); err != nil { | ||
| slog.Error("Failed to save rules", "error", err) | ||
| SendMessageToWebView("saveRulesError", map[string]interface{}{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| path, _ := rules.GetPath() | ||
| type rulesResponse struct { | ||
| rules.RulesFile | ||
| Path string `json:"path,omitempty"` | ||
| } | ||
| SendMessageToWebView("rules", rulesResponse{RulesFile: rf, Path: util.ShortenPath(path)}) | ||
|
|
||
| if SaveRulesHandler != nil { | ||
| SaveRulesHandler(rf) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Report runtime rejection before acknowledging save success.
SaveRulesHandler can fail while regenerating the VM in main_windows.go, but this path persists the file and sends "rules" success before the handler runs. Return an error from the handler or validate/rebuild before saving so the UI and runtime cannot diverge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/finicky/src/window/window_windows.go` around lines 334 - 349, The save
flow in window_windows.go acknowledges success too early: rules.Save and the
"rules" WebView message happen before SaveRulesHandler runs, so runtime failures
in the handler can leave the UI out of sync. Update the save path around
SaveRulesHandler to validate/rebuild first or make SaveRulesHandler return an
error and only send the success response after it succeeds; if it fails, emit
the error message instead of the success message.
- launcher: fix duplicate URL + misplaced args in the no-known-exe fallback; open the URL via ShellExecuteW (util.OpenURLDefault) instead of `cmd /c start`, so URL metacharacters can't be re-tokenized by the shell into commands - main: same injection-safe ShellExecuteW fallback for the single-instance URL hand-off (still guarded so it never relaunches Finicky when it's the default) - launcher: drain child stdout/stderr via CombinedOutput to avoid a pipe deadlock - detect: enumerate StartMenuInternet under both HKCU and HKLM (+dedupe) so per-user browser installs (default Chrome/Edge/Firefox) are detected - window: gate WebView2 Debug mode behind FINICKY_DEBUG (off by default) - main: back off on repeated IPC Accept() errors instead of a tight loop - ci: `copy /Y` so the Windows asset copy step can't prompt/hang - docs: correct the winget id (JohnSterling.Finicky) and make the native build example self-contained Pre-existing patterns shared verbatim with the macOS build (vm/configInfo/lastError races, fatal-error continuation, failed-rules-load handling) are left matching the reference implementation rather than diverged in the Windows files. Refs johnste#542 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Thanks @coderabbitai — solid pass. Addressed the port-specific findings in 5ed1293: Fixed
Intentionally not changed (pre-existing, shared verbatim with the macOS build): the Still pending before this leaves draft: a real-Windows smoke test — the runtime WebView2 + thread-affinity behavior is exactly the class of thing neither static review nor macOS-hosted tests can cover. Planned for the next day or two. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show --stat 5ed1293 2>/dev/null | head -50Length of output: 2530
Agreed on leaving the Good luck with the real-Windows smoke test — WebView2 thread-affinity and message-pump timing are exactly the kind of thing that only shows up on real hardware, so that's a sensible next gate before taking this out of draft. (っ◔◡◔)っ 🐰 [resolve] |
chaoz23
added a commit
to chaoz23/finicky
that referenced
this pull request
Jul 4, 2026
…ults error
Three on-box smoke-test findings, all verified on native Windows. Note: the
version bug is the Mac session's F7; the IPC bug is renumbered F8 here to avoid
colliding with it.
F3 (bridge): App.svelte overwrote window.finicky.sendMessage with the macOS-only
window.webkit.messageHandlers path, making every UI->Go request a silent no-op
under WebView2 (getInstalledBrowsers, getRules, saveRules, testUrl all dropped).
Route sendMessage to whichever bridge exists: webkit on macOS,
window.__finicky_send on Windows. This also unblocks F2 (test URL) and the save
path of F4. Browser detection itself was never broken (returns Chrome/IE/Edge) -
the request just never reached Go, which corrects the earlier
"detect_windows.go returns empty" read in F7's reference note.
F8 (IPC): single-instance/keepRunning used net.Listen("unix", ...), which fails
intermittently with WSAEINVAL on Windows and left the URL listener dead. Switch
to a Windows named pipe via go-winio (pipe FinickyBrowserRouter), matching the
Global mutex scope. Two launches now share one process; the second's URL arrives
over the pipe.
F7 (version, Windows half): GetCurrentVersion shelled out to macOS
"defaults read Info.plist" on every startup (3 ERROR lines, empty version).
Split into version_darwin.go (unchanged) and version_windows.go, which reads the
embedded PE version resource via x/sys/windows. Eliminates the errors; now
reports 4.2.2. The Mac-side work to inject the correct (non-stale) version via
ldflags remains open.
Tests made platform-agnostic (assert browser.DefaultBrowserName). All go test /
go vet green. macOS paths unchanged. New direct dep:
github.com/Microsoft/go-winio v0.6.2 (pending Mac-session ratify before
promotion to windows-support / PR johnste#542).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chaoz23
added a commit
to chaoz23/finicky
that referenced
this pull request
Jul 4, 2026
Installer (first real-hardware run): compile with Inno Setup 6, silent
per-user install / uninstall / reinstall all exit 0 with no UAC; ProgID,
finicky:// protocol, Capabilities, RegisteredApplications, autostart Run
key verified in registry and fully removed on uninstall. Flagged: install
dir {localappdata}\Finicky collides with the app cache dir - recommend
{userpf}\Finicky.
Tier 1: OS-level finicky:// handoff via ShellExecute launches the
installed exe, decodes, routes; second launch hands off over the named
pipe to the resident primary (1 process). Remaining: the human
set-default-browser step in Settings.
Tier 2: profile rule (Google Chrome, profile "dan") resolves display
name via Local State and emits --profile-directory=Default. PASS.
MAC_HANDOFF.md: work order for the Mac session - ratify go-winio,
cherry-pick 7babaf3 to windows-support, F7 version ldflags, F2/F1 UI
round, installer polish, PR johnste#542 un-draft. Second-machine validation
tabled by owner decision.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Smoke-test fixes (real Win10): - Default browser is now platform-aware via browser.DefaultBrowserName (Edge on Windows, Safari on macOS) — replaces hardcoded "com.apple.Safari" in resolver and rules. UI no longer seeds the field with "Safari". - Rules editor: ignore the echo of our own saveRules so it doesn't reassign `rules` and steal focus when moving between the browser and URL fields (F4 — new rules couldn't be created under WebView2). - detect_windows: log per-root (HKCU/HKLM) and total browser counts to diagnose empty browser dropdown (F3).
…m wording - versioninfo.json + resource_windows_amd64.syso (goversioninfo) embed finicky.ico and version metadata into the PE; linked for windows/amd64 only, macOS unaffected - About page: "macOS application" -> "Available on macOS and Windows"
…ults error
Three on-box smoke-test findings, all verified on native Windows. Note: the
version bug is the Mac session's F7; the IPC bug is renumbered F8 here to avoid
colliding with it.
F3 (bridge): App.svelte overwrote window.finicky.sendMessage with the macOS-only
window.webkit.messageHandlers path, making every UI->Go request a silent no-op
under WebView2 (getInstalledBrowsers, getRules, saveRules, testUrl all dropped).
Route sendMessage to whichever bridge exists: webkit on macOS,
window.__finicky_send on Windows. This also unblocks F2 (test URL) and the save
path of F4. Browser detection itself was never broken (returns Chrome/IE/Edge) -
the request just never reached Go, which corrects the earlier
"detect_windows.go returns empty" read in F7's reference note.
F8 (IPC): single-instance/keepRunning used net.Listen("unix", ...), which fails
intermittently with WSAEINVAL on Windows and left the URL listener dead. Switch
to a Windows named pipe via go-winio (pipe FinickyBrowserRouter), matching the
Global mutex scope. Two launches now share one process; the second's URL arrives
over the pipe.
F7 (version, Windows half): GetCurrentVersion shelled out to macOS
"defaults read Info.plist" on every startup (3 ERROR lines, empty version).
Split into version_darwin.go (unchanged) and version_windows.go, which reads the
embedded PE version resource via x/sys/windows. Eliminates the errors; now
reports 4.2.2. The Mac-side work to inject the correct (non-stale) version via
ldflags remains open.
Tests made platform-agnostic (assert browser.DefaultBrowserName). All go test /
go vet green. macOS paths unchanged. New direct dep:
github.com/Microsoft/go-winio v0.6.2 (pending Mac-session ratify before
promotion to windows-support / PR johnste#542).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Win11's Default Apps picker never offered Finicky with the shipped registration. Three stacked causes, diagnosed live on Win11 Home 26200: 1. Registration shape too minimal: the picker filters http/https candidates to apps that also declare Capabilities\FileAssociations (.htm/.html), Capabilities\Startmenu, and InstallInfo. Added all three to installer.iss and finicky-register.reg. 2. Missing SHChangeNotify(SHCNE_ASSOCCHANGED): the installer's [Code] stub skipped it as "optional polish" - it is required. Now imported from shell32 and called at ssPostInstall. 3. HKCU alone was insufficient on this build: the picker listed Finicky only after the registration was mirrored to HKLM. Strategy decision (elevate installer vs stay per-user) escalated to MAC_HANDOFF.md / PR review - not changed unilaterally here. Also: the picker labels entries with the exe's FileDescription, which read "Rule-based browser router". Windows convention (Chrome: "Google Chrome") is the app display name - changed to "Finicky" in versioninfo.json and regenerated resource_windows_amd64.syso. Verified on box: Finicky now appears in the HTTPS-link picker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ch is the windowless resident-router mode; with a primary running it exits silently, so the shortcut appeared dead). Verified: --window with resident primary opens the window via the show-window pipe request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Win11 26200 records the user's http/https choice under UserChoiceLatest\ProgId and leaves the legacy UserChoice key permanently stale, so isDefaultBrowser reported false while Finicky was demonstrably routing clicks. Prefer UserChoiceLatest, fall back to UserChoice for older builds. Verified on box: 'Finicky is the default browser' now logs; choice survived a reboot; Tier 1 complete. Also logs F13 (Run-key autostart never executes at logon - mitigation applied, verify next boot) and closes the stale-label question as this-box state-repo debris. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… fallback (F10 decision) Per-user (HKCU-only) registration never appears in the Win11 26200 Default Apps picker; machine-wide registration works. Default to admin install: HKA registry roots (HKLM in admin mode, HKCU per-user), Program Files install dir (also un-collides install dir from the app cache dir), machine-wide autostart. /CURRENTUSER preserved for no-admin environments. Verified on box: install/uninstall clean, exe in Program Files, HKLM registration correct, default-browser choice survives reinstall, router logs 'Finicky is the default browser'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Product owner guidance: target audience is personal, unmanaged machines (no prior Windows install base to migrate). Skip the all-users/current-user question on interactive installs; /CURRENTUSER stays available via command line only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GetCurrentVersion now returns the ldflags-injected finicky/version.version (from git describe --tags --match 'v*') and falls back to platform metadata (Info.plist / PE resource) for bare builds. build-windows.sh injects it; installer.iss version is CI-overridable via /DMyAppVersion; versioninfo.json bumped off the stale 4.2.2 to 4.4.0-alpha. Verified: log shows version=v4.4.0-alpha-25-g9b6c1db. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test-url round trip had no failure mode: a dead bridge or hung resolver spun the loading indicator forever, and a backend error payload rendered as an empty result grid. Add a 5s response timeout with a clear message, and render backend {error} payloads as an error box. Also marks F1 fully complete - the StartPage Safari seed was already removed on this branch (findings doc was stale).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Real-hardware smoke test complete (Windows 11) — branch updated with 9 fix commitsWe ran a full smoke test of this branch natively on Windows 11 Home (build 26200): built from source (Go 1.26 / Node 24), exercised the CLI, config window, rules editor, installer lifecycle, and default-browser integration end-to-end. 13 findings were root-caused; the fixes are now on this branch. Full evidence log: Highlights, most significant first:
Verified working on hardware: routing via CLI, macOS impact of shared-file changes was kept intentionally minimal (webkit bridge tried first; darwin version path unchanged; tests assert the platform constant). macOS CI verification is the remaining checkbox before un-drafting. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The macOS job on windows-* branches is the shared-file regression guard (App.svelte, version.go etc. compile into the macOS app too) - name now says so. Windows build joins it so the Actions list reads as a two-platform matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… asset-copy steps); inject version; fetch tags Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/macos-build.yml (1)
2-138: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit
permissions:block.No top-level
permissionsis defined, so jobs run with default (potentially broad)GITHUB_TOKENpermissions. Since this workflow now also triggers on the newwindows-support/windows-smokebranches and includes a signing job with sensitive secrets, scoping to least privilege (e.g.contents: read) is good practice.🔒 Suggested fix
name: macOS Build (cross-platform regression check) +permissions: + contents: read + on:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/macos-build.yml around lines 2 - 138, Add an explicit top-level permissions block to the macOS workflow so the default GITHUB_TOKEN is not overprivileged. Update the workflow around the on/jobs section to scope access to least privilege, likely using contents: read for the checkout and artifact steps, and keep the build and sign-and-notarize jobs working with the named steps that use actions/checkout, actions/upload-artifact, and actions/download-artifact.Source: Linters/SAST tools
.github/workflows/windows-build.yml (2)
15-18: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider disabling credential persistence on checkout.
Static analysis flags that this checkout step doesn't set
persist-credentials: false. The default persistsGITHUB_TOKENin the local git config for the rest of the job, which subsequent steps likenpm ci(third-party install scripts) orgo build(module fetches) could read/exfiltrate.🔒 Suggested fix
- name: Checkout code uses: actions/checkout@v4 with: fetch-depth: 0 # full history + tags so `git describe` yields the real version + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/windows-build.yml around lines 15 - 18, The Checkout code step in the Windows build workflow is persisting credentials by default, which leaves GITHUB_TOKEN available to later steps. Update the actions/checkout@v4 configuration in the checkout step to explicitly disable credential persistence while keeping the existing fetch-depth behavior, so subsequent steps like npm ci or go build cannot read the token from git config.Source: Linters/SAST tools
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm these branch triggers are temporary.
windows-supportandwindows-smokelook like scratch branches for this draft port; worth pruning from the trigger list once the branch is merged/closed to avoid stale CI runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/windows-build.yml at line 5, The branch trigger list in the Windows build workflow includes temporary scratch branches that should not remain long term. Update the workflow’s branch filter to keep only the intended permanent branch entries, and remove `windows-support` and `windows-smoke` from the trigger list once they are no longer needed in the `branches` configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/finicky/src/version/version.go`:
- Around line 113-116: The orphaned doc comment is sitting above checkForUpdates
and now reads like its documentation, but it actually describes
GetCurrentVersion which is already documented elsewhere. Remove this stray
comment block from above checkForUpdates, or move it back to the
GetCurrentVersion declaration if it is still needed there, so checkForUpdates is
not mislabeled by unrelated docs.
---
Nitpick comments:
In @.github/workflows/macos-build.yml:
- Around line 2-138: Add an explicit top-level permissions block to the macOS
workflow so the default GITHUB_TOKEN is not overprivileged. Update the workflow
around the on/jobs section to scope access to least privilege, likely using
contents: read for the checkout and artifact steps, and keep the build and
sign-and-notarize jobs working with the named steps that use actions/checkout,
actions/upload-artifact, and actions/download-artifact.
In @.github/workflows/windows-build.yml:
- Around line 15-18: The Checkout code step in the Windows build workflow is
persisting credentials by default, which leaves GITHUB_TOKEN available to later
steps. Update the actions/checkout@v4 configuration in the checkout step to
explicitly disable credential persistence while keeping the existing fetch-depth
behavior, so subsequent steps like npm ci or go build cannot read the token from
git config.
- Line 5: The branch trigger list in the Windows build workflow includes
temporary scratch branches that should not remain long term. Update the
workflow’s branch filter to keep only the intended permanent branch entries, and
remove `windows-support` and `windows-smoke` from the trigger list once they are
no longer needed in the `branches` configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c0801cb-d00a-40b3-a39b-2194c87b1d7c
⛔ Files ignored due to path filters (1)
apps/finicky/src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
.github/workflows/macos-build.yml.github/workflows/windows-build.ymlREADME.mdapps/finicky/src/browser/default_browser_darwin.goapps/finicky/src/browser/default_browser_windows.goapps/finicky/src/browser/detect_windows.goapps/finicky/src/browser/launcher_windows.goapps/finicky/src/browser_windows.goapps/finicky/src/go.modapps/finicky/src/main_windows.goapps/finicky/src/resolver/resolver.goapps/finicky/src/resolver/resolver_test.goapps/finicky/src/resource_windows_amd64.sysoapps/finicky/src/rules/rules.goapps/finicky/src/rules/rules_test.goapps/finicky/src/util/shellexec_windows.goapps/finicky/src/version/version.goapps/finicky/src/version/version_darwin.goapps/finicky/src/version/version_windows.goapps/finicky/src/versioninfo.jsonapps/finicky/src/window/window_windows.gopackages/finicky-ui/src/App.sveltepackages/finicky-ui/src/lib/testUrlStore.tspackages/finicky-ui/src/pages/About.sveltepackages/finicky-ui/src/pages/Rules.sveltepackages/finicky-ui/src/pages/StartPage.sveltepackages/finicky-ui/src/pages/TestUrl.sveltepackages/finicky-ui/src/types.tsscripts/build-windows.shscripts/installer.iss
✅ Files skipped from review due to trivial changes (6)
- apps/finicky/src/versioninfo.json
- packages/finicky-ui/src/pages/About.svelte
- apps/finicky/src/browser/default_browser_windows.go
- apps/finicky/src/browser/default_browser_darwin.go
- apps/finicky/src/resolver/resolver_test.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/build-windows.sh
- apps/finicky/src/browser_windows.go
- apps/finicky/src/go.mod
- apps/finicky/src/browser/detect_windows.go
- apps/finicky/src/main_windows.go
- apps/finicky/src/window/window_windows.go
- scripts/installer.iss
Comment on lines
+113
to
+116
| // GetCurrentVersion returns the running build's version. Its implementation is | ||
| // platform-specific (version_darwin.go reads Info.plist; version_windows.go | ||
| // reads the PE version resource) because the two platforms package version | ||
| // metadata differently. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Orphaned doc comment now misleadingly documents checkForUpdates.
This comment block describes GetCurrentVersion's platform-specific fallback, but GetCurrentVersion is already defined and documented above (lines 47-54). As written, this comment now sits directly above the unrelated checkForUpdates function, making it look like checkForUpdates's doc comment.
🧹 Proposed fix
-// GetCurrentVersion returns the running build's version. Its implementation is
-// platform-specific (version_darwin.go reads Info.plist; version_windows.go
-// reads the PE version resource) because the two platforms package version
-// metadata differently.
-
func checkForUpdates() (releaseInfo *ReleaseInfo) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GetCurrentVersion returns the running build's version. Its implementation is | |
| // platform-specific (version_darwin.go reads Info.plist; version_windows.go | |
| // reads the PE version resource) because the two platforms package version | |
| // metadata differently. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/finicky/src/version/version.go` around lines 113 - 116, The orphaned doc
comment is sitting above checkForUpdates and now reads like its documentation,
but it actually describes GetCurrentVersion which is already documented
elsewhere. Remove this stray comment block from above checkForUpdates, or move
it back to the GetCurrentVersion declaration if it is still needed there, so
checkForUpdates is not mislabeled by unrelated docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — opened per the discussion in #538 so the Windows surface area is concrete and reviewable. Zero obligation to merge. If you'd rather I maintain this as a fork, that's completely fine too.
What this does
Adds a Windows platform layer behind
//go:build windows, mirroring the existing_darwinsplit. Most of the v4 codebase (Go / Svelte / TS) is already cross-platform; this fills in the platform-specific remainder.Your macOS build is unaffected. Every new file is
//go:build windows; the changes to existing files are one-line//go:build darwintags plus a couple of small cross-platform helpers (e.g.LogDir()).go build/go teststill pass on darwin with no behavior change.Approach
--window) from secondary launches to the resident instance. Same pattern VS Code / Slack use.jchv/go-webview2(pure Go, no cgo, so it still cross-compiles from macOS). A localhost HTTP server replacesWKURLSchemeHandler; the JS↔Go bridge usesBind(). The window is created and message-pumped on theLockOSThreadmain thread (Win32 delivers window messages only to the creating thread).StartMenuInternetregistry + known install paths;config_dir_windowsadded tobrowsers.jsonfor Chromium / Firefox profile resolution (%LOCALAPPDATA%/%APPDATA%).On maintenance (the concern from #538)
I'm offering to own the Windows side — triage and fix Windows issues, keep the platform layer current as the core evolves, and handle the Windows release artifacts. Windows CI is separate so it won't gate your macOS releases.
WINDOWS_PORT_QC.mddocuments the platform layer and a reusable QC checklist (including a thread-affinity audit that caught a WebView2 message-loop bug which only surfaces on real hardware), so it isn't a black box for you.Status / not yet done
GOOS=windows CGO_ENABLED=0) and unit tests pass; the runtime smoke test (protocol handler, WebView2 render, IPC hand-off) is the next step and why this is a draft.Refs #538
🤖 Generated with Claude Code
Summary by CodeRabbit