Conversation
Closed
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.
Comment on lines
+136
to
+137
| {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.
Suggested change
| {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"}, |
Comment on lines
+140
to
+165
| // 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.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR adds JSON rule support and UI for editing rules, implements macOS browser/profile detection, introduces a centralized resolver that merges JS config and cached JSON rules, refactors VM/config handling, and wires native UI messaging for rules and browser/profile discovery. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Main as Main Process
participant Resolver as Resolver
participant VM as Config VM
participant Rules as Rules Cache
participant Launcher as Browser Launcher
User->>Main: URL received
Main->>Resolver: ResolveURL(vm, url, opener, bg)
alt VM (JS config) present
Resolver->>VM: evaluateURL(originalUrl, opener, cachedRules?)
VM-->>Resolver: BrowserResult (or error)
Resolver->>Rules: getCachedRules()
Rules-->>Resolver: JSON handlers
Resolver->>Resolver: merge JS result with cached rules
else No JS config
Resolver->>Rules: getCachedRules()
Rules-->>Resolver: BrowserResult from JSON
end
Resolver-->>Main: BrowserConfig (URL, profile, OpenInBackground)
Main->>Launcher: Launch browser with resolved config
Launcher-->>User: URL opened
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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
Comment on lines
+143
to
+165
| 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).
Comment on lines
+151
to
+163
| 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.
johnste
force-pushed
the
feat/ui-rules
branch
2 times, most recently
from
85e4e98 to
0ec1d3d
Compare
johnste
force-pushed
the
feat/ui-rules
branch
4 times, most recently
from
07f7bec to
c90c2b6
Compare
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
apps/finicky/src/version/version.go (1)
239-246:⚠️ Potential issue | 🟡 MinorNormalize the mock tag more defensively.
Line 241 only strips lowercase
v, so values likeV1.2.3orv1.2.3still produce malformed release URLs. Trim whitespace first, then strip bothvandVbefore formatting the links.Suggested fix
if mockVersion := os.Getenv("FINICKY_MOCK_UPDATE"); mockVersion != "" { slog.Info("FINICKY_MOCK_UPDATE set, returning mock update", "version", mockVersion) - mockTag := strings.TrimPrefix(mockVersion, "v") + mockTag := strings.TrimSpace(mockVersion) + mockTag = strings.TrimPrefix(mockTag, "v") + mockTag = strings.TrimPrefix(mockTag, "V") return &ReleaseInfo{ HasUpdate: true, LatestVersion: mockVersion, DownloadUrl: fmt.Sprintf("https://github.com/johnste/finicky/releases/tag/v%s", mockTag), ReleaseUrl: fmt.Sprintf("https://github.com/johnste/finicky/releases/tag/v%s", mockTag), }, true, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/version/version.go` around lines 239 - 246, The mock-tag normalization is too narrow: when FINICKY_MOCK_UPDATE contains surrounding whitespace or an uppercase "V" the generated mockTag and URLs are malformed; in the block that reads os.Getenv("FINICKY_MOCK_UPDATE") (the mockVersion variable and mockTag usage that returns a *ReleaseInfo), first strings.TrimSpace(mockVersion) to remove whitespace, then strip both lowercase and uppercase prefixes (e.g. call strings.TrimPrefix twice or use a small helper to remove "v"/"V") before assigning mockTag, and keep using mockTag when constructing DownloadUrl and ReleaseUrl so links are valid.packages/finicky-ui/src/App.svelte (2)
157-164:⚠️ Potential issue | 🟡 MinorReplace
on:clickwithonclickfor Svelte 5 compatibility.As previously noted, Svelte 5 requires standard DOM event attributes (
onclick) for native HTML elements like<button>. The currenton:clicksyntax is deprecated.Proposed fix
- <button class="config-badge" on:click={() => showPathToast(basename(config.configPath), "Configuration loaded from", config.configPath)}> + <button class="config-badge" onclick={() => showPathToast(basename(config.configPath), "Configuration loaded from", config.configPath)}> ✓ {basename(config.configPath)} </button> {/if} {`#if` rulesFile.path && rulesFile.path !== config.configPath} - <button class="config-badge" on:click={() => showPathToast(basename(rulesFile.path!), "Configuration loaded from", rulesFile.path!)}> + <button class="config-badge" onclick={() => showPathToast(basename(rulesFile.path!), "Configuration loaded from", rulesFile.path!)}> ✓ {basename(rulesFile.path)} </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/App.svelte` around lines 157 - 164, Replace the Svelte-specific event bindings on the configuration badge buttons with standard DOM attributes for Svelte 5: change both occurrences of on:click on the buttons that call showPathToast (the one using basename(config.configPath) and the one using basename(rulesFile.path!)) to use onclick instead, preserving the exact arrow function call and arguments to showPathToast so the handlers (basename, config.configPath, rulesFile.path, and showPathToast) remain unchanged.
109-111:⚠️ Potential issue | 🟡 MinorStartup message requests could fail silently before window.finicky is ready.
As previously noted, these
sendMessagecalls execute during component initialization beforewindow.webkit?.messageHandlers?.finickymay be fully ready. Consider deferring untilonMountto ensure the native bridge is available.Suggested fix using onMount
+import { onMount } from "svelte"; // Drain messages that arrived before the Svelte app was ready. for (const msg of _preloadQueue) { handleMessage(msg); } -// Request rules file info on startup so the footer badge appears immediately. -window.finicky.sendMessage({ type: "getRules" }); -window.finicky.sendMessage({ type: "getInstalledBrowsers" }); +onMount(() => { + // Request rules file info on startup so the footer badge appears immediately. + 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/App.svelte` around lines 109 - 111, The startup sendMessage calls (window.finicky.sendMessage({ type: "getRules" }) and window.finicky.sendMessage({ type: "getInstalledBrowsers" })) run during component init and can fail before the native bridge is ready; move these calls into an onMount handler (import and use Svelte's onMount) and guard for the bridge (e.g., check window.finicky or window.webkit?.messageHandlers?.finicky) before calling sendMessage so the requests are deferred until the native bridge is available.apps/finicky/src/window/window.go (2)
268-275:⚠️ Potential issue | 🟡 MinorSilent failure when browser name is missing or wrong type.
As previously noted, if
msg["browser"]is missing or not a string,browserNamewill be empty, leading to a potentially confusing response. The issue remains unaddressed.Proposed validation
func handleGetBrowserProfiles(msg map[string]interface{}) { - browserName, _ := msg["browser"].(string) + browserName, ok := msg["browser"].(string) + if !ok || browserName == "" { + slog.Error("getBrowserProfiles message missing or invalid browser field") + return + } profiles := browser.GetProfilesForBrowser(browserName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/window/window.go` around lines 268 - 275, handleGetBrowserProfiles silently proceeds when msg["browser"] is missing or not a string; validate that msg["browser"] is present and of type string (assign to browserName only if ok), and if invalid return early with a clear error payload via SendMessageToWebView (e.g., include an "error" or "status":"invalid_request" field) instead of calling browser.GetProfilesForBrowser with an empty name; update logic around handleGetBrowserProfiles, browserName, browser.GetProfilesForBrowser and the SendMessageToWebView call to perform this validation and emit the error response.
223-246:⚠️ Potential issue | 🟡 MinorEarly validation failures still lack UI feedback.
The
rules.Saveerror at line 244 now sends asaveRulesErrormessage (addressing the past review), but the earlier validation failures at lines 226-227, 232-233, and 238-239 still return silently without notifying the UI. Users won't know why their save failed in these cases.Proposed fix to add error responses for all failure paths
func handleSaveRules(msg map[string]interface{}) { payload, ok := msg["payload"] if !ok { slog.Error("saveRules message missing payload field") + SendMessageToWebView("saveRulesError", map[string]interface{}{"error": "Missing payload"}) return } payloadBytes, err := json.Marshal(payload) if err != nil { slog.Error("Failed to marshal saveRules payload", "error", err) + SendMessageToWebView("saveRulesError", map[string]interface{}{"error": "Invalid payload format"}) return } var rf rules.RulesFile if err := json.Unmarshal(payloadBytes, &rf); err != nil { slog.Error("Failed to parse saveRules payload", "error", err) + SendMessageToWebView("saveRulesError", map[string]interface{}{"error": "Failed to parse rules"}) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/window/window.go` around lines 223 - 246, The handler handleSaveRules currently returns silently on early failures (missing payload, json.Marshal error, json.Unmarshal error); update each failure path to call SendMessageToWebView with a descriptive error event (e.g., "saveRulesError") and include the error details or a clear message (for missing payload include a message like "missing payload", for marshal/unmarshal include err.Error()) before returning, keeping the existing Save error behavior intact so the UI receives feedback for all failure cases.
🧹 Nitpick comments (5)
packages/finicky-ui/src/pages/TestUrl.svelte (2)
6-8: Consider using the store directly or a derived binding.The current pattern initializes
testUrlfrom$testUrlInputonce at component creation. If$testUrlInputis updated externally (e.g., from another component), this component won't reflect those changes. If bidirectional sync is needed, consider using$testUrlInputdirectly in the template, or use$effectto keep them synchronized.🤖 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 6 - 8, The component initializes a local let testUrl = $testUrlInput once, so it won’t update when the store changes; replace that pattern by either using $testUrlInput directly in the template (remove the local testUrl) or make testUrl reactive to the store with a reactive assignment ($: testUrl = $testUrlInput) or bind the input to the store (bind:value={$testUrlInput}) so the component stays synchronized with the testUrlInput store; update any references to the local testUrl accordingly.
214-216: Consider using theme variables for warning colors.The hint message uses hardcoded
rgba(245, 188, 28, ...)values instead of the--log-warningtheme variable. This could cause inconsistency if the warning color is adjusted inapp.css.Suggested approach
.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;Alternatively, define dedicated
--hint-bgand--hint-bordertokens inapp.css.🤖 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 214 - 216, The CSS block for the hint message currently hardcodes rgba(245,188,28,...) for background and border; replace those hardcoded values with the theme token --log-warning (e.g. use rgba(var(--log-warning) / 0.06) for background and rgba(var(--log-warning) / 0.25) for border) or, if you prefer explicit tokens, add new variables --hint-bg and --hint-border to app.css and reference them here in TestUrl.svelte so the hint styling follows the global theme; update the background and border declarations in the same CSS selector that contains the current background/border/border-radius rules.packages/finicky-ui/src/components/Tooltip.svelte (1)
15-18: Consider adding accessibility attributes.The tooltip works for mouse users but screen readers won't announce the tooltip content. Consider adding
aria-describedbyortitleattribute for accessibility.Example with basic accessibility
-<span class="tooltip-anchor" class:block> +<span class="tooltip-anchor" class:block title={text}> {`@render` children()} <span class="tooltip-text">{text}</span> </span>Or for richer accessibility, use
aria-describedbywith a unique ID linking the anchor to the tooltip text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/finicky-ui/src/components/Tooltip.svelte` around lines 15 - 18, The tooltip isn't exposed to screen readers; update Tooltip.svelte to generate a unique id (e.g., tooltipId) and attach it to the tooltip text element (the span with class "tooltip-text"), add aria-describedby={tooltipId} to the anchor span (class "tooltip-anchor") and also include a fallback title attribute on the anchor; mark the tooltip text with role="tooltip" and ensure the anchor is focusable (tabindex) so keyboard users trigger the description. Use the existing children() and text symbols when wiring these attributes.apps/finicky/src/resolver/resolver.go (1)
23-40: Consider usingsync.RWMutexfor better read concurrency.
getCachedRulesis a read-only operation called on every URL resolution, whileSetCachedRulesis called infrequently (only on rule saves). UsingRWMutexwould allow concurrent reads.♻️ Proposed optimization
var ( - cachedRulesMu sync.Mutex + cachedRulesMu sync.RWMutex cachedRulesFile rules.RulesFile ) func SetCachedRules(rf rules.RulesFile) { cachedRulesMu.Lock() cachedRulesFile = rf cachedRulesMu.Unlock() } func getCachedRules() rules.RulesFile { - cachedRulesMu.Lock() - defer cachedRulesMu.Unlock() + cachedRulesMu.RLock() + defer cachedRulesMu.RUnlock() return cachedRulesFile }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/resolver/resolver.go` around lines 23 - 40, The current mutex (cachedRulesMu) serializes all access; change its type from sync.Mutex to sync.RWMutex, keep cachedRulesFile as is, call cachedRulesMu.Lock()/Unlock() in SetCachedRules and use cachedRulesMu.RLock()/RUnlock() in getCachedRules so multiple concurrent readers of getCachedRules can proceed while writes still exclusive; update the variable declaration for cachedRulesMu and replace the locking calls in SetCachedRules and getCachedRules accordingly (referencing cachedRulesMu, SetCachedRules, getCachedRules, cachedRulesFile).packages/finicky-ui/src/pages/StartPage.svelte (1)
57-85: Duplicated payload construction betweensave()andscheduleSave().Both functions construct an identical payload object for the
saveRulesmessage. This duplication increases maintenance burden and risk of drift.♻️ Extract payload construction into a helper
+ function buildRulesPayload() { + return { + ...rulesFile, + defaultBrowser, + defaultProfile, + options: { keepRunning, hideIcon, logRequests, checkForUpdates }, + }; + } + function save() { if (isJSConfig) return; clearTimeout(saveTimer); window.finicky.sendMessage({ type: "saveRules", - payload: { - ...rulesFile, - defaultBrowser, - defaultProfile, - options: { keepRunning, hideIcon, logRequests, checkForUpdates }, - }, + payload: buildRulesPayload(), }); } function scheduleSave() { if (isJSConfig) return; clearTimeout(saveTimer); saveTimer = setTimeout(() => { window.finicky.sendMessage({ type: "saveRules", - payload: { - ...rulesFile, - defaultBrowser, - defaultProfile, - options: { keepRunning, hideIcon, logRequests, checkForUpdates }, - }, + payload: buildRulesPayload(), }); }, 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/StartPage.svelte` around lines 57 - 85, Duplicate payload creation in save() and scheduleSave() should be extracted into a single helper (e.g., buildSavePayload) that returns the object sent with the "saveRules" message; update save() and the setTimeout callback in scheduleSave() to call that helper and reuse the same payload, keeping existing guards (isJSConfig), timeout handling (saveTimer/clearTimeout), and SAVE_DEBOUNCE behavior intact and referencing rulesFile, defaultBrowser, defaultProfile and options: { keepRunning, hideIcon, logRequests, checkForUpdates }.
🤖 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 50-61: The loop in detect.go uses displayNameAtPath (presentation
label) as the durable key, which breaks profile matching; switch to using
bundle.bundleIdentifier as the stable identifier (use bundle.bundleIdentifier
when checking/adding to seen and when emitting the detected browser id) and keep
the display name from displayNameAtPath separately for UI rendering (e.g.,
store/displayName in a parallel field or object). Update logic around
isLikelyBrowser, seen, names and the values produced from appURLs so the
identifier returned is the bundle ID while the human name remains available for
UI.
In `@apps/finicky/src/main.go`:
- Around line 173-176: The code calls vm.GetAllConfigOptions() without checking
vm for nil, which can panic because setupVM may return (nil, nil); update all
places that call vm.GetAllConfigOptions() (e.g., the block setting
shouldKeepRunning and timeoutChan and the other occurrences around where vm is
used) to first verify vm != nil and handle the nil case (e.g., treat as default
config or skip calling GetAllConfigOptions and set shouldKeepRunning/timeoutChan
appropriately) and ensure any downstream logic that assumes vm is non-nil either
returns an error or uses safe defaults; specifically adjust the code paths that
obtain vm from setupVM to guard before invoking GetAllConfigOptions.
- Around line 211-217: After setupVM returns, vm may be nil and calling
vm.GetAllConfigOptions() causes a nil pointer panic; modify the logic after
setupVM(cfw, namespace) so you first handle setupErr via
handleRuntimeError(setupErr) as before, then check if vm != nil before calling
vm.GetAllConfigOptions() or launching checkForUpdates(); if vm is nil set
shouldKeepRunning = false (or leave previous value if that is intended) and
avoid calling vm.GetAllConfigOptions() and starting go checkForUpdates(),
otherwise read shouldKeepRunning = vm.GetAllConfigOptions().KeepRunning and then
start go checkForUpdates().
- Around line 245-246: The call to C.RunApp can dereference a nil vm when
setupVM returns (nil, nil); update the code around setupVM and the C.RunApp
invocation to guard against a nil vm by checking vm != nil before calling
vm.GetAllConfigOptions().HideIcon (or by using a safe default boolean when vm is
nil), e.g., compute shouldHideIcon := false; if vm != nil { shouldHideIcon =
vm.GetAllConfigOptions().HideIcon } and then pass C.bool(!shouldHideIcon) into
C.RunApp; ensure references are to setupVM, vm, GetAllConfigOptions, HideIcon
and C.RunApp.
In `@apps/finicky/src/rules/rules.go`:
- Around line 180-221: ToJSConfigScript currently always serializes
rf.DefaultBrowser as a plain string, ignoring rf.DefaultProfile; change it to
emit the same {name, profile} object shape used for handler browsers when
rf.DefaultProfile != "" so fallback routing preserves the selected profile.
Concretely, replace the logic that sets defaultBrowserJSON: build an interface{}
defaultBrowserObj = rf.DefaultBrowser (string) unless rf.DefaultProfile != "" in
which case set defaultBrowserObj = map[string]string{"name": rf.DefaultBrowser,
"profile": rf.DefaultProfile}, then json.Marshal(defaultBrowserObj) and use that
JSON in the returned config; keep error handling around the marshal as-is.
Target symbols: ToJSConfigScript, rf.DefaultBrowser, rf.DefaultProfile,
defaultBrowserJSON.
In `@packages/finicky-ui/src/components/BrowserProfileSelector.svelte`:
- Line 127: The `.empty` class condition uses `!browser` inside a block that
only renders when `browser` is truthy, so it can never be true; update the class
directive on the select wrapper in BrowserProfileSelector.svelte to check the
selected profile instead (e.g. replace `!browser` with `!profile` or
`!selectedProfile`) so it becomes `class:empty={required && !profile &&
!isCustom}` (or `!selectedProfile` if that is the prop name), ensuring the empty
state reflects a missing profile selection.
In `@packages/finicky-ui/src/pages/LogViewer.svelte`:
- Around line 45-58: The description slot is wrapped by PageContainer in a <p>,
so having a <div class="log-controls"> inside the description produces invalid
HTML; change the block rendered by the description() snippet to use phrasing
content (e.g., replace the outer <div class="log-controls"> with a <span
class="log-controls"> or otherwise move the controls out of the description
slot) while retaining the inputs and handlers (bind:checked={showDebug},
onchange storing to localStorage, and the buttons using onClearLogs and
copyLogs) so the UI and behavior remain unchanged but the markup is valid.
In `@packages/finicky-ui/src/pages/Rules.svelte`:
- Around line 146-189: The inner $effect that updates rowProfileIsCustom is
currently declared inside the outer $effect (the block manipulating rules,
rowIsCustom and fetching profiles) which causes duplicate effect subscriptions
on each outer re-run; extract that inner $effect into its own top-level $effect
(so it's not nested), keep its logic referencing rules, profilesByBrowser and
rowProfileIsCustom unchanged, and ensure the outer $effect only performs rules
normalization and the untrack fetch loop (including proper closure of the
untrack call) so effects are registered once and no duplicate subscriptions are
created.
---
Duplicate comments:
In `@apps/finicky/src/version/version.go`:
- Around line 239-246: The mock-tag normalization is too narrow: when
FINICKY_MOCK_UPDATE contains surrounding whitespace or an uppercase "V" the
generated mockTag and URLs are malformed; in the block that reads
os.Getenv("FINICKY_MOCK_UPDATE") (the mockVersion variable and mockTag usage
that returns a *ReleaseInfo), first strings.TrimSpace(mockVersion) to remove
whitespace, then strip both lowercase and uppercase prefixes (e.g. call
strings.TrimPrefix twice or use a small helper to remove "v"/"V") before
assigning mockTag, and keep using mockTag when constructing DownloadUrl and
ReleaseUrl so links are valid.
In `@apps/finicky/src/window/window.go`:
- Around line 268-275: handleGetBrowserProfiles silently proceeds when
msg["browser"] is missing or not a string; validate that msg["browser"] is
present and of type string (assign to browserName only if ok), and if invalid
return early with a clear error payload via SendMessageToWebView (e.g., include
an "error" or "status":"invalid_request" field) instead of calling
browser.GetProfilesForBrowser with an empty name; update logic around
handleGetBrowserProfiles, browserName, browser.GetProfilesForBrowser and the
SendMessageToWebView call to perform this validation and emit the error
response.
- Around line 223-246: The handler handleSaveRules currently returns silently on
early failures (missing payload, json.Marshal error, json.Unmarshal error);
update each failure path to call SendMessageToWebView with a descriptive error
event (e.g., "saveRulesError") and include the error details or a clear message
(for missing payload include a message like "missing payload", for
marshal/unmarshal include err.Error()) before returning, keeping the existing
Save error behavior intact so the UI receives feedback for all failure cases.
In `@packages/finicky-ui/src/App.svelte`:
- Around line 157-164: Replace the Svelte-specific event bindings on the
configuration badge buttons with standard DOM attributes for Svelte 5: change
both occurrences of on:click on the buttons that call showPathToast (the one
using basename(config.configPath) and the one using basename(rulesFile.path!))
to use onclick instead, preserving the exact arrow function call and arguments
to showPathToast so the handlers (basename, config.configPath, rulesFile.path,
and showPathToast) remain unchanged.
- Around line 109-111: The startup sendMessage calls
(window.finicky.sendMessage({ type: "getRules" }) and
window.finicky.sendMessage({ type: "getInstalledBrowsers" })) run during
component init and can fail before the native bridge is ready; move these calls
into an onMount handler (import and use Svelte's onMount) and guard for the
bridge (e.g., check window.finicky or window.webkit?.messageHandlers?.finicky)
before calling sendMessage so the requests are deferred until the native bridge
is available.
---
Nitpick comments:
In `@apps/finicky/src/resolver/resolver.go`:
- Around line 23-40: The current mutex (cachedRulesMu) serializes all access;
change its type from sync.Mutex to sync.RWMutex, keep cachedRulesFile as is,
call cachedRulesMu.Lock()/Unlock() in SetCachedRules and use
cachedRulesMu.RLock()/RUnlock() in getCachedRules so multiple concurrent readers
of getCachedRules can proceed while writes still exclusive; update the variable
declaration for cachedRulesMu and replace the locking calls in SetCachedRules
and getCachedRules accordingly (referencing cachedRulesMu, SetCachedRules,
getCachedRules, cachedRulesFile).
In `@packages/finicky-ui/src/components/Tooltip.svelte`:
- Around line 15-18: The tooltip isn't exposed to screen readers; update
Tooltip.svelte to generate a unique id (e.g., tooltipId) and attach it to the
tooltip text element (the span with class "tooltip-text"), add
aria-describedby={tooltipId} to the anchor span (class "tooltip-anchor") and
also include a fallback title attribute on the anchor; mark the tooltip text
with role="tooltip" and ensure the anchor is focusable (tabindex) so keyboard
users trigger the description. Use the existing children() and text symbols when
wiring these attributes.
In `@packages/finicky-ui/src/pages/StartPage.svelte`:
- Around line 57-85: Duplicate payload creation in save() and scheduleSave()
should be extracted into a single helper (e.g., buildSavePayload) that returns
the object sent with the "saveRules" message; update save() and the setTimeout
callback in scheduleSave() to call that helper and reuse the same payload,
keeping existing guards (isJSConfig), timeout handling (saveTimer/clearTimeout),
and SAVE_DEBOUNCE behavior intact and referencing rulesFile, defaultBrowser,
defaultProfile and options: { keepRunning, hideIcon, logRequests,
checkForUpdates }.
In `@packages/finicky-ui/src/pages/TestUrl.svelte`:
- Around line 6-8: The component initializes a local let testUrl = $testUrlInput
once, so it won’t update when the store changes; replace that pattern by either
using $testUrlInput directly in the template (remove the local testUrl) or make
testUrl reactive to the store with a reactive assignment ($: testUrl =
$testUrlInput) or bind the input to the store (bind:value={$testUrlInput}) so
the component stays synchronized with the testUrlInput store; update any
references to the local testUrl accordingly.
- Around line 214-216: The CSS block for the hint message currently hardcodes
rgba(245,188,28,...) for background and border; replace those hardcoded values
with the theme token --log-warning (e.g. use rgba(var(--log-warning) / 0.06) for
background and rgba(var(--log-warning) / 0.25) for border) or, if you prefer
explicit tokens, add new variables --hint-bg and --hint-border to app.css and
reference them here in TestUrl.svelte so the hint styling follows the global
theme; update the background and border declarations in the same CSS selector
that contains the current background/border/border-radius rules.
🪄 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: 0f48f6dc-44ee-4e42-ad1e-faa65cd8b223
📒 Files selected for processing (37)
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.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/Tooltip.sveltepackages/finicky-ui/src/components/icons/Lock.sveltepackages/finicky-ui/src/components/icons/Rules.sveltepackages/finicky-ui/src/components/icons/Warning.sveltepackages/finicky-ui/src/components/icons/X.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
Comment on lines
+50
to
+61
| 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]; |
There was a problem hiding this comment.
Return a stable browser identifier here.
displayNameAtPath: is a presentation label, not a durable key. The new profile flow in apps/finicky/src/browser/launcher.go only matches bundle IDs or browsers.json app_name values, so localized or user-renamed apps will stop resolving profiles even though detection succeeded.
Please key this API on the bundle ID and keep the display name separate for UI rendering.
🤖 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 50 - 61, The loop in
detect.go uses displayNameAtPath (presentation label) as the durable key, which
breaks profile matching; switch to using bundle.bundleIdentifier as the stable
identifier (use bundle.bundleIdentifier when checking/adding to seen and when
emitting the detected browser id) and keep the display name from
displayNameAtPath separately for UI rendering (e.g., store/displayName in a
parallel field or object). Update logic around isLikelyBrowser, seen, names and
the values produced from appURLs so the identifier returned is the bundle ID
while the human name remains available for UI.
| /> | ||
| <button class="clear-btn" onclick={handleClearProfile} aria-label="Clear profile"><XIcon /></button> | ||
| {:else} | ||
| <div class="select-wrapper" class:empty={required && !browser && !isCustom}> |
There was a problem hiding this comment.
The .empty class condition can never be true for the profile dropdown.
The condition required && !browser && !isCustom is inside a block that only renders when browser is truthy (line 114). Since browser is guaranteed to be non-empty here, !browser is always false, making the .empty class unreachable.
If the intent is to highlight when a profile is required but not selected, the condition should likely be:
Suggested fix
- <div class="select-wrapper" class:empty={required && !browser && !isCustom}>
+ <div class="select-wrapper" class:empty={required && !profile && !isProfileCustom}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="select-wrapper" class:empty={required && !browser && !isCustom}> | |
| <div class="select-wrapper" class:empty={required && !profile && !isProfileCustom}> |
🤖 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` at line
127, The `.empty` class condition uses `!browser` inside a block that only
renders when `browser` is truthy, so it can never be true; update the class
directive on the select wrapper in BrowserProfileSelector.svelte to check the
selected profile instead (e.g. replace `!browser` with `!profile` or
`!selectedProfile`) so it becomes `class:empty={required && !profile &&
!isCustom}` (or `!selectedProfile` if that is the prop name), ensuring the empty
state reflects a missing profile selection.
Comment on lines
+45
to
+58
| {#snippet description()} | ||
| <div class="log-controls"> | ||
| <label class="debug-toggle"> | ||
| <input | ||
| type="checkbox" | ||
| bind:checked={showDebug} | ||
| onchange={() => localStorage.setItem("showDebugLogs", String(showDebug))} | ||
| /> | ||
| Show debug | ||
| </label> | ||
| <button onclick={clearLogs}>Clear</button> | ||
| <button onclick={copyLogs}>Copy logs</button> | ||
| <button onclick={onClearLogs}>Clear</button> | ||
| <button onclick={copyLogs}>Copy</button> | ||
| </div> | ||
| </div> | ||
| {/snippet} |
There was a problem hiding this comment.
Use phrasing content in the description snippet.
PageContainer wraps description in a <p> (packages/finicky-ui/src/components/PageContainer.svelte:19-23), so the <div> here produces invalid HTML and the browser will reparent these controls unpredictably. Use phrasing content here (for example a <span class="log-controls">) or move the controls outside the description slot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/finicky-ui/src/pages/LogViewer.svelte` around lines 45 - 58, The
description slot is wrapped by PageContainer in a <p>, so having a <div
class="log-controls"> inside the description produces invalid HTML; change the
block rendered by the description() snippet to use phrasing content (e.g.,
replace the outer <div class="log-controls"> with a <span class="log-controls">
or otherwise move the controls out of the description slot) while retaining the
inputs and handlers (bind:checked={showDebug}, onchange storing to localStorage,
and the buttons using onClearLogs and copyLogs) so the UI and behavior remain
unchanged but the markup is valid.
Comment on lines
+146
to
+189
| $effect(() => { | ||
| if (pendingSave) return; | ||
| const newRules = rulesFile.rules.map((r: Rule) => ({ | ||
| ...r, | ||
| match: Array.isArray(r.match) ? r.match : r.match ? [r.match as unknown as string] : [""], | ||
| })); | ||
| rules = newRules; | ||
| rowIsCustom = newRules.map( | ||
| (r) => r.browser !== "" && !installedBrowsers.includes(r.browser) | ||
| ); | ||
| rowProfileIsCustom = newRules.map( | ||
| (r) => { | ||
| const profile = r.profile ?? ""; | ||
| if (profile === "") return false; | ||
| const profiles = untrack(() => profilesByBrowser[r.browser]); | ||
| if (profiles === undefined) return false; // not fetched yet — correct later | ||
| return !profiles.includes(profile); | ||
| } | ||
| ); | ||
| // Fetch profiles for browsers not yet loaded — untrack to avoid re-triggering on profilesByBrowser changes | ||
| const browsersToFetch = new Set<string>(); | ||
| for (const r of newRules) { | ||
| if (r.browser) browsersToFetch.add(r.browser); | ||
| } | ||
| untrack(() => { | ||
| for (const b of browsersToFetch) { | ||
| if (profilesByBrowser[b] === undefined) { | ||
| window.finicky.sendMessage({ type: "getBrowserProfiles", browser: b }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Once profile lists arrive, correct any rowProfileIsCustom entries that were | ||
| // deferred because profilesByBrowser wasn't loaded yet during the initial sync. | ||
| $effect(() => { | ||
| rules.forEach((r, i) => { | ||
| const profile = r.profile ?? ""; | ||
| if (profile === "") return; | ||
| const profiles = profilesByBrowser[r.browser]; | ||
| if (profiles === undefined) return; // still loading | ||
| rowProfileIsCustom[i] = !profiles.includes(profile); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Nested $effect will create duplicate effect subscriptions on each re-run.
The $effect starting at line 180 is nested inside the $effect starting at line 146. Each time the outer effect re-runs, it creates a new inner effect subscription without cleaning up the previous one, leading to multiple executions and potential memory leaks.
🐛 Proposed fix - move inner effect outside
$effect(() => {
if (pendingSave) return;
const newRules = rulesFile.rules.map((r: Rule) => ({
...r,
match: Array.isArray(r.match) ? r.match : r.match ? [r.match as unknown as string] : [""],
}));
rules = newRules;
rowIsCustom = newRules.map(
(r) => r.browser !== "" && !installedBrowsers.includes(r.browser)
);
rowProfileIsCustom = newRules.map(
(r) => {
const profile = r.profile ?? "";
if (profile === "") return false;
const profiles = untrack(() => profilesByBrowser[r.browser]);
if (profiles === undefined) return false;
return !profiles.includes(profile);
}
);
// Fetch profiles for browsers not yet loaded
const browsersToFetch = new Set<string>();
for (const r of newRules) {
if (r.browser) browsersToFetch.add(r.browser);
}
untrack(() => {
for (const b of browsersToFetch) {
if (profilesByBrowser[b] === undefined) {
window.finicky.sendMessage({ type: "getBrowserProfiles", browser: b });
}
}
});
-
- // Once profile lists arrive, correct any rowProfileIsCustom entries that were
- // deferred because profilesByBrowser wasn't loaded yet during the initial sync.
- $effect(() => {
- rules.forEach((r, i) => {
- const profile = r.profile ?? "";
- if (profile === "") return;
- const profiles = profilesByBrowser[r.browser];
- if (profiles === undefined) return; // still loading
- rowProfileIsCustom[i] = !profiles.includes(profile);
- });
- });
});
+
+ // Once profile lists arrive, correct any rowProfileIsCustom entries that were
+ // deferred because profilesByBrowser wasn't loaded yet during the initial sync.
+ $effect(() => {
+ rules.forEach((r, i) => {
+ const profile = r.profile ?? "";
+ if (profile === "") return;
+ const profiles = profilesByBrowser[r.browser];
+ if (profiles === undefined) return; // still loading
+ rowProfileIsCustom[i] = !profiles.includes(profile);
+ });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $effect(() => { | |
| if (pendingSave) return; | |
| const newRules = rulesFile.rules.map((r: Rule) => ({ | |
| ...r, | |
| match: Array.isArray(r.match) ? r.match : r.match ? [r.match as unknown as string] : [""], | |
| })); | |
| rules = newRules; | |
| rowIsCustom = newRules.map( | |
| (r) => r.browser !== "" && !installedBrowsers.includes(r.browser) | |
| ); | |
| rowProfileIsCustom = newRules.map( | |
| (r) => { | |
| const profile = r.profile ?? ""; | |
| if (profile === "") return false; | |
| const profiles = untrack(() => profilesByBrowser[r.browser]); | |
| if (profiles === undefined) return false; // not fetched yet — correct later | |
| return !profiles.includes(profile); | |
| } | |
| ); | |
| // Fetch profiles for browsers not yet loaded — untrack to avoid re-triggering on profilesByBrowser changes | |
| const browsersToFetch = new Set<string>(); | |
| for (const r of newRules) { | |
| if (r.browser) browsersToFetch.add(r.browser); | |
| } | |
| untrack(() => { | |
| for (const b of browsersToFetch) { | |
| if (profilesByBrowser[b] === undefined) { | |
| window.finicky.sendMessage({ type: "getBrowserProfiles", browser: b }); | |
| } | |
| } | |
| }); | |
| // Once profile lists arrive, correct any rowProfileIsCustom entries that were | |
| // deferred because profilesByBrowser wasn't loaded yet during the initial sync. | |
| $effect(() => { | |
| rules.forEach((r, i) => { | |
| const profile = r.profile ?? ""; | |
| if (profile === "") return; | |
| const profiles = profilesByBrowser[r.browser]; | |
| if (profiles === undefined) return; // still loading | |
| rowProfileIsCustom[i] = !profiles.includes(profile); | |
| }); | |
| }); | |
| }); | |
| $effect(() => { | |
| if (pendingSave) return; | |
| const newRules = rulesFile.rules.map((r: Rule) => ({ | |
| ...r, | |
| match: Array.isArray(r.match) ? r.match : r.match ? [r.match as unknown as string] : [""], | |
| })); | |
| rules = newRules; | |
| rowIsCustom = newRules.map( | |
| (r) => r.browser !== "" && !installedBrowsers.includes(r.browser) | |
| ); | |
| rowProfileIsCustom = newRules.map( | |
| (r) => { | |
| const profile = r.profile ?? ""; | |
| if (profile === "") return false; | |
| const profiles = untrack(() => profilesByBrowser[r.browser]); | |
| if (profiles === undefined) return false; // not fetched yet — correct later | |
| return !profiles.includes(profile); | |
| } | |
| ); | |
| // Fetch profiles for browsers not yet loaded — untrack to avoid re-triggering on profilesByBrowser changes | |
| const browsersToFetch = new Set<string>(); | |
| for (const r of newRules) { | |
| if (r.browser) browsersToFetch.add(r.browser); | |
| } | |
| untrack(() => { | |
| for (const b of browsersToFetch) { | |
| if (profilesByBrowser[b] === undefined) { | |
| window.finicky.sendMessage({ type: "getBrowserProfiles", browser: b }); | |
| } | |
| } | |
| }); | |
| }); | |
| // Once profile lists arrive, correct any rowProfileIsCustom entries that were | |
| // deferred because profilesByBrowser wasn't loaded yet during the initial sync. | |
| $effect(() => { | |
| rules.forEach((r, i) => { | |
| const profile = r.profile ?? ""; | |
| if (profile === "") return; | |
| const profiles = profilesByBrowser[r.browser]; | |
| if (profiles === undefined) return; // still loading | |
| rowProfileIsCustom[i] = !profiles.includes(profile); | |
| }); | |
| }); |
🤖 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 146 - 189, The inner
$effect that updates rowProfileIsCustom is currently declared inside the outer
$effect (the block manipulating rules, rowIsCustom and fetching profiles) which
causes duplicate effect subscriptions on each outer re-run; extract that inner
$effect into its own top-level $effect (so it's not nested), keep its logic
referencing rules, profilesByBrowser and rowProfileIsCustom unchanged, and
ensure the outer $effect only performs rules normalization and the untrack fetch
loop (including proper closure of the untrack call) so effects are registered
once and no duplicate subscriptions are created.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/finicky/src/main.go (3)
143-167:⚠️ Potential issue | 🔴 CriticalRace condition on shared
vmvariable remains unaddressed.
SaveRulesHandlerwrites to the globalvm(line 161) without synchronization. Meanwhile, the main event loop readsvmat line 193 (resolver.ResolveURL),TestURLInternalreads it at line 308 (spawned in a goroutine via line 137), andcheckForUpdatesreads it at line 365 (spawned in goroutines at lines 133, 164, 222, 233). Sincegoja.Runtimeis not goroutine-safe, concurrent access can cause race conditions and state corruption.,
🤖 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 - 167, The handler SaveRulesHandler mutates the shared global vm without synchronization while resolver.ResolveURL, TestURLInternal, and checkForUpdates read it concurrently, causing race conditions; add a synchronization primitive (e.g., a package-level sync.RWMutex named vmMu) and wrap all writes to vm (the assignment in SaveRulesHandler and any other code that replaces vm) with vmMu.Lock()/Unlock() and wrap all reads (resolver.ResolveURL, TestURLInternal, checkForUpdates and any goroutines that call vm methods) with vmMu.RLock()/RUnlock(); ensure that when you call vm methods you hold the appropriate lock for the duration of the call to prevent concurrent access to the goja.Runtime instance.
161-165:⚠️ Potential issue | 🟠 Major
logRequestssetting not applied after rebuilding VM from rules.After setting
vm = newVM, the code updatesshouldKeepRunningand triggerscheckForUpdates(), but doesn't calllogger.SetupFile()with the newLogRequestsvalue. The UI-savedlogRequeststoggle won't take effect until the next 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 161 - 165, After assigning the rebuilt VM (vm = newVM), the new VM's LogRequests setting isn't applied to the logger; call logger.SetupFile(...) with the new value so UI toggled logRequests takes effect immediately. In the block where vm is replaced (after vm = newVM and when vm != nil), read the new config via vm.GetAllConfigOptions().LogRequests and invoke logger.SetupFile with that boolean (and any existing parameters used elsewhere) so the logger reflects the rebuilt VM's LogRequests setting without requiring a restart; ensure this mirrors the initial logger setup behavior used on startup.
449-449:⚠️ Potential issue | 🟡 MinorInconsistent handling of options-only rules files.
The condition here only checks
rf.DefaultBrowser != "" || len(rf.Rules) > 0, butSaveRulesHandlerat line 147 additionally checksrf.Options == nil. A rules file containing only UI settings (e.g.,keepRunning,hideIcon) without any rules or default browser will be ignored here, while the same file would be processed inSaveRulesHandler.🐛 Proposed fix for consistency
- if rf.DefaultBrowser != "" || len(rf.Rules) > 0 { + if rf.DefaultBrowser != "" || len(rf.Rules) > 0 || rf.Options != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/finicky/src/main.go` at line 449, The branch that decides whether to process a parsed rules file only checks rf.DefaultBrowser != "" || len(rf.Rules) > 0, which misses files that contain only UI options; update that condition to also treat rf.Options != nil as a valid payload so option-only files are handled the same way as in SaveRulesHandler (referencing rf.DefaultBrowser, rf.Rules, rf.Options and the SaveRulesHandler behavior) — modify the conditional in the same function to include rf.Options != nil and ensure downstream code that processes the rules/options handles nil vs non-nil Options consistently.
🧹 Nitpick comments (1)
apps/finicky/src/main.go (1)
430-434: Redundant rules loading when no JS config is present.When
currentBundlePath == "", rules are loaded at lines 430-434 (unconditionally) and then loaded again at lines 444-448. Consider reusing the already-loadedrffrom the first load to avoid redundant disk I/O.♻️ Proposed refactor to eliminate double loading
+ var preloadedRules rules.RulesFile + var preloadErr error // Always seed the cached rules from disk so JSON rules are applied // immediately on startup, even when a JS config is also present. - if rf, rulesErr := rules.Load(); rulesErr != nil { + if preloadedRules, preloadErr = rules.Load(); preloadErr != nil { slog.Warn("Failed to pre-load rules cache", "error", rulesErr) } else { - resolver.SetCachedRules(rf) + resolver.SetCachedRules(preloadedRules) } var newVM *config.VM if currentBundlePath != "" { newVM, err = config.New(finickyConfigAPIJS, namespace, currentBundlePath) if err != nil { return nil, fmt.Errorf("failed to setup VM: %v", err) } } else { - rf, rulesErr := rules.Load() - if rulesErr != nil { - slog.Warn("Failed to load rules file", "error", rulesErr) - } else { - resolver.SetCachedRules(rf) - if rf.DefaultBrowser != "" || len(rf.Rules) > 0 { + if preloadErr == nil { + if preloadedRules.DefaultBrowser != "" || len(preloadedRules.Rules) > 0 || preloadedRules.Options != nil { - script, scriptErr := rules.ToJSConfigScript(rf, namespace) + script, scriptErr := rules.ToJSConfigScript(preloadedRules, namespace)Also applies to: 444-448
🤖 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 430 - 434, The code unconditionally calls rules.Load() twice when currentBundlePath == "" leading to redundant disk I/O; modify the logic so the first successful load (rf from the initial rules.Load() block) is reused instead of calling rules.Load() again: preserve rf in a variable in the surrounding scope and, where the later block currently calls rules.Load() again (the second rules.Load() and resolver.SetCachedRules call), check if rf is already non-nil and call resolver.SetCachedRules(rf) directly, only invoking rules.Load() if rf is nil; ensure references to currentBundlePath, rf, rules.Load, and resolver.SetCachedRules are used to locate and update the two loading sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/finicky/src/main.go`:
- Around line 143-167: The handler SaveRulesHandler mutates the shared global vm
without synchronization while resolver.ResolveURL, TestURLInternal, and
checkForUpdates read it concurrently, causing race conditions; add a
synchronization primitive (e.g., a package-level sync.RWMutex named vmMu) and
wrap all writes to vm (the assignment in SaveRulesHandler and any other code
that replaces vm) with vmMu.Lock()/Unlock() and wrap all reads
(resolver.ResolveURL, TestURLInternal, checkForUpdates and any goroutines that
call vm methods) with vmMu.RLock()/RUnlock(); ensure that when you call vm
methods you hold the appropriate lock for the duration of the call to prevent
concurrent access to the goja.Runtime instance.
- Around line 161-165: After assigning the rebuilt VM (vm = newVM), the new VM's
LogRequests setting isn't applied to the logger; call logger.SetupFile(...) with
the new value so UI toggled logRequests takes effect immediately. In the block
where vm is replaced (after vm = newVM and when vm != nil), read the new config
via vm.GetAllConfigOptions().LogRequests and invoke logger.SetupFile with that
boolean (and any existing parameters used elsewhere) so the logger reflects the
rebuilt VM's LogRequests setting without requiring a restart; ensure this
mirrors the initial logger setup behavior used on startup.
- Line 449: The branch that decides whether to process a parsed rules file only
checks rf.DefaultBrowser != "" || len(rf.Rules) > 0, which misses files that
contain only UI options; update that condition to also treat rf.Options != nil
as a valid payload so option-only files are handled the same way as in
SaveRulesHandler (referencing rf.DefaultBrowser, rf.Rules, rf.Options and the
SaveRulesHandler behavior) — modify the conditional in the same function to
include rf.Options != nil and ensure downstream code that processes the
rules/options handles nil vs non-nil Options consistently.
---
Nitpick comments:
In `@apps/finicky/src/main.go`:
- Around line 430-434: The code unconditionally calls rules.Load() twice when
currentBundlePath == "" leading to redundant disk I/O; modify the logic so the
first successful load (rf from the initial rules.Load() block) is reused instead
of calling rules.Load() again: preserve rf in a variable in the surrounding
scope and, where the later block currently calls rules.Load() again (the second
rules.Load() and resolver.SetCachedRules call), check if rf is already non-nil
and call resolver.SetCachedRules(rf) directly, only invoking rules.Load() if rf
is nil; ensure references to currentBundlePath, rf, rules.Load, and
resolver.SetCachedRules are used to locate and update the two loading sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71dab91a-6112-4bfa-b9c8-44c0d8a7e60b
📒 Files selected for processing (4)
apps/finicky/src/main.goapps/finicky/src/rules/rules.gopackages/finicky-ui/src/pages/Rules.sveltepackages/finicky-ui/src/pages/StartPage.svelte
✅ Files skipped from review due to trivial changes (1)
- packages/finicky-ui/src/pages/Rules.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/finicky/src/rules/rules.go
- packages/finicky-ui/src/pages/StartPage.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #380
Summary by CodeRabbit
New Features
Style
Tests
Chores