Conversation
There was a problem hiding this comment.
Pull request overview
Implements Issue #380 by introducing a separate JSON-backed “rules” configuration that can be edited from the UI, plus plumbing to load/save rules and merge them with an existing JS config (JS handlers take priority).
Changes:
- Add
rules.jsonmodel + persistence in the Go app, including conversion into finickyConfig-compatible handlers and VM rebuild when running without JS config. - Add a new Rules UI page with browser/profile selection and rule editing (including reordering) and wire it into app navigation and native messaging.
- Extend config/runtime wiring (resolver, window messaging, installed browsers + profiles, config watcher debounce) to support the new UI configuration flow.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/rules.json | Example rules.json fixture for dev/testing scenarios. |
| testdata/config.js | Example JS config fixture to test precedence vs rules.json. |
| scripts/test.sh | Convenience script to run Go tests from apps/finicky/src. |
| scripts/dev.sh | Dev runner for multiple config/rules scenarios + mock updates. |
| packages/finicky-ui/src/utils/text.ts | Stops exporting internal helper used by log formatting. |
| packages/finicky-ui/src/types.ts | Adds UI types for rules file + exposes isJSConfig in config info. |
| packages/finicky-ui/src/pages/TestUrl.svelte | Persists test URL input via a store and updates styling. |
| packages/finicky-ui/src/pages/StartPage.svelte | Adds editable preferences UI (default browser/profile + options) with JS-config locking. |
| packages/finicky-ui/src/pages/Rules.svelte | New Rules page (edit rules, patterns, drag reorder, save debounce, profiles fetch). |
| packages/finicky-ui/src/pages/LogViewer.svelte | UI tweak (checkbox accent color). |
| packages/finicky-ui/src/pages/About.svelte | Layout/style refresh of About page content. |
| packages/finicky-ui/src/lib/testUrlStore.ts | Adds store for test URL input persistence. |
| packages/finicky-ui/src/components/TabBar.svelte | Adds Rules tab and reorganizes tab groups + nav styling. |
| packages/finicky-ui/src/components/PageContainer.svelte | Allows HTML in descriptions and tweaks layout spacing/typography. |
| packages/finicky-ui/src/components/OptionRow.svelte | New reusable toggle row with locked-state UX. |
| packages/finicky-ui/src/components/icons/Rules.svelte | New Rules icon. |
| packages/finicky-ui/src/components/icons/Lock.svelte | New Lock icon used for readonly/locked controls. |
| packages/finicky-ui/src/components/BrowserProfileSelector.svelte | New browser + optional profile selector used by Rules/Preferences. |
| packages/finicky-ui/src/App.svelte | Wires new routes/messages (rules, installed browsers, profiles) + footer config badges. |
| packages/finicky-ui/src/app.css | Introduces new theme variables and updates overall styling. |
| apps/finicky/src/window/window.go | Adds WebView message handlers for rules CRUD + installed browsers/profiles. |
| apps/finicky/src/version/version.go | Supports FINICKY_MOCK_UPDATE env var for UI/dev testing. |
| apps/finicky/src/util/directories.go | Adds ShortenPath helper for user-facing path display. |
| apps/finicky/src/rules/rules.go | New rules package: load/save rules.json + JSON<->JS config conversion. |
| apps/finicky/src/rules/rules_test.go | Unit tests for rules package behavior. |
| apps/finicky/src/resolver/resolver.go | New resolver that merges cached JSON rules with JS config evaluation. |
| apps/finicky/src/resolver/resolver_test.go | Tests for resolver behavior across no-config/JS-only/JSON-only/merged cases. |
| apps/finicky/src/main.m | Ignores spurious openFile calls during launch (CLI flag paths). |
| apps/finicky/src/main.h | Adds didFinishLaunching state for launch-time filtering. |
| apps/finicky/src/main.go | Adds --rules/--no-config, VM/rules wiring, and switches URL evaluation to resolver. |
| apps/finicky/src/config/vm.go | Refactors VM creation to accept API bytes + adds IsJSConfig + bulk option retrieval. |
| apps/finicky/src/config/configfiles.go | Debounces config file watcher notifications. |
| apps/finicky/src/browser/launcher.go | Adds browser profile enumeration and refactors profile parsing helpers. |
| apps/finicky/src/browser/detect.go | Adds installed browser detection via Launch Services (Cgo/AppKit). |
| apps/finicky/src/browser/browsers.json | Adds Safari entry (type recognized for completeness). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class="header-section"> | ||
| <h2>{title}</h2> | ||
| {#if description} | ||
| <p class="page-description">{description}</p> | ||
| <p class="page-description">{@html description}</p> | ||
| {/if} |
There was a problem hiding this comment.
{@html description} renders raw HTML. Since description is a plain string prop, this makes it easy for future call sites to accidentally pass user-controlled content and introduce XSS. Consider keeping description as text and adding a separate explicitly-named prop (e.g. descriptionHtml) or restricting/escaping input before rendering as HTML.
| @@ -1,3 +1,22 @@ | |||
| export interface Rule { | |||
| match: string[]; | |||
There was a problem hiding this comment.
Rule.match is typed as string[], but the backend JSON format supports both a single string and an array (and rules.go will marshal a single pattern as a string). This forces unsafe casts in Rules.svelte and can hide bugs; update the type to string | string[] (or introduce a separate JSON DTO type) and normalize to an array at the UI boundary.
| match: string[]; | |
| match: string | string[]; |
| func TestToJSHandlers_SkipsIncompleteRules(t *testing.T) { | ||
| rules := []Rule{ | ||
| {Match: "", Browser: "Firefox"}, // no match | ||
| {Match: "example.com", Browser: ""}, // no browser | ||
| {Match: "example.com", Browser: "Safari"}, // valid | ||
| } |
There was a problem hiding this comment.
These tests construct Rule values using Match: "...", but Rule.Match is defined as []string in rules.go. This won’t compile; update the test data to use slices (e.g. Match: []string{"..."}) or adjust the struct/test helpers accordingly.
| func TestToJSHandlers_StringBrowser(t *testing.T) { | ||
| rules := []Rule{ | ||
| {Match: "*github.com/*", Browser: "Firefox"}, | ||
| } | ||
| result := ToJSHandlers(rules) |
There was a problem hiding this comment.
Same issue here: Rule.Match is a []string, but the test uses Match: "*github.com/*". Update to []string{"*github.com/*"} (and similarly for other rules in this test file) so it compiles and matches runtime behavior.
| func TestToJSConfigScript_DefaultBrowserFallback(t *testing.T) { | ||
| rf := RulesFile{Rules: []Rule{{Match: "example.com", Browser: "Firefox"}}} | ||
| script, err := ToJSConfigScript(rf, "finickyConfig") | ||
| if err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
RulesFile{Rules: []Rule{{Match: "example.com", ...}}} uses Match as a string, but Rule.Match is a []string. This will fail to compile; use Match: []string{"example.com"}.
apps/finicky/src/rules/rules_test.go
Outdated
| {Match: "*github.com/*", Browser: "Google Chrome", Profile: "Personal"}, | ||
| {Match: "https://linear.app/*", Browser: "Safari"}, |
There was a problem hiding this comment.
In this round-trip test, rule literals use Match: "..." even though Rule.Match is []string. Update both entries to []string{...} so the test compiles and actually exercises the JSON single-string marshal/unmarshal behavior you want to verify.
| {Match: "*github.com/*", Browser: "Google Chrome", Profile: "Personal"}, | |
| {Match: "https://linear.app/*", Browser: "Safari"}, | |
| {Match: []string{"*github.com/*"}, Browser: "Google Chrome", Profile: "Personal"}, | |
| {Match: []string{"https://linear.app/*"}, Browser: "Safari"}, |
| // Debounce: reset the timer so only the last event in a burst fires. | ||
| cfw.debounceMu.Lock() | ||
| if cfw.debounceTimer != nil { | ||
| cfw.debounceTimer.Stop() | ||
| } | ||
| notify := cfw.configChangeNotify | ||
| cfw.debounceTimer = time.AfterFunc(500*time.Millisecond, func() { | ||
| notify <- struct{}{} | ||
| }) |
There was a problem hiding this comment.
The debounce timer callback sends on an unbuffered configChangeNotify channel from a timer goroutine. If the receiver is busy (e.g. resolving/launching a browser), this send can block and potentially accumulate stuck goroutines after successive file events. Consider making the channel buffered (size 1) and/or doing a non-blocking send (select with default) to coalesce bursts safely.
| // Set up rules save handler. | ||
| // When there is no JS config, rebuild the VM from the updated rules. | ||
| // When there is a JS config, JSON rules are loaded fresh in evaluateURL — nothing to do. | ||
| window.SaveRulesHandler = func(rf rules.RulesFile) { | ||
| slog.Debug("Rules updated", "count", len(rf.Rules)) | ||
| resolver.SetCachedRules(rf) | ||
| if vm == nil || !vm.IsJSConfig() { | ||
| if rf.DefaultBrowser == "" && len(rf.Rules) == 0 { | ||
| vm = nil | ||
| return | ||
| } | ||
| script, err := rules.ToJSConfigScript(rf, namespace) | ||
| if err != nil { | ||
| slog.Error("Failed to generate config from rules", "error", err) | ||
| return | ||
| } | ||
| newVM, err := config.NewFromScript(finickyConfigAPIJS, namespace, script) | ||
| if err != nil { | ||
| slog.Error("Failed to rebuild VM from rules", "error", err) | ||
| return | ||
| } | ||
| vm = newVM | ||
| shouldKeepRunning = vm.GetAllConfigOptions().KeepRunning | ||
| go checkForUpdates() | ||
| } | ||
| } |
There was a problem hiding this comment.
window.SaveRulesHandler mutates the global vm (and triggers update checks) from the WebView message handler path, while vm is also read/used concurrently in the main event-loop goroutine (URL resolution and config reload). This introduces a real data race and can lead to crashes or inconsistent config evaluation. Route rule-save events through the existing select loop via a channel (or guard all vm reads/writes with a mutex) so VM rebuilds and shouldKeepRunning updates happen in one place.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a JSON rules-based browser configuration system alongside JavaScript configs. It adds browser detection via macOS Launch Services, profile discovery, URL-to-browser resolution logic, debounced config file watching, and an interactive Rules editor UI with browser/profile selectors, alongside comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Safari as Safari/Browser
participant App as Finicky App
participant Resolver as Resolver
participant Config as JS Config
participant Rules as JSON Rules
participant Browser as Launch Browser
User->>Safari: Opens finicky:// link
Safari->>App: Calls handleURL
alt JS Config Enabled
App->>Resolver: ResolveURL(vm, url, opener)
Resolver->>Config: Evaluate JS handlers
Config-->>Resolver: Selected browser + profile
else JSON Rules Only
App->>Resolver: ResolveURL(nil, url, opener)
Resolver->>Rules: Check cached rules
Rules-->>Resolver: Selected browser + profile
end
Resolver-->>App: BrowserConfig + error
App->>Browser: Launch with profile
Browser-->>User: Opens URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/finicky-ui/src/pages/TestUrl.svelte (1)
61-71:⚠️ Potential issue | 🟡 MinorSubscription cleanup not handled on component destroy.
The
testUrlResult.subscribe()call at line 62 returns an unsubscribe function that is never called. In Svelte, manual subscriptions outside of$:reactive statements or template$storesyntax need explicit cleanup to prevent memory leaks when the component is destroyed.🛠️ Proposed fix using onDestroy
<script lang="ts"> + import { onDestroy } from "svelte"; import PageContainer from "../components/PageContainer.svelte"; // ... other imports // Subscribe to test results - testUrlResult.subscribe(() => { + const unsubscribe = testUrlResult.subscribe(() => { clearTimeout(loadingTimer); loading = false; }); + + onDestroy(() => { + unsubscribe(); + clearTimeout(debounceTimer); + clearTimeout(loadingTimer); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/pages/TestUrl.svelte` around lines 61 - 71, The subscription to testUrlResult is never cleaned up; capture the unsubscribe function returned by testUrlResult.subscribe(...) (e.g., const unsubscribe = testUrlResult.subscribe(...)) and register cleanup in Svelte's onDestroy by calling unsubscribe() (and also clearTimeout(loadingTimer) if still pending) to avoid memory leaks; modify the block around testUrlResult.subscribe, referencing testUrlResult, loadingTimer, and loading, and add an onDestroy import/use to call unsubscribe() and any timeout cleanup.apps/finicky/src/main.go (1)
257-287:⚠️ Potential issue | 🟠 MajorPreserve
nilwhen Launch Services provides no opener.This always enqueues
&opener, so the resolver sees a truthy opener object with empty fields instead ofnull. JS handlers that distinguish “no opener” from “known opener” will misroute.Suggested fix
func HandleURL(url *C.char, name *C.char, bundleId *C.char, path *C.char, windowTitle *C.char, openInBackground C.bool) { - var opener resolver.OpenerInfo + var opener *resolver.OpenerInfo if name != nil && bundleId != nil && path != nil { - opener = resolver.OpenerInfo{ + opener = &resolver.OpenerInfo{ Name: C.GoString(name), BundleID: C.GoString(bundleId), Path: C.GoString(path), } if windowTitle != nil { opener.WindowTitle = C.GoString(windowTitle) @@ urlListener <- URLInfo{ URL: urlString, - Opener: &opener, + Opener: opener, OpenInBackground: bool(openInBackground), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/main.go` around lines 257 - 287, The code always enqueues a non-nil pointer to a zero-value resolver.OpenerInfo even when Launch Services provided no opener; change the local opener to be a pointer (var opener *resolver.OpenerInfo) and only allocate & populate it inside the existing if (name != nil && bundleId != nil && path != nil) block (set WindowTitle on that allocated struct if provided). Then pass opener (which will be nil when no opener was provided) into URLInfo so JS can distinguish “no opener” from an empty opener.
🧹 Nitpick comments (13)
packages/finicky-ui/src/types.ts (1)
7-18: Consider reusingConfigOptionsforoptionsshapes.
RulesFile.optionsandConfigInfo.optionsduplicate the same optional fields. ReusingPartial<ConfigOptions>reduces drift risk.♻️ Suggested refactor
export interface RulesFile { defaultBrowser: string; defaultProfile?: string; - options?: { - keepRunning?: boolean; - hideIcon?: boolean; - logRequests?: boolean; - checkForUpdates?: boolean; - }; + options?: Partial<ConfigOptions>; rules: Rule[]; path?: string; } ... export interface ConfigInfo { configPath: string; isJSConfig?: boolean; handlers?: number; rewrites?: number; defaultBrowser?: string; - options?: { - keepRunning?: boolean; - hideIcon?: boolean; - logRequests?: boolean; - checkForUpdates?: boolean; - }; + options?: Partial<ConfigOptions>; }Also applies to: 67-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/types.ts` around lines 7 - 18, RulesFile defines an inline options shape that duplicates ConfigInfo.options; change both RulesFile.options and ConfigInfo.options to reuse the existing ConfigOptions type by declaring them as Partial<ConfigOptions> (or ConfigOptions | undefined if you prefer full type) to avoid duplicated field definitions—update the interface references to use Partial<ConfigOptions> and remove the duplicated inline object shape in RulesFile and the corresponding duplicated definition in ConfigInfo so the shared ConfigOptions type is the single source of truth.packages/finicky-ui/src/components/icons/Rules.svelte (1)
1-13: Apply the same SVG accessibility treatment here.For decorative use, mark this SVG with
aria-hidden="true"(and optionallyfocusable="false").♿ Suggested tweak
<svg xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + focusable="false" width="24" height="24"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/icons/Rules.svelte` around lines 1 - 13, The Rules.svelte SVG is missing accessibility attributes for decorative icons; update the <svg> element in Rules.svelte (the SVG with class "lucide lucide-list-filter") to include aria-hidden="true" and optionally focusable="false" so it is ignored by assistive technologies when used decoratively, keeping no change to visual rendering or other attributes.packages/finicky-ui/src/components/icons/Lock.svelte (1)
1-15: Mark decorative icon SVG as non-accessible.If this icon is decorative, add
aria-hidden="true"(and optionallyfocusable="false") to avoid extra screen reader noise.♿ Suggested tweak
<svg xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + focusable="false" width="16" height="16"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/icons/Lock.svelte` around lines 1 - 15, The Lock.svelte SVG is decorative and should be hidden from assistive tech; update the root <svg> element (the one with class "lucide lucide-lock" in Lock.svelte) to include aria-hidden="true" and optionally focusable="false" so screen readers skip it; if this icon ever becomes meaningful, replace those attributes with an accessible title/role instead.packages/finicky-ui/src/app.css (1)
1-44: Consider consolidating the two:rootblocks.There are two separate
:rootblocks (lines 1-15 and 17-44). While valid CSS, this split reduces maintainability. The first block sets base styles and explicit color values, while the second defines CSS custom properties. Consider merging them into a single:rootblock for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/app.css` around lines 1 - 44, The file contains two separate :root blocks—one with base font and color-scheme properties and another with CSS custom properties (e.g., --bg-primary, --nav-text, --accent-color); merge them into a single :root block by moving the base declarations (font-family, line-height, font-weight, color-scheme, color, background-color, font-synthesis, text-rendering, -webkit-font-smoothing, -moz-osx-font-smoothing) together with all custom property definitions (the --* variables) and remove the duplicate :root to improve maintainability and avoid fragmentation.packages/finicky-ui/src/pages/TestUrl.svelte (1)
216-218: Hard-coded color values reduce theme consistency.The
.hint-messageuses hard-codedrgba(245, 188, 28, ...)values instead of theme variables. Consider usingvar(--log-warning)with appropriate alpha for consistency with the rest of the theme system.♻️ Suggested fix
.hint-message { display: flex; align-items: center; gap: 10px; padding: 12px 16px; - background: rgba(245, 188, 28, 0.06); - border: 1px solid rgba(245, 188, 28, 0.25); + background: color-mix(in srgb, var(--log-warning) 6%, transparent); + border: 1px solid color-mix(in srgb, var(--log-warning) 25%, transparent); border-radius: 8px;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/pages/TestUrl.svelte` around lines 216 - 218, Replace the hard-coded rgba colors in the .hint-message style with the theme variable so the component uses the shared color token; specifically, update the background and border that currently use rgba(245, 188, 28, ...) to use var(--log-warning) with appropriate alpha (e.g., via color-mix() or by using an RGB CSS variable and applying alpha) in the .hint-message rule inside TestUrl.svelte so the hint matches the app theme.packages/finicky-ui/src/components/OptionRow.svelte (2)
120-143: Hard-coded colors in toggle styling reduce theme flexibility.The toggle slider uses hard-coded colors (
#666,#ddd,#111111) instead of CSS variables. This may cause visual inconsistency if the theme changes or in high-contrast modes.♻️ Suggested theme-aware colors
.toggle-slider { position: absolute; cursor: pointer; top: 0; left: 0; right: 0; bottom: 0; - background-color: `#666`; + background-color: var(--text-secondary); transition: 0.3s; border-radius: 24px; } .toggle-slider:before { position: absolute; content: ""; height: 18px; width: 18px; left: 3px; bottom: 3px; - background-color: `#ddd`; + background-color: var(--bg-primary); transition: 0.3s; border-radius: 50%; } .toggle input:checked + .toggle-slider:before { transform: translateX(20px); - background-color: `#111111`; + background-color: var(--bg-primary); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/OptionRow.svelte` around lines 120 - 143, Replace the three hard-coded color values used in the toggle CSS (.toggle-slider background-color `#666`, .toggle-slider:before background-color `#ddd`, and checked .toggle-slider:before background-color `#111111`) with theme-aware CSS variables (e.g., use existing --toggle-active for the checked track and add/choose variables such as --toggle-bg and --toggle-thumb for the track and thumb defaults) so the rules in .toggle-slider, .toggle-slider:before, and .toggle input:checked + .toggle-slider:before derive colors from variables instead of fixed hex values; ensure sensible fallback values are provided in the root or component :root so themes without those variables still render predictably.
27-28: Verifyonchangehandler binding syntax for Svelte 5.In Svelte 5, using
{onchange}as a shorthand on native elements binds to the DOM event. However, the prop type is() => voidwhile DOMonchangeevents pass anEventargument. Ifonchangeis meant to be called on state change, this works, but if consumers expect event data, the signature mismatch could cause confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/OptionRow.svelte` around lines 27 - 28, The current shorthand {onchange} on the input in OptionRow.svelte can mismatch Svelte 5 DOM event typing: decide whether the exported prop onchange should receive the DOM Event or be a no-arg callback; if it should receive the Event, replace the shorthand with an explicit event binding on:change={onchange} and update the prop type to (e: Event) => void, otherwise wrap the call to drop the event payload (on:change={() => onchange?.()}) or change the prop name to something like onToggle to make the no-arg intent explicit; ensure this change is applied alongside the existing bind:checked and locked usage so consumers get the expected signature.packages/finicky-ui/src/pages/Rules.svelte (2)
176-183: Draggable list item missing keyboard accessibility.The rule rows are draggable via mouse but lack keyboard support for reordering. Users relying on keyboard navigation cannot reorder rules. Consider adding keyboard event handlers (e.g., arrow keys with modifier) or ARIA live regions for accessibility compliance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/pages/Rules.svelte` around lines 176 - 183, The draggable rule rows (element with class "rule-row", using dragIndex and handlers onDragStart/onDragOver/onDragEnd) lack keyboard accessibility; add keyboard support by making each rule-row focusable (tabindex="0"), manage an "active/dragging" state via keyboard (e.g., onKeyDown onRuleKeyDown) that starts a keyboard-reorder mode on a modifier key (Space or Enter) and then moves the item with ArrowUp/ArrowDown to call the same reorder logic used by onDragStart/onDragOver (or a shared moveRule(indexFrom, indexTo) helper), update ARIA attributes (aria-grabbed, role="option" or "listitem", aria-posinset/aria-setsize) while reordering, and announce changes via a small ARIA live region when an item is picked up, moved, or dropped so keyboard users receive feedback; wire cleanup to onKeyUp/Escape to cancel and ensure onDragEnd behavior is mirrored.
162-167: Inline HTML in description prop bypasses content sanitization.Using inline HTML with
styleattributes in the description (line 165) works because it's hardcoded, but this pattern could become a security risk if the description ever accepts dynamic content. Consider using a Svelte component or CSS class instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/pages/Rules.svelte` around lines 162 - 167, The description prop currently embeds inline HTML with a style attribute in the PageContainer usage (PageContainer, description, isJSConfig) which bypasses sanitization; replace that inline HTML by creating a small presentational Svelte component (e.g., JsConfigBadge or DescriptionWithBadge) or add a CSS class (e.g., .accent-badge) and use a plain text/element slot on PageContainer instead of passing raw HTML in the description prop; update the PageContainer API (or its consumer) to accept a description slot or component and revise the Rules.svelte usage to render <JsConfigBadge/> or a <span class="accent-badge">JS config is active</span> so styling is done via CSS and no inline style HTML is injected.packages/finicky-ui/src/components/BrowserProfileSelector.svelte (2)
204-207: Hardcoded color breaks the CSS variable pattern.The clear button hover uses a hardcoded color
#f44336while the rest of the component uses CSS custom properties. Consider using a CSS variable for consistency with the design system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/BrowserProfileSelector.svelte` around lines 204 - 207, The .clear-btn:hover rule in BrowserProfileSelector.svelte uses a hardcoded color `#f44336`; change it to use the component/design-system CSS variable (e.g., replace with var(--color-danger) or the project's equivalent like var(--fg-danger)), and include a sensible fallback (e.g., var(--color-danger, `#f44336`)) to preserve behavior if the variable is missing; update the .clear-btn:hover selector to reference that CSS custom property so the component follows the existing variable pattern.
82-90: Consider adding accessibility attributes to form controls.The
<input>elements and<select>elements lack associated<label>elements oraria-labelattributes. The clear buttons also lack accessible names. This could impact keyboard and screen reader users.♿ Suggested accessibility improvements
<input class="browser-input" type="text" placeholder="e.g. Firefox" + aria-label="Custom browser name" value={browser} oninput={handleBrowserInput} {disabled} /> - <button class="clear-btn" onclick={handleClearBrowser}>✕</button> + <button class="clear-btn" onclick={handleClearBrowser} aria-label="Clear browser">✕</button>Apply similar changes to the profile input and clear button as well.
Also applies to: 108-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/BrowserProfileSelector.svelte` around lines 82 - 90, The form controls in BrowserProfileSelector.svelte lack accessible names; add explicit labels or aria-label attributes for the text inputs (the element with class "browser-input" handled by handleBrowserInput and the profile input referenced around lines 108-116) and for any <select> controls, and give the clear buttons (the button invoking handleClearBrowser and the profile clear button) accessible names via aria-label or aria-labelledby; ensure labels are programmatically associated (for example using <label for="..."> or matching id+aria-labelledby) so screen readers and keyboard users can identify each control.apps/finicky/src/rules/rules.go (2)
69-75: GlobalcustomPathis not thread-safe.The package-level
customPathvariable is accessed without synchronization. IfSetCustomPathandGetPathare ever called concurrently (e.g., during tests with parallel execution), this could lead to data races.Since this is documented as intended for testing and CLI flags (typically set once at startup), the risk is low, but consider using
sync.Onceor a mutex if concurrent access becomes possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/rules/rules.go` around lines 69 - 75, The package-level variable customPath is not protected against concurrent access by SetCustomPath and readers (e.g., GetPath), risking data races; protect reads and writes by introducing a sync.RWMutex (e.g., customPathMu) and use customPathMu.Lock() / Unlock() in SetCustomPath and customPathMu.RLock() / RUnlock() around any GetPath or other readers, ensuring all accesses to customPath are synchronized (alternatively use sync/atomic for pointer updates or sync.Once if the value is only set once).
129-141: Consider atomic file writes to prevent corruption.
os.WriteFilewrites directly to the target file. If the process is interrupted mid-write (crash, power loss), the file could be left in a corrupted partial state. For user configuration files, an atomic write pattern (write to temp file, then rename) is safer.🛡️ Suggested atomic write pattern
func SaveToPath(rf RulesFile, path string) error { if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { return err } data, err := json.MarshalIndent(rf, "", " ") if err != nil { return err } - return os.WriteFile(path, data, 0644) + // Write to temp file first, then atomically rename + tmpPath := path + ".tmp" + if err := os.WriteFile(tmpPath, data, 0644); err != nil { + return err + } + return os.Rename(tmpPath, path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/rules/rules.go` around lines 129 - 141, SaveToPath currently writes directly with os.WriteFile which can leave a partial/corrupt file on interruption; change it to perform an atomic write: after json.MarshalIndent(rf...), create a temp file in the same directory (use filepath.Dir(path)), write the data to that temp file, flush and sync the file (fsync) and then atomically rename the temp file to the target path (os.Rename); ensure you still call os.MkdirAll before creating the temp file and return any errors from the temp write/sync/rename steps instead of calling os.WriteFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/finicky/src/browser/detect.go`:
- Around line 34-71: The helper getAllHttpsHandlerNames() uses AppKit APIs and
must run on the main thread; update the call site in didReceiveScriptMessage:
(the WKScriptMessageHandler callback) to marshal to the main thread by wrapping
the HandleWebViewMessage(...) invocation in
dispatch_sync(dispatch_get_main_queue(), ^{ ... }) so all
NSWorkspace/NSBundle/NSFileManager work happens on the main queue, or
alternatively perform the main-thread dispatch inside getAllHttpsHandlerNames()
itself to ensure its body runs on the main thread.
In `@apps/finicky/src/config/configfiles.go`:
- Around line 365-374: The debounce timer's callback (created in
cfw.debounceTimer via time.AfterFunc) performs a blocking send to
cfw.configChangeNotify while holding no lock, which lets timer goroutines pile
up and defeats coalescing; change the callback to perform a non-blocking send
(e.g., use a select with case notify <- struct{}{} and a default to drop the
signal if the channel is full) so pending timers won't block, or alternatively
make configChangeNotify a size-1 buffered channel and still use a non-blocking
send to ensure only one coalesced notification is delivered; adjust the timer
callback that references cfw.debounceTimer, cfw.debounceMu, and
configChangeNotify accordingly.
In `@apps/finicky/src/main.go`:
- Around line 147-149: The current early-return sets vm = nil when
rf.DefaultBrowser == "" && len(rf.Rules) == 0 which incorrectly treats
option-only rules files as empty; update the condition in the VM-initialization
logic (the block referencing rf, vm, DefaultBrowser and Rules) to instead detect
whether any persisted settings exist (e.g., check fields like rf.KeepRunning,
rf.HideIcon, rf.LogRequests, rf.CheckForUpdates, etc.) and only set vm = nil
when no rules and no other persisted settings are present; apply the same fix to
the analogous conditional later in the file (the block around the other rf/ vm
check).
- Around line 151-163: After swapping in newVM (from config.NewFromScript) you
must also apply the saved logRequests setting so the logger reflects the rebuilt
config immediately; after setting vm = newVM and updating shouldKeepRunning =
vm.GetAllConfigOptions().KeepRunning, call logger.SetupFile(...) (or the
existing logger initialization routine) with the new VM's logRequests value
(e.g., vm.GetAllConfigOptions().LogRequests or
newVM.GetAllConfigOptions().LogRequests) before launching checkForUpdates() so
the logRequests toggle takes effect without a restart.
- Around line 143-165: The SaveRulesHandler replaces the global vm and other
goroutines (TestURLInternal, the main event loop via resolver.ResolveURL and
evaluateURL) concurrently read/write the same goja.Runtime; make VM access
concurrency-safe by adding a package-level sync.RWMutex (e.g., vmMu) and use
vmMu.Lock() when replacing or mutating vm and vmMu.RLock() when reading/using it
(including in resolver.ResolveURL, evaluateURL, TestURLInternal, and any
main-loop accesses); ensure nil checks and assignments of vm and
shouldKeepRunning are done under the same lock and release promptly, and add
locking around calls that execute JS on the runtime so only one goroutine
accesses goja.Runtime at a time (apply same protection to the other spots noted
in the comment).
In `@apps/finicky/src/main.m`:
- Around line 112-119: The current application:openFile: early-return discards
legitimate pre-launch file-open events; change it to enqueue or handle only true
CLI flag values: add a mutable array property (e.g., pendingOpenFiles) on the
app delegate and when !self.didFinishLaunching, if filename looks like a CLI
flag (e.g., startsWith "-" or equals known flag tokens like "--config") ignore
it, otherwise append the filename to pendingOpenFiles for processing later in
applicationDidFinishLaunching (process and clear pendingOpenFiles there); ensure
application:openFile: no longer unconditionally returns false during launch so
legitimate file-open requests are preserved.
In `@apps/finicky/src/rules/rules.go`:
- Around line 178-199: ToJSConfigScript currently interpolates the namespace
directly into the generated JS ("var %s = ..."), allowing JS injection if
namespace is untrusted; fix by either validating the namespace parameter (in
ToJSConfigScript) against a safe JS identifier regex (e.g.
^[A-Za-z_$][A-Za-z0-9_$]*$) and returning an error for invalid values, or avoid
direct interpolation entirely by using a safe bracket assignment pattern with
the JSON-marshaled namespace (e.g. assign to
window[<json-stringified-namespace>] instead of "var <namespace>") so special
characters cannot break the generated script; update the fmt.Sprintf call that
emits the var assignment accordingly and add tests for both valid and invalid
namespace inputs.
In `@apps/finicky/src/util/directories.go`:
- Around line 27-29: The current prefix check using strings.HasPrefix(path,
home) can spuriously match directories like "/Users/alice2"; update the
condition in the path-shortening logic to only treat path as inside home when it
is exactly equal to home or when it has home followed by a path separator (e.g.
check path == home || strings.HasPrefix(path, home+string(os.PathSeparator))).
Apply this change where the code references path and home in the directories.go
shortening routine so "~" substitution only happens for true home-boundary
matches.
In `@apps/finicky/src/version/version.go`:
- Around line 239-245: The FINICKY_MOCK_UPDATE handling returns URLs by directly
embedding mockVersion which can double-up a leading "v" (e.g., "vv1.2.3");
update the block in the function that reads FINICKY_MOCK_UPDATE so you normalize
the value (e.g., strings.TrimSpace then strings.TrimPrefix(mockVersion, "v") and
TrimPrefix(..., "V")) before composing DownloadUrl and ReleaseUrl while keeping
LatestVersion set to the original or normalized form as desired; modify the code
around the ReleaseInfo construction (the block that sets
HasUpdate/LatestVersion/DownloadUrl/ReleaseUrl) to use the normalized tag string
for the URL fmt.Sprintf calls.
In `@apps/finicky/src/window/window.go`:
- Around line 267-274: handleGetBrowserProfiles silently proceeds when
msg["browser"] is missing or not a string, producing an empty browserName and
confusing output; validate the input and return an error message instead: check
msg["browser"] exists and is a non-empty string at the start of
handleGetBrowserProfiles, and if invalid send an error via SendMessageToWebView
(e.g., event "browserProfilesError" or include an "error" field) rather than
calling browser.GetProfilesForBrowser; reference the function
handleGetBrowserProfiles and the calls to browser.GetProfilesForBrowser and
SendMessageToWebView when making the change.
- Around line 223-260: The handler handleSaveRules currently logs errors from
rules.Save(rf) but never notifies the UI; modify the error branch after
rules.Save(rf) to send a failure message back to the webview (using
SendMessageToWebView) containing a status/error string (e.g.,
{"status":"error","error": err.Error()}) so the UI can show feedback, and keep
the existing success path (sending rulesResponse and invoking SaveRulesHandler)
unchanged; reference the rules.Save call, the SendMessageToWebView function, and
the rules.GetPath/SaveRulesHandler flow when implementing the error response.
In `@packages/finicky-ui/src/App.svelte`:
- Around line 104-107: The startup sendMessage calls may run before the webkit
message handler is ready; wrap the calls to window.finicky.sendMessage({ type:
"getRules" }) and window.finicky.sendMessage({ type: "getInstalledBrowsers" })
in a Svelte onMount handler (import onMount from "svelte") and/or add a
readiness check (e.g., guard that window.webkit?.messageHandlers?.finicky or
window.finicky exists) before sending so messages are deferred until the bridge
is available; update App.svelte to perform the sendMessage calls inside onMount
with the readiness check to avoid silent failures.
- Around line 153-160: Replace the Svelte 4 event syntax on native buttons with
standard DOM attributes for Svelte 5: change the two occurrences using on:click
that call showPathToast (the button rendering basename(config.configPath) and
the one rendering basename(rulesFile.path)) to use onclick instead, preserving
the same inline arrow function arguments (basename(...), the label
"Configuration loaded from", and the full path argument) and keep the
conditional blocks and className intact.
In `@packages/finicky-ui/src/components/PageContainer.svelte`:
- Around line 21-22: The component currently injects raw HTML via the
description prop using {`@html` description}; change the public API to make this a
consciously trusted sink by introducing a new prop name or slot (e.g.,
descriptionHtml prop or a named <slot name="description">) and stop calling
{`@html` description}. Update PageContainer.svelte to render plain text for the
existing description prop (e.g., {description}) and only use {`@html`
descriptionHtml} (or render the named slot) so the intent is explicit, and
update callers like Rules.svelte to pass hardcoded HTML into the new
descriptionHtml prop or into the named slot instead of the original description
prop.
In `@packages/finicky-ui/src/pages/Rules.svelte`:
- Around line 46-61: The debounce timer set by scheduleSave() (saveTimer) isn't
cleared on component teardown; add a Svelte onDestroy handler that calls
clearTimeout(saveTimer) (and clears or nulls saveTimer and pendingSave if
desired) to prevent the timeout from firing after the component is unmounted;
locate scheduleSave, saveTimer, and SAVE_DEBOUNCE in the file and register
onDestroy to perform the cleanup.
- Around line 127-131: The $effect block that calls window.finicky.sendMessage({
type: "getRules" }) and window.finicky.sendMessage({ type:
"getInstalledBrowsers" }) is being used for mount-only behavior but will re-run
on reactive updates; replace it with Svelte's onMount (import onMount from
"svelte") and move those sendMessage calls into an onMount callback (or
alternatively wrap them in untrack) so the messages are sent only once on
component mount; update the block that currently uses $effect to call onMount(()
=> { ... }) referencing the same window.finicky.sendMessage calls.
In `@packages/finicky-ui/src/pages/StartPage.svelte`:
- Around line 38-51: The reactive sync block currently assigns
defaultBrowserIsCustom and therefore depends on installedBrowsers, causing
backend updates to rerun the block and overwrite local edits; remove the
defaultBrowserIsCustom assignment from the big $: block (the block that sets
keepRunning, hideIcon, logRequests, checkForUpdates, defaultBrowser,
defaultProfile) and add a separate reactive statement like the existing
defaultProfileIsCustom one that computes defaultBrowserIsCustom = defaultBrowser
!== "" && !installedBrowsers.includes(defaultBrowser), so only that derived flag
reacts to installedBrowsers while defaultBrowser/defaultProfile stay stable
(references: defaultBrowserIsCustom, installedBrowsers, defaultProfileIsCustom,
profilesByBrowser, defaultBrowser, defaultProfile).
In `@scripts/dev.sh`:
- Around line 48-80: Default the scenario selector to "4" (both) before the case
block so an empty $1 runs scenario 4; modify the code around the existing case
"$1" in block (and the scenario labels 0|normal .. 4|both) to use a
pre-initialized value instead of raw $1 — e.g., if $1 is empty set a local
variable or reset positional parameters so the case tests a non-empty selector
(default "4") and ensure existing flags like --mock-update remain honored;
update references from case "$1" in to case "$SCENARIO" in (or set -- 4 ...) so
./scripts/dev.sh without args executes the "4|both" branch.
---
Outside diff comments:
In `@apps/finicky/src/main.go`:
- Around line 257-287: The code always enqueues a non-nil pointer to a
zero-value resolver.OpenerInfo even when Launch Services provided no opener;
change the local opener to be a pointer (var opener *resolver.OpenerInfo) and
only allocate & populate it inside the existing if (name != nil && bundleId !=
nil && path != nil) block (set WindowTitle on that allocated struct if
provided). Then pass opener (which will be nil when no opener was provided) into
URLInfo so JS can distinguish “no opener” from an empty opener.
In `@packages/finicky-ui/src/pages/TestUrl.svelte`:
- Around line 61-71: The subscription to testUrlResult is never cleaned up;
capture the unsubscribe function returned by testUrlResult.subscribe(...) (e.g.,
const unsubscribe = testUrlResult.subscribe(...)) and register cleanup in
Svelte's onDestroy by calling unsubscribe() (and also clearTimeout(loadingTimer)
if still pending) to avoid memory leaks; modify the block around
testUrlResult.subscribe, referencing testUrlResult, loadingTimer, and loading,
and add an onDestroy import/use to call unsubscribe() and any timeout cleanup.
---
Nitpick comments:
In `@apps/finicky/src/rules/rules.go`:
- Around line 69-75: The package-level variable customPath is not protected
against concurrent access by SetCustomPath and readers (e.g., GetPath), risking
data races; protect reads and writes by introducing a sync.RWMutex (e.g.,
customPathMu) and use customPathMu.Lock() / Unlock() in SetCustomPath and
customPathMu.RLock() / RUnlock() around any GetPath or other readers, ensuring
all accesses to customPath are synchronized (alternatively use sync/atomic for
pointer updates or sync.Once if the value is only set once).
- Around line 129-141: SaveToPath currently writes directly with os.WriteFile
which can leave a partial/corrupt file on interruption; change it to perform an
atomic write: after json.MarshalIndent(rf...), create a temp file in the same
directory (use filepath.Dir(path)), write the data to that temp file, flush and
sync the file (fsync) and then atomically rename the temp file to the target
path (os.Rename); ensure you still call os.MkdirAll before creating the temp
file and return any errors from the temp write/sync/rename steps instead of
calling os.WriteFile.
In `@packages/finicky-ui/src/app.css`:
- Around line 1-44: The file contains two separate :root blocks—one with base
font and color-scheme properties and another with CSS custom properties (e.g.,
--bg-primary, --nav-text, --accent-color); merge them into a single :root block
by moving the base declarations (font-family, line-height, font-weight,
color-scheme, color, background-color, font-synthesis, text-rendering,
-webkit-font-smoothing, -moz-osx-font-smoothing) together with all custom
property definitions (the --* variables) and remove the duplicate :root to
improve maintainability and avoid fragmentation.
In `@packages/finicky-ui/src/components/BrowserProfileSelector.svelte`:
- Around line 204-207: The .clear-btn:hover rule in
BrowserProfileSelector.svelte uses a hardcoded color `#f44336`; change it to use
the component/design-system CSS variable (e.g., replace with var(--color-danger)
or the project's equivalent like var(--fg-danger)), and include a sensible
fallback (e.g., var(--color-danger, `#f44336`)) to preserve behavior if the
variable is missing; update the .clear-btn:hover selector to reference that CSS
custom property so the component follows the existing variable pattern.
- Around line 82-90: The form controls in BrowserProfileSelector.svelte lack
accessible names; add explicit labels or aria-label attributes for the text
inputs (the element with class "browser-input" handled by handleBrowserInput and
the profile input referenced around lines 108-116) and for any <select>
controls, and give the clear buttons (the button invoking handleClearBrowser and
the profile clear button) accessible names via aria-label or aria-labelledby;
ensure labels are programmatically associated (for example using <label
for="..."> or matching id+aria-labelledby) so screen readers and keyboard users
can identify each control.
In `@packages/finicky-ui/src/components/icons/Lock.svelte`:
- Around line 1-15: The Lock.svelte SVG is decorative and should be hidden from
assistive tech; update the root <svg> element (the one with class "lucide
lucide-lock" in Lock.svelte) to include aria-hidden="true" and optionally
focusable="false" so screen readers skip it; if this icon ever becomes
meaningful, replace those attributes with an accessible title/role instead.
In `@packages/finicky-ui/src/components/icons/Rules.svelte`:
- Around line 1-13: The Rules.svelte SVG is missing accessibility attributes for
decorative icons; update the <svg> element in Rules.svelte (the SVG with class
"lucide lucide-list-filter") to include aria-hidden="true" and optionally
focusable="false" so it is ignored by assistive technologies when used
decoratively, keeping no change to visual rendering or other attributes.
In `@packages/finicky-ui/src/components/OptionRow.svelte`:
- Around line 120-143: Replace the three hard-coded color values used in the
toggle CSS (.toggle-slider background-color `#666`, .toggle-slider:before
background-color `#ddd`, and checked .toggle-slider:before background-color
`#111111`) with theme-aware CSS variables (e.g., use existing --toggle-active
for the checked track and add/choose variables such as --toggle-bg and
--toggle-thumb for the track and thumb defaults) so the rules in .toggle-slider,
.toggle-slider:before, and .toggle input:checked + .toggle-slider:before derive
colors from variables instead of fixed hex values; ensure sensible fallback
values are provided in the root or component :root so themes without those
variables still render predictably.
- Around line 27-28: The current shorthand {onchange} on the input in
OptionRow.svelte can mismatch Svelte 5 DOM event typing: decide whether the
exported prop onchange should receive the DOM Event or be a no-arg callback; if
it should receive the Event, replace the shorthand with an explicit event
binding on:change={onchange} and update the prop type to (e: Event) => void,
otherwise wrap the call to drop the event payload (on:change={() =>
onchange?.()}) or change the prop name to something like onToggle to make the
no-arg intent explicit; ensure this change is applied alongside the existing
bind:checked and locked usage so consumers get the expected signature.
In `@packages/finicky-ui/src/pages/Rules.svelte`:
- Around line 176-183: The draggable rule rows (element with class "rule-row",
using dragIndex and handlers onDragStart/onDragOver/onDragEnd) lack keyboard
accessibility; add keyboard support by making each rule-row focusable
(tabindex="0"), manage an "active/dragging" state via keyboard (e.g., onKeyDown
onRuleKeyDown) that starts a keyboard-reorder mode on a modifier key (Space or
Enter) and then moves the item with ArrowUp/ArrowDown to call the same reorder
logic used by onDragStart/onDragOver (or a shared moveRule(indexFrom, indexTo)
helper), update ARIA attributes (aria-grabbed, role="option" or "listitem",
aria-posinset/aria-setsize) while reordering, and announce changes via a small
ARIA live region when an item is picked up, moved, or dropped so keyboard users
receive feedback; wire cleanup to onKeyUp/Escape to cancel and ensure onDragEnd
behavior is mirrored.
- Around line 162-167: The description prop currently embeds inline HTML with a
style attribute in the PageContainer usage (PageContainer, description,
isJSConfig) which bypasses sanitization; replace that inline HTML by creating a
small presentational Svelte component (e.g., JsConfigBadge or
DescriptionWithBadge) or add a CSS class (e.g., .accent-badge) and use a plain
text/element slot on PageContainer instead of passing raw HTML in the
description prop; update the PageContainer API (or its consumer) to accept a
description slot or component and revise the Rules.svelte usage to render
<JsConfigBadge/> or a <span class="accent-badge">JS config is active</span> so
styling is done via CSS and no inline style HTML is injected.
In `@packages/finicky-ui/src/pages/TestUrl.svelte`:
- Around line 216-218: Replace the hard-coded rgba colors in the .hint-message
style with the theme variable so the component uses the shared color token;
specifically, update the background and border that currently use rgba(245, 188,
28, ...) to use var(--log-warning) with appropriate alpha (e.g., via color-mix()
or by using an RGB CSS variable and applying alpha) in the .hint-message rule
inside TestUrl.svelte so the hint matches the app theme.
In `@packages/finicky-ui/src/types.ts`:
- Around line 7-18: RulesFile defines an inline options shape that duplicates
ConfigInfo.options; change both RulesFile.options and ConfigInfo.options to
reuse the existing ConfigOptions type by declaring them as
Partial<ConfigOptions> (or ConfigOptions | undefined if you prefer full type) to
avoid duplicated field definitions—update the interface references to use
Partial<ConfigOptions> and remove the duplicated inline object shape in
RulesFile and the corresponding duplicated definition in ConfigInfo so the
shared ConfigOptions type is the single source of truth.
🪄 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: 2f612b7e-b0cc-479b-97d3-91cd15fa65fd
📒 Files selected for processing (35)
apps/finicky/src/browser/browsers.jsonapps/finicky/src/browser/detect.goapps/finicky/src/browser/launcher.goapps/finicky/src/config/configfiles.goapps/finicky/src/config/vm.goapps/finicky/src/main.goapps/finicky/src/main.happs/finicky/src/main.mapps/finicky/src/resolver/resolver.goapps/finicky/src/resolver/resolver_test.goapps/finicky/src/rules/rules.goapps/finicky/src/rules/rules_test.goapps/finicky/src/util/directories.goapps/finicky/src/version/version.goapps/finicky/src/window/window.gopackages/finicky-ui/src/App.sveltepackages/finicky-ui/src/app.csspackages/finicky-ui/src/components/BrowserProfileSelector.sveltepackages/finicky-ui/src/components/OptionRow.sveltepackages/finicky-ui/src/components/PageContainer.sveltepackages/finicky-ui/src/components/TabBar.sveltepackages/finicky-ui/src/components/icons/Lock.sveltepackages/finicky-ui/src/components/icons/Rules.sveltepackages/finicky-ui/src/lib/testUrlStore.tspackages/finicky-ui/src/pages/About.sveltepackages/finicky-ui/src/pages/LogViewer.sveltepackages/finicky-ui/src/pages/Rules.sveltepackages/finicky-ui/src/pages/StartPage.sveltepackages/finicky-ui/src/pages/TestUrl.sveltepackages/finicky-ui/src/types.tspackages/finicky-ui/src/utils/text.tsscripts/dev.shscripts/test.shtestdata/config.jstestdata/rules.json
| static char **getAllHttpsHandlerNames(int *count) { | ||
| @autoreleasepool { | ||
| NSURL *url = [NSURL URLWithString:@"https://example.com"]; | ||
| NSArray<NSURL *> *appURLs = [[NSWorkspace sharedWorkspace] URLsForApplicationsToOpenURL:url]; | ||
| if (!appURLs || appURLs.count == 0) { | ||
| *count = 0; | ||
| return NULL; | ||
| } | ||
|
|
||
| NSMutableSet<NSString *> *seen = [NSMutableSet set]; | ||
| NSMutableArray<NSString *> *names = [NSMutableArray array]; | ||
| NSSet *excludedBundleIDs = [NSSet setWithObjects: | ||
| @"se.johnste.finicky", | ||
| @"net.kassett.finicky", | ||
| nil]; | ||
|
|
||
| for (NSURL *appURL in appURLs) { | ||
| NSBundle *bundle = [NSBundle bundleWithURL:appURL]; | ||
| if ([excludedBundleIDs containsObject:bundle.bundleIdentifier]) continue; | ||
| if (!isLikelyBrowser(appURL)) continue; | ||
|
|
||
| NSString *name = [[NSFileManager defaultManager] displayNameAtPath:[appURL path]]; | ||
| if ([name hasSuffix:@".app"]) { | ||
| name = [name substringToIndex:[name length] - 4]; | ||
| } | ||
| if (![seen containsObject:name]) { | ||
| [seen addObject:name]; | ||
| [names addObject:name]; | ||
| } | ||
| } | ||
|
|
||
| *count = (int)names.count; | ||
| char **result = (char **)malloc(names.count * sizeof(char *)); | ||
| for (NSInteger i = 0; i < (NSInteger)names.count; i++) { | ||
| result[i] = strdup([names[i] UTF8String]); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" | head -20Repository: johnste/finicky
Length of output: 794
🏁 Script executed:
fd -t f "window\.go" --type fRepository: johnste/finicky
Length of output: 92
🏁 Script executed:
fd -t f "detect\." --type fRepository: johnste/finicky
Length of output: 93
🏁 Script executed:
git ls-files | grep -E "(browser|window)" | head -30Repository: johnste/finicky
Length of output: 681
🏁 Script executed:
file apps/finicky/src/browser/detect.goRepository: johnste/finicky
Length of output: 103
🏁 Script executed:
cat -n apps/finicky/src/browser/detect.goRepository: johnste/finicky
Length of output: 3794
🏁 Script executed:
grep -n "GetInstalledBrowsers" apps/finicky/src/browser/*.goRepository: johnste/finicky
Length of output: 249
🏁 Script executed:
sed -n '140,180p' apps/finicky/src/window/window.goRepository: johnste/finicky
Length of output: 1093
🏁 Script executed:
sed -n '260,270p' apps/finicky/src/window/window.goRepository: johnste/finicky
Length of output: 423
🏁 Script executed:
rg "HandleWebViewMessage" --type go --type m --type h -B 5 -A 5Repository: johnste/finicky
Length of output: 84
🏁 Script executed:
grep -n "window.h\|window.m\|bridge\|WebView" apps/finicky/src/window/window.m | head -40Repository: johnste/finicky
Length of output: 1446
🏁 Script executed:
sed -n '1,100p' apps/finicky/src/window/window.mRepository: johnste/finicky
Length of output: 3711
🏁 Script executed:
rg "GetInstalledBrowsers\|dispatch.*main" apps/finicky/src/Repository: johnste/finicky
Length of output: 41
🏁 Script executed:
grep -n "didReceiveScriptMessage\|WKScriptMessageHandler" apps/finicky/src/window.mRepository: johnste/finicky
Length of output: 117
🏁 Script executed:
sed -n '165,195p' apps/finicky/src/window.mRepository: johnste/finicky
Length of output: 127
🏁 Script executed:
rg "GetInstalledBrowsers\|dispatch.*main" apps/finicky/src/Repository: johnste/finicky
Length of output: 41
🏁 Script executed:
grep -n "didReceiveScriptMessage\|WKScriptMessageHandler" apps/finicky/src/window/window.mRepository: johnste/finicky
Length of output: 121
🏁 Script executed:
sed -n '165,195p' apps/finicky/src/window/window.mRepository: johnste/finicky
Length of output: 1371
🌐 Web query:
WKScriptMessageHandler didReceiveScriptMessage main thread macOS documentation
💡 Result:
Apple’s WKScriptMessageHandler callback (userContentController(_:didReceive:) / Obj‑C userContentController:didReceiveScriptMessage:) is documented as being invoked when a script message is received, but the documentation does not promise it runs on the main thread. [1]
In practice, WebKit may call it off the main thread depending on how your handler is structured; for example, developers have observed it being on the main thread when the handler is a UIResponder subclass (iOS) but on “random threads” when it’s a plain NSObject, and the recommended fix is to force main-actor/main-thread handling (e.g., @MainActor or DispatchQueue.main.async). [2]
What to do (macOS included):
- If you touch UI/AppKit from the callback, hop to the main thread/main actor yourself.
- Swift (safe default):
(Or wrap UI work in
extension MyHandler: WKScriptMessageHandler { `@MainActor` func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) { // UI-safe work here } }
DispatchQueue.main.async { ... }.) [2]
Sources
- Apple-doc mirror showing the official description/signature (no thread guarantee stated). [1]
- Observed non-main-thread delivery +
@MainActorworkaround discussion. [2]
🏁 Script executed:
rg "GetInstalledBrowsers" --type go -B 3 -A 3Repository: johnste/finicky
Length of output: 1491
🏁 Script executed:
rg "NSWorkspace\|NSBundle\|NSFileManager" apps/finicky/src/browser/detect.goRepository: johnste/finicky
Length of output: 41
Run the browser enumeration on the main thread.
The Objective-C helper getAllHttpsHandlerNames() touches NSWorkspace, NSBundle, and NSFileManager—all AppKit APIs that require main-thread access. The call path from window/window.m line 173 (didReceiveScriptMessage:) invokes this code without marshalling to the main thread first. WKScriptMessageHandler callbacks are not guaranteed to run on the main thread, which makes this undefined behavior. This can cause crashes or intermittent browser detection failures.
Add dispatch_sync(dispatch_get_main_queue(), ^{ ... }) around the HandleWebViewMessage() call in the didReceiveScriptMessage: handler, or move the AppKit work onto the main thread inside the Objective-C helper itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/finicky/src/browser/detect.go` around lines 34 - 71, The helper
getAllHttpsHandlerNames() uses AppKit APIs and must run on the main thread;
update the call site in didReceiveScriptMessage: (the WKScriptMessageHandler
callback) to marshal to the main thread by wrapping the
HandleWebViewMessage(...) invocation in dispatch_sync(dispatch_get_main_queue(),
^{ ... }) so all NSWorkspace/NSBundle/NSFileManager work happens on the main
queue, or alternatively perform the main-thread dispatch inside
getAllHttpsHandlerNames() itself to ensure its body runs on the main thread.
| // Debounce: reset the timer so only the last event in a burst fires. | ||
| cfw.debounceMu.Lock() | ||
| if cfw.debounceTimer != nil { | ||
| cfw.debounceTimer.Stop() | ||
| } | ||
| notify := cfw.configChangeNotify | ||
| cfw.debounceTimer = time.AfterFunc(500*time.Millisecond, func() { | ||
| notify <- struct{}{} | ||
| }) | ||
| cfw.debounceMu.Unlock() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unbuffered channel producer/consumer relationship and debounce send points.
rg -nP 'configChange\s*:=\s*make\(chan struct{}\)' apps/finicky/src/main.go
rg -nP 'time\.AfterFunc|configChangeNotify <- struct{}\{\}|notify <- struct{}\{\}' apps/finicky/src/config/configfiles.goRepository: johnste/finicky
Length of output: 240
🏁 Script executed:
# Find all usages of configChangeNotify to understand initialization and consumption
rg -n 'configChangeNotify' apps/finicky/src/config/configfiles.go apps/finicky/src/main.go -B 2 -A 2Repository: johnste/finicky
Length of output: 1970
🏁 Script executed:
# Check the main loop to see how it consumes the configChange channel
rg -n 'select' apps/finicky/src/main.go -B 5 -A 5Repository: johnste/finicky
Length of output: 265
🏁 Script executed:
# Verify if there are other goroutines that might block the main loop
rg -n 'go func\(\)' apps/finicky/src/main.go -A 3 | head -50Repository: johnste/finicky
Length of output: 322
🏁 Script executed:
# Get the full select statement in main.go to see all channel cases
sed -n '175,200p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 790
🏁 Script executed:
# Find where configChange is created and check how many receivers are on it
sed -n '110,130p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 631
🏁 Script executed:
# Check if there are multiple sends to configChange/configChangeNotify
rg -n '<-.*configChange|configChange.*<-' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 86
🏁 Script executed:
# Get the complete select statement in the URL listener goroutine
sed -n '180,225p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 1456
🏁 Script executed:
# Check if there's a context deadline or timeout mechanism that could prevent blocking
rg -n 'context\.|timeout|Context' apps/finicky/src/main.go | head -20Repository: johnste/finicky
Length of output: 306
🏁 Script executed:
# Check ResolveURL implementation to understand if it's a blocking operation
rg -n 'func.*ResolveURL' apps/finicky/src/ -A 20 | head -40Repository: johnste/finicky
Length of output: 2738
🏁 Script executed:
# Check LaunchBrowser to see if it blocks
rg -n 'func.*LaunchBrowser' apps/finicky/src/ -A 10 | head -30Repository: johnste/finicky
Length of output: 831
🏁 Script executed:
# Verify the debounce logic: are there multiple debounceTimer.Stop() calls?
sed -n '355,380p' apps/finicky/src/config/configfiles.goRepository: johnste/finicky
Length of output: 812
Debounce callback can block and defeat event coalescing.
The timer goroutine sends to configChangeNotify on line 372. With an unbuffered channel, this send blocks if the main loop is busy processing a URL (lines 182–199 in main.go call ResolveURL and LaunchBrowser, which are synchronous operations). When file changes occur in rapid succession, multiple blocked timer goroutines accumulate waiting to send, defeating the debounce's purpose of producing a single coalesced notification.
💡 Safer coalescing pattern
notify := cfw.configChangeNotify
cfw.debounceTimer = time.AfterFunc(500*time.Millisecond, func() {
- notify <- struct{}{}
+ select {
+ case notify <- struct{}{}:
+ default:
+ // Drop if a notification is already pending/being handled.
+ }
})📝 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.
| // Debounce: reset the timer so only the last event in a burst fires. | |
| cfw.debounceMu.Lock() | |
| if cfw.debounceTimer != nil { | |
| cfw.debounceTimer.Stop() | |
| } | |
| notify := cfw.configChangeNotify | |
| cfw.debounceTimer = time.AfterFunc(500*time.Millisecond, func() { | |
| notify <- struct{}{} | |
| }) | |
| cfw.debounceMu.Unlock() | |
| // Debounce: reset the timer so only the last event in a burst fires. | |
| cfw.debounceMu.Lock() | |
| if cfw.debounceTimer != nil { | |
| cfw.debounceTimer.Stop() | |
| } | |
| notify := cfw.configChangeNotify | |
| cfw.debounceTimer = time.AfterFunc(500*time.Millisecond, func() { | |
| select { | |
| case notify <- struct{}{}: | |
| default: | |
| // Drop if a notification is already pending/being handled. | |
| } | |
| }) | |
| cfw.debounceMu.Unlock() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/finicky/src/config/configfiles.go` around lines 365 - 374, The debounce
timer's callback (created in cfw.debounceTimer via time.AfterFunc) performs a
blocking send to cfw.configChangeNotify while holding no lock, which lets timer
goroutines pile up and defeats coalescing; change the callback to perform a
non-blocking send (e.g., use a select with case notify <- struct{}{} and a
default to drop the signal if the channel is full) so pending timers won't
block, or alternatively make configChangeNotify a size-1 buffered channel and
still use a non-blocking send to ensure only one coalesced notification is
delivered; adjust the timer callback that references cfw.debounceTimer,
cfw.debounceMu, and configChangeNotify accordingly.
| window.SaveRulesHandler = func(rf rules.RulesFile) { | ||
| slog.Debug("Rules updated", "count", len(rf.Rules)) | ||
| resolver.SetCachedRules(rf) | ||
| if vm == nil || !vm.IsJSConfig() { | ||
| if rf.DefaultBrowser == "" && len(rf.Rules) == 0 { | ||
| vm = nil | ||
| return | ||
| } | ||
| script, err := rules.ToJSConfigScript(rf, namespace) | ||
| if err != nil { | ||
| slog.Error("Failed to generate config from rules", "error", err) | ||
| return | ||
| } | ||
| newVM, err := config.NewFromScript(finickyConfigAPIJS, namespace, script) | ||
| if err != nil { | ||
| slog.Error("Failed to rebuild VM from rules", "error", err) | ||
| return | ||
| } | ||
| vm = newVM | ||
| shouldKeepRunning = vm.GetAllConfigOptions().KeepRunning | ||
| go checkForUpdates() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file structure and read the main.go file to understand the context
git ls-files apps/finicky/src/main.go | head -5Repository: johnste/finicky
Length of output: 83
🏁 Script executed:
# Get file info and line count
wc -l apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 87
🏁 Script executed:
# Read the SaveRulesHandler section (lines 143-165) and surrounding context
sed -n '135,170p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 1161
🏁 Script executed:
# Check lines 189-190 and context
sed -n '185,195p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 409
🏁 Script executed:
# Check lines 299-300 and context
sed -n '295,305p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 389
🏁 Script executed:
# Search for resolver.ResolveURL to understand what it does
rg "ResolveURL" apps/finicky/src/ -A 5 -B 2Repository: johnste/finicky
Length of output: 14133
🏁 Script executed:
# Search for goja.Runtime usage and any mutex definitions
rg "goja\.Runtime|sync\.Mutex|sync\.RWMutex" apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 85
🏁 Script executed:
# Look at the vm definition and how it's used globally
rg "var vm|vm =" apps/finicky/src/main.go | head -20Repository: johnste/finicky
Length of output: 140
🏁 Script executed:
# Read the evaluateURL function to see how vm/runtime is used
sed -n '1,150p' apps/finicky/src/resolver/resolver.go | tail -100Repository: johnste/finicky
Length of output: 3213
🏁 Script executed:
# Search for evaluateURL full implementation
rg "func evaluateURL" apps/finicky/src/resolver/resolver.go -A 30Repository: johnste/finicky
Length of output: 1065
🏁 Script executed:
# Check the full evaluateURL and runtime.Set calls
sed -n '1,200p' apps/finicky/src/resolver/resolver.goRepository: johnste/finicky
Length of output: 4313
🏁 Script executed:
# Look for any synchronization primitives in main.go
rg "sync\.|mutex|lock|RWMutex|Mutex" apps/finicky/src/main.go -iRepository: johnste/finicky
Length of output: 82
🏁 Script executed:
# Check if there are goroutines spawned that access vm
rg "go " apps/finicky/src/main.go | head -20Repository: johnste/finicky
Length of output: 342
🏁 Script executed:
# Look at line 138-180 to see the full context around SaveRulesHandler
sed -n '138,180p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 1264
🏁 Script executed:
# Check if goja.Runtime is documented as thread-safe and look at lock usage
rg "LockOSThread|Lock|Unlock" apps/finicky/src/config/config.go -B 2 -A 2Repository: johnste/finicky
Length of output: 132
🏁 Script executed:
# Look for the config.VM structure and Runtime() method
rg "type VM struct|func.*Runtime\(\)" apps/finicky/src/config/config.go -A 5Repository: johnste/finicky
Length of output: 132
🏁 Script executed:
# Check how many goroutines actually access resolver.ResolveURL
rg "resolver\.ResolveURL|TestURLInternal" apps/finicky/src/main.go -B 2 -A 2Repository: johnste/finicky
Length of output: 657
🏁 Script executed:
# Look at the main event loop to understand threading model
sed -n '165,250p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 2320
🏁 Script executed:
# Check if there's any synchronization in the main event loop where SaveRulesHandler might be called
rg "SaveRulesHandler|window\." apps/finicky/src/main.go | head -20Repository: johnste/finicky
Length of output: 573
🏁 Script executed:
# Check the goja import and documentation
grep -n "goja" apps/finicky/src/config/config.go | head -10Repository: johnste/finicky
Length of output: 125
🏁 Script executed:
# Find config package files
fd "config" apps/finicky/src --type f -name "*.go"Repository: johnste/finicky
Length of output: 289
🏁 Script executed:
# Look for VM type definition
rg "type VM struct" apps/finicky/src -A 10Repository: johnste/finicky
Length of output: 629
🏁 Script executed:
# Check for any mutex or synchronization in the config package
rg "Mutex|Lock|sync\." apps/finicky/src/config -A 2 -B 2Repository: johnste/finicky
Length of output: 730
🏁 Script executed:
# Find the config.VM Runtime method
rg "func.*\(\s*vm\s*\*VM\s*\).*Runtime" apps/finicky/src -A 5Repository: johnste/finicky
Length of output: 209
🏁 Script executed:
# Check if goja.Runtime is thread-safe by looking at documentation or comments
web_search
goja Runtime thread safety concurrencyRepository: johnste/finicky
Length of output: 152
🌐 Web query:
goja Runtime thread safety and goroutine safety
💡 Result:
goja.Runtimeis not goroutine-safe / thread-safe. A single*goja.Runtime“can only be used by a single goroutine at a time.” If you need concurrency, create multiple runtimes, or serialize all access through one owning goroutine (e.g., an event-loop pattern). [1]- You generally can’t share JS object values between runtimes. The README notes it’s “not possible to pass object values between runtimes.” (Primitives are OK.) [1]
- Promise resolve/reject funcs aren’t goroutine-safe either.
Runtime.NewPromise()returnsresolve/rejectfunctions that “must not be called in parallel with VM running”; the docs show scheduling back onto the VM/event-loop goroutine. [2] - If you do share host objects across runtimes, the host side must be goroutine-safe.
NewSharedDynamicObject/NewSharedDynamicArraycan create objects sharable across multiple runtimes, but the providedDynamicObject/DynamicArray“must be goroutine-safe.” [3]
Sources:
[1] goja README / FAQ on pkg.go.dev (goroutine safety + cross-runtime value passing) (pkg.go.dev)
[2] Runtime.NewPromise docs (resolve/reject not goroutine-safe; schedule on loop) (pkg.go.dev)
[3] NewSharedDynamicObject / NewSharedDynamicArray docs (must be goroutine-safe) (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/github.com/dop251/goja
- 2: https://pkg.go.dev/github.com/dop251/goja
- 3: https://pkg.go.dev/github.com/dop251/goja
🏁 Script executed:
# Let me check the entire VM structure and its methods
sed -n '1,100p' apps/finicky/src/config/vm.goRepository: johnste/finicky
Length of output: 2824
🏁 Script executed:
# Check all calls to vm from the main event loop
sed -n '165,300p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 3757
🏁 Script executed:
# Look for synchronization when vm is accessed
rg "vm\." apps/finicky/src/main.go | head -30Repository: johnste/finicky
Length of output: 405
🏁 Script executed:
# Check how SaveRulesHandler is called and from what context
rg "SaveRulesHandler" apps/finicky/src -B 5 -A 5Repository: johnste/finicky
Length of output: 2075
🏁 Script executed:
# Look at the event loop more carefully - understand all places where vm is accessed
sed -n '180,320p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 4023
🏁 Script executed:
# Verify TestURLInternal context - is it indeed spawned in a goroutine?
sed -n '135,145p' apps/finicky/src/main.goRepository: johnste/finicky
Length of output: 491
🌐 Web query:
goja Runtime state isolation multiple evaluations concurrent
💡 Result:
-
State isolation: A
goja.Runtimeis a full JS VM instance with its own global object/heap. If you run multipleRunString/RunScript/eval()calls on the sameRuntime, they share globals and mutated state across evaluations. For isolation between evaluations, create a newgoja.Runtimeper isolated context (or explicitly reset/recreate the global environment yourself). [1] -
Concurrency:
goja.Runtimeis not goroutine-safe. A single runtime instance may only be used by one goroutine at a time; you must not evaluate concurrently on the same runtime. To run scripts concurrently, use one runtime per goroutine, or funnel all access to a runtime through a single “owner” goroutine / event-loop style scheduler. [1][2] -
Common pattern for concurrent hosts: Implement an event loop (or use one like
goja_nodejs) and only touch the runtime from the loop’s goroutine; other goroutines post tasks into the loop. [1][2][3]
Sources: [1] goja README (“Is it goroutine-safe?”) (github.com), [2] pkg.go.dev docs noting Runtime methods are not safe for concurrent use and recommending an event loop (pkg.go.dev), [3] example repo emphasizing all VM interactions must happen on the owning goroutine (github.com)
Citations:
- 1: https://github.com/dop251/goja?utm_source=openai
- 2: https://pkg.go.dev/github.com/dop251/goja?utm_source=openai
- 3: https://github.com/go-go-golems/go-go-goja?utm_source=openai
🏁 Script executed:
# Final verification: check if there's any channel or synchronization mechanism that gates SaveRulesHandler or TestURLInternal
rg "SaveRulesHandler|TestURLInternal" apps/finicky/src/window -B 10 -A 2Repository: johnste/finicky
Length of output: 1565
Protect shared goja.Runtime access with synchronization.
SaveRulesHandler replaces the global vm while TestURLInternal (spawned in its own goroutine) and the main event loop both read and mutate that same instance via resolver.ResolveURL. goja.Runtime is not goroutine-safe and can only be used by a single goroutine at a time. Each resolver.ResolveURL call invokes evaluateURL, which writes per-request globals like url, opener, and _jsonHandlers into the runtime, then executes JS that reads them. Concurrent access across the UI-triggered SaveRulesHandler, the spawned test-URL goroutine, and the main event loop will cause race conditions and state bleed. Guard all VM reads/writes with a mutex, or funnel all VM access through a single owning goroutine using channels.
Also applies to: lines 189–190, 301–302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/finicky/src/main.go` around lines 143 - 165, The SaveRulesHandler
replaces the global vm and other goroutines (TestURLInternal, the main event
loop via resolver.ResolveURL and evaluateURL) concurrently read/write the same
goja.Runtime; make VM access concurrency-safe by adding a package-level
sync.RWMutex (e.g., vmMu) and use vmMu.Lock() when replacing or mutating vm and
vmMu.RLock() when reading/using it (including in resolver.ResolveURL,
evaluateURL, TestURLInternal, and any main-loop accesses); ensure nil checks and
assignments of vm and shouldKeepRunning are done under the same lock and release
promptly, and add locking around calls that execute JS on the runtime so only
one goroutine accesses goja.Runtime at a time (apply same protection to the
other spots noted in the comment).
apps/finicky/src/main.go
Outdated
| if rf.DefaultBrowser == "" && len(rf.Rules) == 0 { | ||
| vm = nil | ||
| return |
There was a problem hiding this comment.
Don’t treat option-only rules.json files as empty.
These checks only look at DefaultBrowser and Rules. A saved rules file that only contains UI-editable settings like keepRunning, hideIcon, logRequests, or checkForUpdates will therefore produce vm = nil, so the new UI settings never take effect. Build a VM whenever any persisted rules-file setting is present.
Also applies to: 441-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/finicky/src/main.go` around lines 147 - 149, The current early-return
sets vm = nil when rf.DefaultBrowser == "" && len(rf.Rules) == 0 which
incorrectly treats option-only rules files as empty; update the condition in the
VM-initialization logic (the block referencing rf, vm, DefaultBrowser and Rules)
to instead detect whether any persisted settings exist (e.g., check fields like
rf.KeepRunning, rf.HideIcon, rf.LogRequests, rf.CheckForUpdates, etc.) and only
set vm = nil when no rules and no other persisted settings are present; apply
the same fix to the analogous conditional later in the file (the block around
the other rf/ vm check).
| script, err := rules.ToJSConfigScript(rf, namespace) | ||
| if err != nil { | ||
| slog.Error("Failed to generate config from rules", "error", err) | ||
| return | ||
| } | ||
| newVM, err := config.NewFromScript(finickyConfigAPIJS, namespace, script) | ||
| if err != nil { | ||
| slog.Error("Failed to rebuild VM from rules", "error", err) | ||
| return | ||
| } | ||
| vm = newVM | ||
| shouldKeepRunning = vm.GetAllConfigOptions().KeepRunning | ||
| go checkForUpdates() |
There was a problem hiding this comment.
Apply logRequests after rebuilding from saved rules.
This branch swaps in newVM but only re-syncs KeepRunning and update checks. The UI now saves logRequests too, yet logger.SetupFile(...) is never re-run here, so that toggle won’t take effect until a later reload or restart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/finicky/src/main.go` around lines 151 - 163, After swapping in newVM
(from config.NewFromScript) you must also apply the saved logRequests setting so
the logger reflects the rebuilt config immediately; after setting vm = newVM and
updating shouldKeepRunning = vm.GetAllConfigOptions().KeepRunning, call
logger.SetupFile(...) (or the existing logger initialization routine) with the
new VM's logRequests value (e.g., vm.GetAllConfigOptions().LogRequests or
newVM.GetAllConfigOptions().LogRequests) before launching checkForUpdates() so
the logRequests toggle takes effect without a restart.
| {#if description} | ||
| <p class="page-description">{description}</p> | ||
| <p class="page-description">{@html description}</p> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the PageContainer.svelte file
cat -n packages/finicky-ui/src/components/PageContainer.svelteRepository: johnste/finicky
Length of output: 1809
🏁 Script executed:
# Find all usages of PageContainer component
rg "PageContainer" --type svelte --type ts --type js -B 2 -A 2Repository: johnste/finicky
Length of output: 89
🏁 Script executed:
# Check imports to understand the component's scope
rg "import.*PageContainer" -B 1 -A 1Repository: johnste/finicky
Length of output: 1551
🏁 Script executed:
# Check TestUrl.svelte for PageContainer usage
rg -A 10 -B 5 "PageContainer" packages/finicky-ui/src/pages/TestUrl.svelteRepository: johnste/finicky
Length of output: 1221
🏁 Script executed:
# Check StartPage.svelte for PageContainer usage
rg -A 10 -B 5 "PageContainer" packages/finicky-ui/src/pages/StartPage.svelteRepository: johnste/finicky
Length of output: 1499
🏁 Script executed:
# Check Rules.svelte for PageContainer usage
rg -A 10 -B 5 "PageContainer" packages/finicky-ui/src/pages/Rules.svelteRepository: johnste/finicky
Length of output: 1327
🏁 Script executed:
# Check LogViewer.svelte for PageContainer usage
rg -A 10 -B 5 "PageContainer" packages/finicky-ui/src/pages/LogViewer.svelteRepository: johnste/finicky
Length of output: 1154
🏁 Script executed:
# Check About.svelte for PageContainer usage
rg -A 10 -B 5 "PageContainer" packages/finicky-ui/src/pages/About.svelteRepository: johnste/finicky
Length of output: 878
Make raw HTML in description an explicit trusted API.
The {@html} rendering on this string prop creates a safety boundary that isn't visible in the type signature. While the current callers (Rules.svelte) intentionally pass hardcoded HTML with styled elements and code tags, there's no mechanism to prevent future callers from passing user-controlled data into this unsafe sink. Use a separately named prop (e.g., descriptionHtml) or expose a named slot for rich content so the security boundary is explicit and clear to consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/finicky-ui/src/components/PageContainer.svelte` around lines 21 -
22, The component currently injects raw HTML via the description prop using
{`@html` description}; change the public API to make this a consciously trusted
sink by introducing a new prop name or slot (e.g., descriptionHtml prop or a
named <slot name="description">) and stop calling {`@html` description}. Update
PageContainer.svelte to render plain text for the existing description prop
(e.g., {description}) and only use {`@html` descriptionHtml} (or render the named
slot) so the intent is explicit, and update callers like Rules.svelte to pass
hardcoded HTML into the new descriptionHtml prop or into the named slot instead
of the original description prop.
| function scheduleSave() { | ||
| clearTimeout(saveTimer); | ||
| pendingSave = true; | ||
| saveTimer = setTimeout(() => { | ||
| const filteredRules = rules.filter( | ||
| (r) => r.match.some((m) => m.trim() !== "") && r.browser.trim() !== "" | ||
| ); | ||
| const payload: RulesFile = { | ||
| defaultBrowser: rulesFile.defaultBrowser, | ||
| defaultProfile: rulesFile.defaultProfile, | ||
| rules: filteredRules, | ||
| }; | ||
| window.finicky.sendMessage({ type: "saveRules", payload }); | ||
| pendingSave = false; | ||
| }, SAVE_DEBOUNCE); | ||
| } |
There was a problem hiding this comment.
Timer not cleared on component destroy could cause memory leak.
The saveTimer set by scheduleSave() is never cleaned up when the component is destroyed. If the user navigates away before the debounce completes, the timer will still fire and attempt to access unmounted component state.
🛠️ Proposed cleanup
+import { onDestroy, untrack, tick } from "svelte";
let saveTimer: ReturnType<typeof setTimeout>;
+onDestroy(() => {
+ clearTimeout(saveTimer);
+});📝 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.
| function scheduleSave() { | |
| clearTimeout(saveTimer); | |
| pendingSave = true; | |
| saveTimer = setTimeout(() => { | |
| const filteredRules = rules.filter( | |
| (r) => r.match.some((m) => m.trim() !== "") && r.browser.trim() !== "" | |
| ); | |
| const payload: RulesFile = { | |
| defaultBrowser: rulesFile.defaultBrowser, | |
| defaultProfile: rulesFile.defaultProfile, | |
| rules: filteredRules, | |
| }; | |
| window.finicky.sendMessage({ type: "saveRules", payload }); | |
| pendingSave = false; | |
| }, SAVE_DEBOUNCE); | |
| } | |
| import { onDestroy } from "svelte"; | |
| let saveTimer: ReturnType<typeof setTimeout>; | |
| onDestroy(() => { | |
| clearTimeout(saveTimer); | |
| }); | |
| function scheduleSave() { | |
| clearTimeout(saveTimer); | |
| pendingSave = true; | |
| saveTimer = setTimeout(() => { | |
| const filteredRules = rules.filter( | |
| (r) => r.match.some((m) => m.trim() !== "") && r.browser.trim() !== "" | |
| ); | |
| const payload: RulesFile = { | |
| defaultBrowser: rulesFile.defaultBrowser, | |
| defaultProfile: rulesFile.defaultProfile, | |
| rules: filteredRules, | |
| }; | |
| window.finicky.sendMessage({ type: "saveRules", payload }); | |
| pendingSave = false; | |
| }, SAVE_DEBOUNCE); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/finicky-ui/src/pages/Rules.svelte` around lines 46 - 61, The
debounce timer set by scheduleSave() (saveTimer) isn't cleared on component
teardown; add a Svelte onDestroy handler that calls clearTimeout(saveTimer) (and
clears or nulls saveTimer and pendingSave if desired) to prevent the timeout
from firing after the component is unmounted; locate scheduleSave, saveTimer,
and SAVE_DEBOUNCE in the file and register onDestroy to perform the cleanup.
| // Request data from native on mount | ||
| $effect(() => { | ||
| window.finicky.sendMessage({ type: "getRules" }); | ||
| window.finicky.sendMessage({ type: "getInstalledBrowsers" }); | ||
| }); |
There was a problem hiding this comment.
Mount-only effect will re-run on every reactive update.
In Svelte 5, $effect blocks run whenever their dependencies change. This effect has no explicit dependencies but will still re-run. For mount-only logic, use onMount instead, or wrap in untrack to prevent re-runs.
🛠️ Proposed fix using onMount
+import { onMount, untrack, tick } from "svelte";
-// Request data from native on mount
-$effect(() => {
- window.finicky.sendMessage({ type: "getRules" });
- window.finicky.sendMessage({ type: "getInstalledBrowsers" });
-});
+// Request data from native on mount
+onMount(() => {
+ window.finicky.sendMessage({ type: "getRules" });
+ window.finicky.sendMessage({ type: "getInstalledBrowsers" });
+});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/finicky-ui/src/pages/Rules.svelte` around lines 127 - 131, The
$effect block that calls window.finicky.sendMessage({ type: "getRules" }) and
window.finicky.sendMessage({ type: "getInstalledBrowsers" }) is being used for
mount-only behavior but will re-run on reactive updates; replace it with
Svelte's onMount (import onMount from "svelte") and move those sendMessage calls
into an onMount callback (or alternatively wrap them in untrack) so the messages
are sent only once on component mount; update the block that currently uses
$effect to call onMount(() => { ... }) referencing the same
window.finicky.sendMessage calls.
| // Sync when the rulesFile prop is updated from the backend. | ||
| // Does NOT read profilesByBrowser — keeps profile fetches from resetting local edits. | ||
| $: { | ||
| keepRunning = rulesFile.options?.keepRunning ?? config.options?.keepRunning ?? true; | ||
| hideIcon = rulesFile.options?.hideIcon ?? config.options?.hideIcon ?? false; | ||
| logRequests = rulesFile.options?.logRequests ?? config.options?.logRequests ?? false; | ||
| checkForUpdates = rulesFile.options?.checkForUpdates ?? config.options?.checkForUpdates ?? true; | ||
| defaultBrowser = isJSConfig ? (config.defaultBrowser ?? "") : (rulesFile.defaultBrowser ?? ""); | ||
| defaultProfile = rulesFile.defaultProfile ?? ""; | ||
| defaultBrowserIsCustom = defaultBrowser !== "" && !installedBrowsers.includes(defaultBrowser); | ||
| } | ||
| // Separate statement so profilesByBrowser changes only update this derived flag, | ||
| // not the browser/profile values above. | ||
| $: defaultProfileIsCustom = defaultProfile !== "" && !(profilesByBrowser[defaultBrowser] ?? []).includes(defaultProfile); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/finicky-ui/src/pages/StartPage.svelte | sed -n '35,55p'Repository: johnste/finicky
Length of output: 1395
🏁 Script executed:
# Let's see more context around the reactive blocks to understand variable declarations
cat -n packages/finicky-ui/src/pages/StartPage.svelte | sed -n '1,80p'Repository: johnste/finicky
Length of output: 4136
🏁 Script executed:
# Search for where installedBrowsers comes from and how it's used
rg -n "installedBrowsers" packages/finicky-ui/src/pages/StartPage.svelte -A 2 -B 2Repository: johnste/finicky
Length of output: 801
🏁 Script executed:
# Check if there are imports or variable declarations for the key variables
rg -n "let.*installedBrowsers|let.*defaultBrowser|let.*profilesByBrowser" packages/finicky-ui/src/pages/StartPage.svelteRepository: johnste/finicky
Length of output: 320
Move defaultBrowserIsCustom to a separate reactive statement to prevent browser-list updates from resetting local edits.
The sync block reads installedBrowsers (line 47), making it a reactive dependency. When async browser discovery completes and updates the installedBrowsers prop, the entire block reruns and overwrites defaultBrowser and defaultProfile with their prop values, wiping out any local changes. Move defaultBrowserIsCustom into its own $: statement like defaultProfileIsCustom, so derived flags update independently from backend sync.
Suggested fix
$: {
keepRunning = rulesFile.options?.keepRunning ?? config.options?.keepRunning ?? true;
hideIcon = rulesFile.options?.hideIcon ?? config.options?.hideIcon ?? false;
logRequests = rulesFile.options?.logRequests ?? config.options?.logRequests ?? false;
checkForUpdates = rulesFile.options?.checkForUpdates ?? config.options?.checkForUpdates ?? true;
defaultBrowser = isJSConfig ? (config.defaultBrowser ?? "") : (rulesFile.defaultBrowser ?? "");
defaultProfile = rulesFile.defaultProfile ?? "";
- defaultBrowserIsCustom = defaultBrowser !== "" && !installedBrowsers.includes(defaultBrowser);
}
+ $: defaultBrowserIsCustom = defaultBrowser !== "" && !installedBrowsers.includes(defaultBrowser);
$: defaultProfileIsCustom = defaultProfile !== "" && !(profilesByBrowser[defaultBrowser] ?? []).includes(defaultProfile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/finicky-ui/src/pages/StartPage.svelte` around lines 38 - 51, The
reactive sync block currently assigns defaultBrowserIsCustom and therefore
depends on installedBrowsers, causing backend updates to rerun the block and
overwrite local edits; remove the defaultBrowserIsCustom assignment from the big
$: block (the block that sets keepRunning, hideIcon, logRequests,
checkForUpdates, defaultBrowser, defaultProfile) and add a separate reactive
statement like the existing defaultProfileIsCustom one that computes
defaultBrowserIsCustom = defaultBrowser !== "" &&
!installedBrowsers.includes(defaultBrowser), so only that derived flag reacts to
installedBrowsers while defaultBrowser/defaultProfile stay stable (references:
defaultBrowserIsCustom, installedBrowsers, defaultProfileIsCustom,
profilesByBrowser, defaultBrowser, defaultProfile).
scripts/dev.sh
Outdated
| case "$1" in | ||
| 0|normal) | ||
| echo "Scenario 0: normal (auto-detect configs)" | ||
| "$BINARY" --window | ||
| ;; | ||
| 1|none) | ||
| echo "Scenario 1: no JS config, no rules.json" | ||
| "$BINARY" --window --no-config --rules "$NO_RULES" | ||
| ;; | ||
| 2|js) | ||
| echo "Scenario 2: JS config only" | ||
| "$BINARY" --window --config "$JS_CONFIG" --rules "$NO_RULES" | ||
| ;; | ||
| 3|json) | ||
| echo "Scenario 3: rules.json only" | ||
| "$BINARY" --window --no-config --rules "$JSON_RULES" | ||
| ;; | ||
| 4|both) | ||
| echo "Scenario 4: JS config + rules.json" | ||
| "$BINARY" --window --config "$JS_CONFIG" --rules "$JSON_RULES" | ||
| ;; | ||
| *) | ||
| echo "Usage: $0 [--mock-update [version]] <scenario>" | ||
| echo "" | ||
| echo " 0 normal Auto-detect configs (default behavior)" | ||
| echo " 1 none No JS config, no rules.json" | ||
| echo " 2 js JS config only ($JS_CONFIG)" | ||
| echo " 3 json rules.json only ($JSON_RULES)" | ||
| echo " 4 both JS config + rules.json" | ||
| echo "" | ||
| echo "Flags:" | ||
| echo " --mock-update [version] Show update banner (default: 1.99.0)" | ||
| exit 0 |
There was a problem hiding this comment.
Honor the documented default scenario.
The header says 4|both is the default, but an empty $1 falls through to the usage block. Default the selector before the case so ./scripts/dev.sh actually runs scenario 4.
Suggested fix
-case "$1" in
+SCENARIO="${1:-4}"
+case "$SCENARIO" in📝 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.
| case "$1" in | |
| 0|normal) | |
| echo "Scenario 0: normal (auto-detect configs)" | |
| "$BINARY" --window | |
| ;; | |
| 1|none) | |
| echo "Scenario 1: no JS config, no rules.json" | |
| "$BINARY" --window --no-config --rules "$NO_RULES" | |
| ;; | |
| 2|js) | |
| echo "Scenario 2: JS config only" | |
| "$BINARY" --window --config "$JS_CONFIG" --rules "$NO_RULES" | |
| ;; | |
| 3|json) | |
| echo "Scenario 3: rules.json only" | |
| "$BINARY" --window --no-config --rules "$JSON_RULES" | |
| ;; | |
| 4|both) | |
| echo "Scenario 4: JS config + rules.json" | |
| "$BINARY" --window --config "$JS_CONFIG" --rules "$JSON_RULES" | |
| ;; | |
| *) | |
| echo "Usage: $0 [--mock-update [version]] <scenario>" | |
| echo "" | |
| echo " 0 normal Auto-detect configs (default behavior)" | |
| echo " 1 none No JS config, no rules.json" | |
| echo " 2 js JS config only ($JS_CONFIG)" | |
| echo " 3 json rules.json only ($JSON_RULES)" | |
| echo " 4 both JS config + rules.json" | |
| echo "" | |
| echo "Flags:" | |
| echo " --mock-update [version] Show update banner (default: 1.99.0)" | |
| exit 0 | |
| SCENARIO="${1:-4}" | |
| case "$SCENARIO" in | |
| 0|normal) | |
| echo "Scenario 0: normal (auto-detect configs)" | |
| "$BINARY" --window | |
| ;; | |
| 1|none) | |
| echo "Scenario 1: no JS config, no rules.json" | |
| "$BINARY" --window --no-config --rules "$NO_RULES" | |
| ;; | |
| 2|js) | |
| echo "Scenario 2: JS config only" | |
| "$BINARY" --window --config "$JS_CONFIG" --rules "$NO_RULES" | |
| ;; | |
| 3|json) | |
| echo "Scenario 3: rules.json only" | |
| "$BINARY" --window --no-config --rules "$JSON_RULES" | |
| ;; | |
| 4|both) | |
| echo "Scenario 4: JS config + rules.json" | |
| "$BINARY" --window --config "$JS_CONFIG" --rules "$JSON_RULES" | |
| ;; | |
| *) | |
| echo "Usage: $0 [--mock-update [version]] <scenario>" | |
| echo "" | |
| echo " 0 normal Auto-detect configs (default behavior)" | |
| echo " 1 none No JS config, no rules.json" | |
| echo " 2 js JS config only ($JS_CONFIG)" | |
| echo " 3 json rules.json only ($JSON_RULES)" | |
| echo " 4 both JS config + rules.json" | |
| echo "" | |
| echo "Flags:" | |
| echo " --mock-update [version] Show update banner (default: 1.99.0)" | |
| exit 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev.sh` around lines 48 - 80, Default the scenario selector to "4"
(both) before the case block so an empty $1 runs scenario 4; modify the code
around the existing case "$1" in block (and the scenario labels 0|normal ..
4|both) to use a pre-initialized value instead of raw $1 — e.g., if $1 is empty
set a local variable or reset positional parameters so the case tests a
non-empty selector (default "4") and ensure existing flags like --mock-update
remain honored; update references from case "$1" in to case "$SCENARIO" in (or
set -- 4 ...) so ./scripts/dev.sh without args executes the "4|both" branch.
Fixes #380
Summary by CodeRabbit
New Features
Improvements