Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces the native WebView message bridge with an authenticated HTTP API and SSE event stream, and rewrites the finicky-ui package from Svelte to React. It also updates shared config/types, log and rules handling, and the UI pages and components that consume the new API. ChangesNative app: SSE/API server and WebView bridge removal
Estimated code review effort: 4 (Complex) | ~75 minutes finicky-ui: Svelte to React rewrite
Estimated code review effort: 4 (Complex) | ~80 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/finicky/src/main.go (1)
468-485: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid dereferencing stale or nil
configInfo.
csis treated as nullable, butlastConfigPayloadunconditionally readsconfigInfo. On the first nil state this can panic; on reload it can reuse stale config metadata.Proposed fix
- cs := newVM.GetConfigState() + ci := ConfigInfo{} + cs := newVM.GetConfigState() if cs != nil { - configInfo = &ConfigInfo{ + ci = ConfigInfo{ Handlers: cs.Handlers, Rewrites: cs.Rewrites, DefaultBrowser: cs.DefaultBrowser, ConfigPath: configPath, } } + configInfo = &ci @@ lastConfigPayload = map[string]interface{}{ - "handlers": configInfo.Handlers, - "rewrites": configInfo.Rewrites, - "defaultBrowser": configInfo.DefaultBrowser, - "configPath": util.ShortenPath(configInfo.ConfigPath), + "handlers": ci.Handlers, + "rewrites": ci.Rewrites, + "defaultBrowser": ci.DefaultBrowser, + "configPath": util.ShortenPath(ci.ConfigPath),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/main.go` around lines 468 - 485, The `lastConfigPayload` block is unconditionally dereferencing `configInfo` even though `GetConfigState()` can return nil, which can panic or reuse stale metadata. Update the `main.go` config-building flow so `configInfo` is only read when it has been freshly populated from `newVM.GetConfigState()`, and otherwise initialize/reset `lastConfigPayload` safely from `cs` or skip the payload build until valid config data exists. Use the `GetConfigState`, `ConfigInfo`, and `lastConfigPayload` symbols to keep the fix localized and ensure reloads don’t retain old values.
🧹 Nitpick comments (9)
apps/finicky/src/rules/rules.go (1)
39-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect fix; minor duplication in
RuleAlias.Logic is correct — normalizing nil to
[]string{}and always marshalingmatchas an array matches the stated goal and is consistent withgetRulesData/writeJSONdownstream inserver.go. One nit:RuleAliasmanually re-declares all ofRule's fields/tags instead of using the standardtype Alias Ruleidiom, which avoids duplication and the risk of silently dropping a future field ifRulechanges.♻️ Optional refactor using the alias-type idiom
func (r Rule) MarshalJSON() ([]byte, error) { - type RuleAlias struct { - Match []string `json:"match"` - Browser string `json:"browser"` - Profile string `json:"profile,omitempty"` - } + type RuleAlias Rule match := r.Match if match == nil { match = []string{} } - return json.Marshal(RuleAlias{Match: match, Browser: r.Browser, Profile: r.Profile}) + alias := RuleAlias(r) + alias.Match = match + return json.Marshal(alias) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/finicky/src/rules/rules.go` around lines 39 - 51, Refactor the MarshalJSON implementation on Rule to avoid duplicating Rule’s fields in the local RuleAlias struct. Keep the existing nil-to-empty-slice normalization for Match, but switch to the standard alias-type pattern in Rule.MarshalJSON so the serialization stays aligned automatically with Rule if fields are added or tags change later.packages/finicky-ui/src/components/TabBar.tsx (1)
32-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated tab-rendering logic between top and bottom tabs.
The
Link/className-building block is repeated almost verbatim fortabsandbottomTabs, differing only by the extratabSmallclass and the error badge. Consider extracting a small shared render helper (e.g.renderTab(path, label, Icon, extraClass?, badge?)) to avoid the duplication.Also applies to: 44-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/components/TabBar.tsx` around lines 32 - 41, The tab rendering in TabBar is duplicated between the main tabs and bottomTabs, so extract the shared Link/className/icon/label markup into a small helper inside TabBar.tsx, such as a renderTab function that takes path, label, Icon, and optional extra class or badge content. Update both tabs.map and bottomTabs.map to call the helper so the only differences remain the tabSmall styling and the error badge.packages/finicky-ui/src/components/icons/About.tsx (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate SVG markup with
InfoIcon.
AboutIcon(packages/finicky-ui/src/components/icons/About.tsx) andInfoIcon(packages/finicky-ui/src/components/icons/Info.tsx) render identical SVG paths (circle + vertical line + dot), differing only inwidth/height. Consider a single parameterizedInfoCircleIcon({ size = 24 })to avoid duplicated markup across icon components.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/components/icons/About.tsx` around lines 1 - 9, `AboutIcon` duplicates the same SVG markup as `InfoIcon`, so replace the repeated icon implementation with a shared parameterized component such as `InfoCircleIcon` and have `AboutIcon` delegate to it with the appropriate size. Update the icon components in the `AboutIcon`/`InfoIcon` pair so the circle, line, and dot paths live in one place and only the size prop varies.packages/finicky-ui/src/components/LogContent.tsx (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUsing array index as React key for log entries.
If
messageBufferis ever trimmed from the front (common for capped log buffers), index-based keys cause React to misassociate DOM nodes with their previous entries during reconciliation. Consider keying byentry.timeplus index, or a stable id if the buffer source provides one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/components/LogContent.tsx` around lines 21 - 22, The LogContent log entry list is using the array index as the React key, which can cause reconciliation issues when messageBuffer is trimmed or reordered. Update the key in the LogContent component’s map over entries to use a stable identifier from each entry, such as entry.time combined with a fallback index or another unique id if available, so each li remains consistently associated with the same log item.packages/finicky-ui/src/lib/appStore.ts (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getNumErrorsappears unused — error-count logic is duplicated at call sites instead.Per cross-file graph context,
StartPage.tsxcomputesmessageBuffer.filter((m) => m.level.toLowerCase() === "error").lengthinline rather than calling this store method. That's the same expression duplicated in two places; if the error-detection rule ever changes (e.g., case-insensitivity tweak, additional levels), one of the two will silently drift out of sync.Consider having consumers call
appStore.getNumErrors()instead of re-implementing the filter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/lib/appStore.ts` around lines 35 - 36, `getNumErrors` in `appStore` is duplicated at call sites instead of being reused. Update consumers such as `StartPage.tsx` to call `appStore.getNumErrors()` rather than re-implementing the `messageBuffer.filter(...).length` logic inline, so the error-count rule stays centralized and consistent.packages/finicky-ui/vite.config.ts (1)
7-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
assetsDir: "assets"is effectively a no-op given the custom filename patterns.When
assetFileNames/chunkFileNames/entryFileNamesare fully overridden without anassetsDir-relative path segment, Rollup/Vite uses the patterns as-is instead of nesting underbuild.assetsDir(that default-nesting behavior only applies to the unmodified default pattern). Confirmed by Vite issue reports: When rollupOptions.output is an array, it defaults to '/[name]. the assetsDir prefix is only used by Vite's own default pattern, not by custom overrides like the ones here. So all entries/chunks/assets will land flat in the output root, makingassetsDir: "assets"dead configuration. Not breaking (Vite still rewrites the generatedindex.htmlreferences correctly), but confusing/misleading.♻️ Suggested cleanup
build: { - assetsDir: "assets", rollupOptions: { output: { assetFileNames: "[name][extname]", chunkFileNames: "[name].js", entryFileNames: "[name].js", }, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/vite.config.ts` around lines 7 - 16, Remove the misleading build.assetsDir setting in vite.config.ts since the custom rollupOptions.output patterns in the Vite config (assetFileNames, chunkFileNames, entryFileNames) already control the output paths and ignore the default assetsDir nesting. Update the build configuration to keep only the filename pattern overrides, or if a nested assets folder is desired, incorporate that path directly into the rollupOptions.output patterns instead of relying on assetsDir.packages/finicky-ui/src/lib/useSSE.ts (1)
7-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo
errorlistener on the EventSource.The native browser will auto-retry on a dropped connection, so this isn't broken, but there's currently zero visibility if the SSE stream to the local API server fails to (re)connect (e.g., app backend restarts) — logs/config/update events would just silently stop updating with no indication to the user or in the console.
useEffect(() => { const es = new EventSource(api.eventsUrl()); + + es.addEventListener("error", (e) => { + console.warn("SSE connection error", e); + }); es.addEventListener("log", (e) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/lib/useSSE.ts` around lines 7 - 30, The EventSource setup in useSSE currently handles only log, config, and updateInfo events, so connection failures/reconnect problems are completely silent. Add an error listener on the EventSource created in useEffect that reports SSE connection issues to the console or appStore, and keep the existing cleanup with es.close(). Use the existing symbols useSSE, appStore, and api.eventsUrl() so the fix stays localized.packages/finicky-ui/src/pages/TestUrl.module.css (1)
123-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeprecated
word-break: break-wordvalue.Stylelint flags
break-wordas deprecated forword-break. Bothword-breakandoverflow-wrapare already set here;overflow-wrap: break-word(line 127) covers the wrapping behavior, so the deprecatedword-break: break-wordcan be dropped or replaced.Proposed fix
.resultValue { color: var(--text-primary); - word-break: break-word; overflow-wrap: break-word; min-width: 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/pages/TestUrl.module.css` around lines 123 - 129, The .resultValue rule in TestUrl.module.css uses the deprecated word-break: break-word value. Update the .resultValue styles by removing that declaration or replacing it with a non-deprecated word-break value, since overflow-wrap: break-word already handles the wrapping behavior; keep the change localized to the .resultValue selector.Source: Linters/SAST tools
packages/finicky-ui/src/pages/TestUrl.tsx (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIneffective lint-suppression comment.
// fallow-ignore-next-line complexityisn't a recognized ESLint directive (typo for "follow"/wrong format), so it has no effect and is just dead text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/pages/TestUrl.tsx` at line 41, The lint suppression comment in TestUrl is ineffective because the directive name is misspelled and not recognized, so it does nothing. Replace the bad comment near the affected complexity check with the correct ESLint directive syntax that matches the intended rule, using the nearby TestUrl component/code block as the reference point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/finicky/src/main.go`:
- Line 65: The HTTP handlers and background actions are accessing shared app
state concurrently, which can race on vm, updateInfo, and lastConfigPayload
during config reloads, rule saves, and update checks. Serialize all access in
main.go by routing state mutations and reads through a single app-state
goroutine or protecting the shared globals with a mutex; make sure handlers like
the ones around the VM resolver, config update, and update status return copied
payloads instead of sharing mutable maps or structs directly.
- Around line 164-165: The API server startup path in main.go currently logs the
failure and continues, which leaves GetAPIPort() unusable for the settings UI.
Update the startup flow around window.StartAPIServer() so that a non-nil error
from that call stops execution immediately instead of falling through, and keep
the failure handling localized to this startup block so the React API URL is
only injected when the server has started successfully.
In `@apps/finicky/src/window/server.go`:
- Around line 155-158: The POST /api/rules flow in server.go returns success
before SaveRulesHandler has finished applying the new rules, which can leave vm
stale for follow-up requests. Update the save path around
SaveRulesHandler/getRulesData so the handler is completed and the new rules are
applied before writing the success response, or change the response to an
explicit pending state until the rebuild is done. Use the SaveRulesHandler hook
and the rule reload path in server.go/main.go as the key places to adjust.
- Around line 100-104: The current CORS setup in corsMiddleware exposes the
loopback API to any origin, so tighten access before setting headers. Add a
per-process secret and origin validation in the server flow around
corsMiddleware and the REST/SSE handlers, requiring the secret on requests
before allowing reads or writes. Update the relevant request handling symbols in
server.go so only trusted origins are reflected and unauthorized localhost calls
are rejected.
In `@packages/finicky-ui/src/App.tsx`:
- Around line 36-50: The initial data bootstrap in App.tsx is swallowing
failures in the load() effect, so the UI stays stuck in default state if
api.initialData() rejects. Update the load flow to handle errors explicitly in
the App component: capture the rejection, surface a visible error/loading state
or notification, and add a retry path (for example by retrying in the same
useEffect or exposing a refresh action) so users can recover when initialData()
is temporarily unavailable.
In `@packages/finicky-ui/src/components/BrowserProfileSelector.tsx`:
- Around line 144-155: The ProfileField render gate in BrowserProfileSelector
should not depend only on profileOptions.length, because that hides an
already-saved custom profile when the browser has no loaded profiles. Update the
conditional around ProfileField to also render when custom.profile is true (and
the browser/value.profile exists), so existing custom profile values remain
visible and editable. Keep the change localized to the BrowserProfileSelector
component and preserve the existing handleProfileSelect, onInput, onClear, and
onBlur behavior.
In `@packages/finicky-ui/src/components/Footer.tsx`:
- Line 29: The path text in Footer is using plain double-quoted strings, so
`${config.configPath}` and `${rulesFile.path}` are rendered literally instead of
interpolated. Update the `showPathToast` calls in `Footer.tsx` to use template
literals in the relevant click handlers so the `config.configPath` and
`rulesFile.path` values are inserted correctly.
In `@packages/finicky-ui/src/components/OptionRow.tsx`:
- Around line 44-52: The locked branch in OptionRow currently nests Inner (which
renders a label/input checkbox) inside a real button, creating invalid
interactive nesting. Update OptionRow’s locked rendering so the wrapper is
non-interactive or otherwise restructure Inner to avoid emitting the label/input
wrapper in the locked state, and keep onLockedClick behavior accessible without
using a button around the checkbox markup.
- Around line 16-39: The checkbox in Inner lacks an accessible name because the
visible option text is outside the toggle label. Fix this by giving the input in
OptionRow/Inner an accessible label, such as using the label prop for
aria-label, or by associating the visible optionLabel text with the checkbox so
assistive tech can announce it. Keep the locked and onChange behavior unchanged.
In `@packages/finicky-ui/src/lib/toast.ts`:
- Around line 42-58: The repeated-toast update path in show() is not refreshing
the extra field, so a later call with the same message/type but different
details leaves stale text in the UI. Update the existing-toast branch in show()
so the mapped toast object also replaces extra alongside duration, timeoutId,
and key, ensuring the latest payload is rendered when matching on message and
type.
In `@packages/finicky-ui/src/lib/useLogControls.ts`:
- Around line 29-40: The formatting work in copyLogs is happening before the
try/catch, so a bad log entry from formatEntry can throw outside the error
handler. Move the messageBuffer filtering and .map(formatEntry) logic inside the
try block (or otherwise wrap it in the same guard) so any RangeError from
Date.toISOString is caught and toast.error is shown. Keep the fix localized to
copyLogs and the formatEntry path it uses.
In `@packages/finicky-ui/src/lib/useRulesSave.ts`:
- Around line 10-20: The debounced save in useRulesSave clears isPending too
early, before api.saveRules finishes, creating a race with consumers like
Rules.tsx that rely on the flag. Move the isPending.current = false assignment
so it only happens after the save attempt has fully completed, ideally in a
finally block around the api.saveRules call, while keeping the existing
appStore.update flow in the useRulesSave callback.
- Around line 13-17: The save flow in useRulesSave is dropping fields from the
existing RulesFile because api.saveRules is called with only defaultBrowser,
defaultProfile, and rules. Update the save payload to preserve the current
rulesFile object (including options and any other existing properties) while
replacing only the rules data, and adjust the updated as any handling so the
returned shape matches the RulesFile model instead of masking the mismatch.
In `@packages/finicky-ui/src/pages/Rules.tsx`:
- Around line 52-65: The rule reordering flow in useDragSort and the draggable
rows in Rules.tsx only support mouse drag-and-drop, so add a keyboard-accessible
reorder path as well. Update the Rules page components that render the
reorderable list so users can move items with buttons or arrow-key handling, or
replace the custom drag logic in useDragSort/onDragStart/onDragOver/onDragEnd
with an accessible DnD solution such as `@dnd-kit/core` that includes keyboard
sensors. While touching the list rendering, replace key={i} usage with a stable
item identifier so reordered/editable rows keep consistent identity.
- Line 182: The save payload from Rules.tsx is dropping existing options, so
update the rules save flow to preserve the full RulesFile shape. In
useRulesSave, ensure the object built from the rules state includes options
alongside defaultBrowser, defaultProfile, and rules before it reaches
handleSaveRulesHTTP. Use the existing RulesFile/options fields so edits on the
Rules page do not clear keepRunning, hideIcon, logRequests, or checkForUpdates.
- Around line 193-199: The save guard in Rules.tsx is being cleared too early,
allowing the Rules effect to re-run before api.saveRules settles and overwrite
in-flight edits. Update the save flow around the relevant save handler and
isPending ref so the flag remains set for the full async round-trip, then reset
it in a finally block after api.saveRules completes; this keeps the useEffect
that depends on rulesFile.rules and installedBrowsers blocked until the save is
fully settled.
In `@packages/finicky-ui/src/pages/StartPage.tsx`:
- Around line 100-135: The StartPage resync effect is resetting local state from
rulesFile/config even while there are unsaved edits, which can overwrite pending
user changes before saveDebounced flushes. Add a pending/dirty guard in
StartPage’s useEffect (similar to the one used in Rules.tsx) so it skips the
reset while edits are in progress, and only resyncs when there are no pending
local changes.
In `@packages/finicky-ui/src/pages/TestUrl.tsx`:
- Around line 67-89: The loading state in TestUrl.tsx can be set after the
request has already finished because the loadingTimer runs in parallel with the
debounced request started in useEffect. Update the effect so loading only turns
on when the actual api.testUrl(normalizeUrl(testUrl)) call begins inside the
debounce callback, and remove the separate pre-request timer; keep
setLoading(false) in the finally path and in the invalid-URL early return so
TestUrl stays accurate.
In `@packages/finicky-ui/src/types.ts`:
- Around line 45-50: Update the TestUrlResult type so openInBackground is
optional instead of required, since TestUrl.tsx already defensively checks
typeof result.openInBackground before using it. Locate the TestUrlResult
interface in types.ts and change the field declaration to match the runtime
shape; keep the other properties unchanged so the type accurately reflects
consumer usage.
In `@packages/finicky-ui/tsconfig.app.json`:
- Around line 2-21: Add composite to the referenced TypeScript config so the
build mode works: both tsconfig.app.json and tsconfig.node.json need composite
enabled because tsconfig.json is being built with tsc -b and references them.
Update the compilerOptions in the referenced configs to set composite: true,
keeping the existing build setup in packages/finicky-ui compatible with
tsconfig.json and the tsc -b command.
---
Outside diff comments:
In `@apps/finicky/src/main.go`:
- Around line 468-485: The `lastConfigPayload` block is unconditionally
dereferencing `configInfo` even though `GetConfigState()` can return nil, which
can panic or reuse stale metadata. Update the `main.go` config-building flow so
`configInfo` is only read when it has been freshly populated from
`newVM.GetConfigState()`, and otherwise initialize/reset `lastConfigPayload`
safely from `cs` or skip the payload build until valid config data exists. Use
the `GetConfigState`, `ConfigInfo`, and `lastConfigPayload` symbols to keep the
fix localized and ensure reloads don’t retain old values.
---
Nitpick comments:
In `@apps/finicky/src/rules/rules.go`:
- Around line 39-51: Refactor the MarshalJSON implementation on Rule to avoid
duplicating Rule’s fields in the local RuleAlias struct. Keep the existing
nil-to-empty-slice normalization for Match, but switch to the standard
alias-type pattern in Rule.MarshalJSON so the serialization stays aligned
automatically with Rule if fields are added or tags change later.
In `@packages/finicky-ui/src/components/icons/About.tsx`:
- Around line 1-9: `AboutIcon` duplicates the same SVG markup as `InfoIcon`, so
replace the repeated icon implementation with a shared parameterized component
such as `InfoCircleIcon` and have `AboutIcon` delegate to it with the
appropriate size. Update the icon components in the `AboutIcon`/`InfoIcon` pair
so the circle, line, and dot paths live in one place and only the size prop
varies.
In `@packages/finicky-ui/src/components/LogContent.tsx`:
- Around line 21-22: The LogContent log entry list is using the array index as
the React key, which can cause reconciliation issues when messageBuffer is
trimmed or reordered. Update the key in the LogContent component’s map over
entries to use a stable identifier from each entry, such as entry.time combined
with a fallback index or another unique id if available, so each li remains
consistently associated with the same log item.
In `@packages/finicky-ui/src/components/TabBar.tsx`:
- Around line 32-41: The tab rendering in TabBar is duplicated between the main
tabs and bottomTabs, so extract the shared Link/className/icon/label markup into
a small helper inside TabBar.tsx, such as a renderTab function that takes path,
label, Icon, and optional extra class or badge content. Update both tabs.map and
bottomTabs.map to call the helper so the only differences remain the tabSmall
styling and the error badge.
In `@packages/finicky-ui/src/lib/appStore.ts`:
- Around line 35-36: `getNumErrors` in `appStore` is duplicated at call sites
instead of being reused. Update consumers such as `StartPage.tsx` to call
`appStore.getNumErrors()` rather than re-implementing the
`messageBuffer.filter(...).length` logic inline, so the error-count rule stays
centralized and consistent.
In `@packages/finicky-ui/src/lib/useSSE.ts`:
- Around line 7-30: The EventSource setup in useSSE currently handles only log,
config, and updateInfo events, so connection failures/reconnect problems are
completely silent. Add an error listener on the EventSource created in useEffect
that reports SSE connection issues to the console or appStore, and keep the
existing cleanup with es.close(). Use the existing symbols useSSE, appStore, and
api.eventsUrl() so the fix stays localized.
In `@packages/finicky-ui/src/pages/TestUrl.module.css`:
- Around line 123-129: The .resultValue rule in TestUrl.module.css uses the
deprecated word-break: break-word value. Update the .resultValue styles by
removing that declaration or replacing it with a non-deprecated word-break
value, since overflow-wrap: break-word already handles the wrapping behavior;
keep the change localized to the .resultValue selector.
In `@packages/finicky-ui/src/pages/TestUrl.tsx`:
- Line 41: The lint suppression comment in TestUrl is ineffective because the
directive name is misspelled and not recognized, so it does nothing. Replace the
bad comment near the affected complexity check with the correct ESLint directive
syntax that matches the intended rule, using the nearby TestUrl component/code
block as the reference point.
In `@packages/finicky-ui/vite.config.ts`:
- Around line 7-16: Remove the misleading build.assetsDir setting in
vite.config.ts since the custom rollupOptions.output patterns in the Vite config
(assetFileNames, chunkFileNames, entryFileNames) already control the output
paths and ignore the default assetsDir nesting. Update the build configuration
to keep only the filename pattern overrides, or if a nested assets folder is
desired, incorporate that path directly into the rollupOptions.output patterns
instead of relying on assetsDir.
🪄 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: 61c7ff13-91b2-41ac-a177-6e62fc1b565f
⛔ Files ignored due to path filters (1)
packages/finicky-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (91)
.gitignoreapps/finicky/src/logger/logger.goapps/finicky/src/main.goapps/finicky/src/rules/rules.goapps/finicky/src/window/server.goapps/finicky/src/window/window.goapps/finicky/src/window/window.happs/finicky/src/window/window.mpackages/finicky-ui/index.htmlpackages/finicky-ui/package.jsonpackages/finicky-ui/src/App.module.csspackages/finicky-ui/src/App.sveltepackages/finicky-ui/src/App.tsxpackages/finicky-ui/src/components/BrowserProfileSelector.module.csspackages/finicky-ui/src/components/BrowserProfileSelector.sveltepackages/finicky-ui/src/components/BrowserProfileSelector.tsxpackages/finicky-ui/src/components/Footer.module.csspackages/finicky-ui/src/components/Footer.tsxpackages/finicky-ui/src/components/LogContent.module.csspackages/finicky-ui/src/components/LogContent.sveltepackages/finicky-ui/src/components/LogContent.tsxpackages/finicky-ui/src/components/OptionRow.module.csspackages/finicky-ui/src/components/OptionRow.sveltepackages/finicky-ui/src/components/OptionRow.tsxpackages/finicky-ui/src/components/PageContainer.module.csspackages/finicky-ui/src/components/PageContainer.sveltepackages/finicky-ui/src/components/PageContainer.tsxpackages/finicky-ui/src/components/TabBar.module.csspackages/finicky-ui/src/components/TabBar.sveltepackages/finicky-ui/src/components/TabBar.tsxpackages/finicky-ui/src/components/ToastContainer.module.csspackages/finicky-ui/src/components/ToastContainer.sveltepackages/finicky-ui/src/components/ToastContainer.tsxpackages/finicky-ui/src/components/Toggle.sveltepackages/finicky-ui/src/components/Tooltip.module.csspackages/finicky-ui/src/components/Tooltip.sveltepackages/finicky-ui/src/components/Tooltip.tsxpackages/finicky-ui/src/components/icons/About.sveltepackages/finicky-ui/src/components/icons/About.tsxpackages/finicky-ui/src/components/icons/External.sveltepackages/finicky-ui/src/components/icons/External.tsxpackages/finicky-ui/src/components/icons/Info.sveltepackages/finicky-ui/src/components/icons/Info.tsxpackages/finicky-ui/src/components/icons/Link.sveltepackages/finicky-ui/src/components/icons/Link.tsxpackages/finicky-ui/src/components/icons/Lock.sveltepackages/finicky-ui/src/components/icons/Lock.tsxpackages/finicky-ui/src/components/icons/Logs.sveltepackages/finicky-ui/src/components/icons/Logs.tsxpackages/finicky-ui/src/components/icons/Preferences.sveltepackages/finicky-ui/src/components/icons/Preferences.tsxpackages/finicky-ui/src/components/icons/Rules.sveltepackages/finicky-ui/src/components/icons/Rules.tsxpackages/finicky-ui/src/components/icons/Spinner.sveltepackages/finicky-ui/src/components/icons/Spinner.tsxpackages/finicky-ui/src/components/icons/Test.sveltepackages/finicky-ui/src/components/icons/Test.tsxpackages/finicky-ui/src/components/icons/Warning.sveltepackages/finicky-ui/src/components/icons/Warning.tsxpackages/finicky-ui/src/components/icons/X.sveltepackages/finicky-ui/src/components/icons/X.tsxpackages/finicky-ui/src/lib/api.tspackages/finicky-ui/src/lib/appStore.tspackages/finicky-ui/src/lib/testUrlStore.tspackages/finicky-ui/src/lib/toast.tspackages/finicky-ui/src/lib/useDebouncedCallback.tspackages/finicky-ui/src/lib/useLogControls.tspackages/finicky-ui/src/lib/useRulesSave.tspackages/finicky-ui/src/lib/useSSE.tspackages/finicky-ui/src/main.tspackages/finicky-ui/src/main.tsxpackages/finicky-ui/src/pages/About.module.csspackages/finicky-ui/src/pages/About.sveltepackages/finicky-ui/src/pages/About.tsxpackages/finicky-ui/src/pages/LogViewer.module.csspackages/finicky-ui/src/pages/LogViewer.sveltepackages/finicky-ui/src/pages/LogViewer.tsxpackages/finicky-ui/src/pages/Rules.module.csspackages/finicky-ui/src/pages/Rules.sveltepackages/finicky-ui/src/pages/Rules.tsxpackages/finicky-ui/src/pages/StartPage.module.csspackages/finicky-ui/src/pages/StartPage.sveltepackages/finicky-ui/src/pages/StartPage.tsxpackages/finicky-ui/src/pages/TestUrl.module.csspackages/finicky-ui/src/pages/TestUrl.sveltepackages/finicky-ui/src/pages/TestUrl.tsxpackages/finicky-ui/src/types.tspackages/finicky-ui/src/vite-env.d.tspackages/finicky-ui/svelte.config.jspackages/finicky-ui/tsconfig.app.jsonpackages/finicky-ui/vite.config.ts
💤 Files with no reviewable changes (29)
- packages/finicky-ui/svelte.config.js
- packages/finicky-ui/src/components/icons/About.svelte
- packages/finicky-ui/src/main.ts
- packages/finicky-ui/src/components/icons/Warning.svelte
- packages/finicky-ui/src/components/icons/Link.svelte
- packages/finicky-ui/src/components/icons/X.svelte
- packages/finicky-ui/src/vite-env.d.ts
- packages/finicky-ui/src/components/OptionRow.svelte
- packages/finicky-ui/src/components/icons/Logs.svelte
- packages/finicky-ui/src/components/TabBar.svelte
- packages/finicky-ui/src/components/LogContent.svelte
- packages/finicky-ui/src/components/icons/External.svelte
- packages/finicky-ui/src/pages/Rules.svelte
- packages/finicky-ui/src/components/ToastContainer.svelte
- packages/finicky-ui/src/components/icons/Rules.svelte
- packages/finicky-ui/src/components/icons/Info.svelte
- packages/finicky-ui/src/components/PageContainer.svelte
- packages/finicky-ui/src/components/icons/Preferences.svelte
- packages/finicky-ui/src/pages/LogViewer.svelte
- packages/finicky-ui/src/components/icons/Test.svelte
- packages/finicky-ui/src/pages/TestUrl.svelte
- packages/finicky-ui/src/pages/About.svelte
- packages/finicky-ui/src/lib/testUrlStore.ts
- packages/finicky-ui/src/components/icons/Spinner.svelte
- packages/finicky-ui/src/components/Tooltip.svelte
- packages/finicky-ui/src/App.svelte
- packages/finicky-ui/src/pages/StartPage.svelte
- packages/finicky-ui/src/components/icons/Lock.svelte
- packages/finicky-ui/src/components/BrowserProfileSelector.svelte
Comment on lines
2
to
+21
| "compilerOptions": { | ||
| "target": "ESNext", | ||
| "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.app.tsbuildinfo", | ||
| "target": "ES2020", | ||
| "useDefineForClassFields": true, | ||
| "lib": ["ES2020", "DOM", "DOM.Iterable"], | ||
| "module": "ESNext", | ||
| "resolveJsonModule": true, | ||
| /** | ||
| * Typecheck JS in `.svelte` and `.js` files by default. | ||
| * Disable checkJs if you'd like to use dynamic types in JS. | ||
| * Note that setting allowJs false does not prevent the use | ||
| * of JS in `.svelte` files. | ||
| */ | ||
| "allowJs": true, | ||
| "checkJs": true, | ||
| "skipLibCheck": true, | ||
| "moduleResolution": "bundler", | ||
| "allowImportingTsExtensions": true, | ||
| "isolatedModules": true, | ||
| "moduleDetection": "force" | ||
| "moduleDetection": "force", | ||
| "noEmit": true, | ||
| "jsx": "react-jsx", | ||
| "strict": true, | ||
| "noUnusedLocals": true, | ||
| "noUnusedParameters": true, | ||
| "noFallthroughCasesInSwitch": true, | ||
| "resolveJsonModule": true | ||
| }, | ||
| "include": ["src/**/*.ts", "src/**/*.js", "src/**/*.svelte"] | ||
| "include": ["src"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect root tsconfig.json and tsconfig.node.json for references/composite setup
fd tsconfig.json --full-path packages/finicky-ui | xargs -I{} sh -c 'echo "== {} =="; cat -n {}'
fd tsconfig.node.json --full-path packages/finicky-ui | xargs -I{} sh -c 'echo "== {} =="; cat -n {}'Repository: johnste/finicky
Length of output: 1167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/finicky-ui/package.json =="
cat -n packages/finicky-ui/package.json
echo
echo "== packages/finicky-ui/tsconfig.app.json =="
cat -n packages/finicky-ui/tsconfig.app.json
echo
echo "== TypeScript reference docs snippet from installed global docs? =="
python3 - <<'PY'
import os, glob
cands = glob.glob('/usr/local/lib/node_modules/typescript/lib/*.d.ts')
print("typescript lib files found:", len(cands))
PYRepository: johnste/finicky
Length of output: 1843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('packages/finicky-ui/package.json')
print(p.exists())
PYRepository: johnste/finicky
Length of output: 158
Add composite: true to both referenced TS configs
packages/finicky-ui/package.json now runs tsc -b, and packages/finicky-ui/tsconfig.json references both tsconfig.app.json and tsconfig.node.json. Both referenced configs need composite: true, or tsc -b will fail with TS6306.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/finicky-ui/tsconfig.app.json` around lines 2 - 21, Add composite to
the referenced TypeScript config so the build mode works: both tsconfig.app.json
and tsconfig.node.json need composite enabled because tsconfig.json is being
built with tsc -b and references them. Update the compilerOptions in the
referenced configs to set composite: true, keeping the existing build setup in
packages/finicky-ui compatible with tsconfig.json and the tsc -b command.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/finicky/src/main.go`:
- Around line 565-572: The first nil config-state publish can still leave
publishedCI nil, so the payload construction in
publishConfigInfo/publishConfigInfo(nil) needs a nil guard before dereferencing
it. Update the logic around publishConfigInfo and the payload map in main.go so
that if publishConfigInfo(ci) returns nil you skip publishing or fall back
safely before accessing publishedCI. Use the publishConfigInfo symbol and the
payload assembly block to locate the fix.
- Around line 324-336: The VM reload path in main.go is publishing a nil value
before checking setupVM errors, which causes the app to lose the last valid VM
on config reload failure. Update the reload flow around setupVM, setVM, and
handleRuntimeError so the previous good VM remains active when setupErr is
non-nil, and only replace the stored VM after a successful setup; keep the
existing error handling and status updates tied to the failure case.
- Around line 256-259: The rules VM reset path in getVM/setVM handling leaves
lifecycle state stale when rf.DefaultBrowser, rf.Rules, and rf.Options are
cleared. Update the clearing branch in main.go so that when the VM is set to
nil, shouldKeepRunning is also reset to its default nil-VM behavior, rather than
preserving the previous value. Use the getVM(), setVM(nil), and
shouldKeepRunning symbols to locate and update the reset logic.
In `@packages/finicky-ui/src/lib/api.ts`:
- Around line 20-30: The API token handling in API_TOKEN is persisting a
per-process bearer secret into localStorage via the devApiToken key. Update this
flow so the token is taken only from the current query parameter or the injected
__FINICKY_API_TOKEN__ value, and remove the persistent fallback/storage path in
api.ts. If you keep any developer convenience for local testing, gate it so it
is not used in shipped code and does not write the token to persistent storage.
🪄 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: 643334b4-7712-41e5-a7ac-b30cc6d39f7d
📒 Files selected for processing (6)
apps/finicky/src/main.goapps/finicky/src/window/server.goapps/finicky/src/window/window.goapps/finicky/src/window/window.happs/finicky/src/window/window.mpackages/finicky-ui/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/finicky/src/window/window.go
- apps/finicky/src/window/window.m
Comment on lines
+20
to
+30
| const API_TOKEN: string = (() => { | ||
| const token = new URLSearchParams(window.location.search).get("apiToken"); | ||
| if (token) { | ||
| localStorage.setItem("devApiToken", token); | ||
| return token; | ||
| } | ||
| return ( | ||
| (window as any).__FINICKY_API_TOKEN__ ?? | ||
| localStorage.getItem("devApiToken") ?? | ||
| "" | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the per-process API token out of persistent storage.
Persisting apiToken as devApiToken leaves the loopback API bearer secret readable by any script for this WebView origin and on disk after the process that generated it. Use only the injected token/current query param, or gate any dev-only convenience so it is not shipped persistently.
Suggested change
const API_TOKEN: string = (() => {
const token = new URLSearchParams(window.location.search).get("apiToken");
if (token) {
- localStorage.setItem("devApiToken", token);
return token;
}
return (
(window as any).__FINICKY_API_TOKEN__ ??
- localStorage.getItem("devApiToken") ??
""
);
})();📝 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
| const API_TOKEN: string = (() => { | |
| const token = new URLSearchParams(window.location.search).get("apiToken"); | |
| if (token) { | |
| localStorage.setItem("devApiToken", token); | |
| return token; | |
| } | |
| return ( | |
| (window as any).__FINICKY_API_TOKEN__ ?? | |
| localStorage.getItem("devApiToken") ?? | |
| "" | |
| ); | |
| const API_TOKEN: string = (() => { | |
| const token = new URLSearchParams(window.location.search).get("apiToken"); | |
| if (token) { | |
| return token; | |
| } | |
| return ( | |
| (window as any).__FINICKY_API_TOKEN__ ?? | |
| "" | |
| ); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 22-22: Do not store sensitive data (credentials, tokens, PII) in localStorage or sessionStorage; it is readable by any script and persists on the device.
Context: localStorage.setItem("devApiToken", token)
Note: [CWE-312] Cleartext Storage of Sensitive Information.
(local-storage-sensitive-data-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/finicky-ui/src/lib/api.ts` around lines 20 - 30, The API token
handling in API_TOKEN is persisting a per-process bearer secret into
localStorage via the devApiToken key. Update this flow so the token is taken
only from the current query parameter or the injected __FINICKY_API_TOKEN__
value, and remove the persistent fallback/storage path in api.ts. If you keep
any developer convenience for local testing, gate it so it is not used in
shipped code and does not write the token to persistent storage.
Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/finicky-ui/src/lib/api.ts (1)
37-56: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
get/posthave no request timeout.Neither wrapper passes an
AbortSignaltofetch, so a hung or unresponsive local API server will leave callers (and the UI) waiting indefinitely with no way to recover. Since this contract change replaces the previous message-passing bridge with HTTP calls across the whole app, add a bounded timeout viaAbortSignal.timeout(...).Proposed fix
async function get<T>(path: string): Promise<T> { const res = await fetch(`${base()}${path}`, { headers: { "X-Finicky-Token": API_TOKEN }, + signal: AbortSignal.timeout(5000), }); if (!res.ok) throw new Error(`GET ${path} failed: ${res.status}`); return res.json(); } async function post<T>(path: string, body: unknown): Promise<T> { const res = await fetch(`${base()}${path}`, { method: "POST", headers: { "Content-Type": "application/json", "X-Finicky-Token": API_TOKEN, }, body: JSON.stringify(body), + signal: AbortSignal.timeout(5000), }); if (!res.ok) throw new Error(await res.text()); return res.json(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/lib/api.ts` around lines 37 - 56, The get and post wrappers in api.ts currently call fetch without any timeout, so a stalled local API can hang the UI indefinitely. Update both get<T> and post<T> to pass an AbortSignal created with AbortSignal.timeout(...) into fetch, using a bounded shared timeout value so all HTTP calls fail predictably. Keep the existing request headers and error handling, but ensure the timeout is applied in both helper functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/finicky-ui/src/lib/api.ts`:
- Around line 37-56: The get and post wrappers in api.ts currently call fetch
without any timeout, so a stalled local API can hang the UI indefinitely. Update
both get<T> and post<T> to pass an AbortSignal created with
AbortSignal.timeout(...) into fetch, using a bounded shared timeout value so all
HTTP calls fail predictably. Keep the existing request headers and error
handling, but ensure the timeout is applied in both helper functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5db87c63-7d79-40c5-a73d-186952a503cb
📒 Files selected for processing (16)
apps/finicky/src/main.goapps/finicky/src/window/server.goapps/finicky/src/window/window.goapps/finicky/src/window/window.happs/finicky/src/window/window.mpackages/finicky-ui/src/App.tsxpackages/finicky-ui/src/components/BrowserProfileSelector.tsxpackages/finicky-ui/src/components/Footer.tsxpackages/finicky-ui/src/components/OptionRow.tsxpackages/finicky-ui/src/lib/api.tspackages/finicky-ui/src/lib/toast.tspackages/finicky-ui/src/lib/useLogControls.tspackages/finicky-ui/src/lib/useRulesSave.tspackages/finicky-ui/src/pages/StartPage.tsxpackages/finicky-ui/src/pages/TestUrl.tsxpackages/finicky-ui/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/finicky-ui/src/lib/useRulesSave.ts
- packages/finicky-ui/src/components/OptionRow.tsx
- packages/finicky-ui/src/App.tsx
- apps/finicky/src/window/window.h
- packages/finicky-ui/src/components/Footer.tsx
- packages/finicky-ui/src/components/BrowserProfileSelector.tsx
- packages/finicky-ui/src/lib/useLogControls.ts
- packages/finicky-ui/src/pages/TestUrl.tsx
- apps/finicky/src/window/server.go
- packages/finicky-ui/src/types.ts
- packages/finicky-ui/src/pages/StartPage.tsx
- apps/finicky/src/window/window.go
- packages/finicky-ui/src/lib/toast.ts
- apps/finicky/src/main.go
- apps/finicky/src/window/window.m
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/finicky-ui/src/pages/StartPage.tsx (2)
105-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate error-count logic.
numErrorsre-implements the exact same filter thatappStore.getNumErrors()already provides (packages/finicky-ui/src/lib/appStore.ts). Prefer calling the store method to keep the definition of "error" in one place.- const numErrors = messageBuffer.filter((m) => m.level.toLowerCase() === "error").length; + const numErrors = appStore.getNumErrors();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/pages/StartPage.tsx` at line 105, The StartPage error count is duplicating logic that already exists in appStore.getNumErrors(), so keep the definition of “error” centralized. In StartPage.tsx, replace the local messageBuffer.filter(...).length calculation with a call to the store method, and use the existing appStore.getNumErrors() helper from appStore.ts instead of re-implementing the filter inline.
113-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated effect for fetching installed browsers.
This exact effect (including the comment) is duplicated verbatim in
Rules.tsx. Consider extracting a shareduseInstalledBrowsers()hook so the WebView-reload caveat and fetch logic live in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/finicky-ui/src/pages/StartPage.tsx` around lines 113 - 119, The installed-browser fetch effect in StartPage is duplicated verbatim in Rules.tsx, so move the api.getBrowsers/appStore.update logic and the WebView reload caveat into a shared useInstalledBrowsers() hook. Update StartPage and Rules to call that hook instead of keeping separate useEffect blocks. Keep the browser-refresh behavior and comment in one place so future changes only need to be made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/finicky-ui/src/lib/useRulesSave.ts`:
- Around line 18-29: Concurrent saves in useRulesSave can apply stale results
out of order, so add a monotonic request/version guard around the debounced save
flow. In debounced, save(), and scheduleSave(), track the latest request id
alongside isPending.current, increment it for each save trigger, and in the
async callback only call appStore.update({ rulesFile: updated as any }) if the
response still matches the newest id; ignore older responses and still clear
isPending.current in finally. Use the existing useDebouncedCallback,
api.saveRules, and appStore.update symbols to locate the save path.
---
Nitpick comments:
In `@packages/finicky-ui/src/pages/StartPage.tsx`:
- Line 105: The StartPage error count is duplicating logic that already exists
in appStore.getNumErrors(), so keep the definition of “error” centralized. In
StartPage.tsx, replace the local messageBuffer.filter(...).length calculation
with a call to the store method, and use the existing appStore.getNumErrors()
helper from appStore.ts instead of re-implementing the filter inline.
- Around line 113-119: The installed-browser fetch effect in StartPage is
duplicated verbatim in Rules.tsx, so move the api.getBrowsers/appStore.update
logic and the WebView reload caveat into a shared useInstalledBrowsers() hook.
Update StartPage and Rules to call that hook instead of keeping separate
useEffect blocks. Keep the browser-refresh behavior and comment in one place so
future changes only need to be made once.
🪄 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: 13dbc05d-0895-469a-b6b8-770f61ace647
⛔ Files ignored due to path filters (1)
packages/finicky-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/finicky/src/config/vm.goapps/finicky/src/main.goapps/finicky/src/window/server.gopackages/finicky-ui/package.jsonpackages/finicky-ui/src/components/BrowserProfileSelector.tsxpackages/finicky-ui/src/components/Footer.tsxpackages/finicky-ui/src/components/LogContent.tsxpackages/finicky-ui/src/components/OptionRow.tsxpackages/finicky-ui/src/components/PageContainer.tsxpackages/finicky-ui/src/components/TabBar.tsxpackages/finicky-ui/src/components/ToastContainer.tsxpackages/finicky-ui/src/components/Tooltip.tsxpackages/finicky-ui/src/lib/api/client.tspackages/finicky-ui/src/lib/api/index.tspackages/finicky-ui/src/lib/useRulesSave.tspackages/finicky-ui/src/pages/Rules.tsxpackages/finicky-ui/src/pages/StartPage.tsxpackages/finicky-ui/src/pages/TestUrl.tsxpackages/finicky-ui/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/finicky-ui/src/lib/api/index.ts
- packages/finicky-ui/src/components/ToastContainer.tsx
- packages/finicky-ui/src/components/PageContainer.tsx
- packages/finicky-ui/src/components/LogContent.tsx
- packages/finicky-ui/src/components/Tooltip.tsx
- packages/finicky-ui/src/components/TabBar.tsx
- packages/finicky-ui/src/types.ts
- packages/finicky-ui/src/components/Footer.tsx
- packages/finicky-ui/src/pages/TestUrl.tsx
- packages/finicky-ui/src/components/OptionRow.tsx
- packages/finicky-ui/package.json
- packages/finicky-ui/src/lib/api/client.ts
- packages/finicky-ui/src/components/BrowserProfileSelector.tsx
- packages/finicky-ui/src/pages/Rules.tsx
- apps/finicky/src/window/server.go
- apps/finicky/src/main.go
Comment on lines
+18
to
+29
| const isPending = useRef(false); | ||
|
|
||
| const debounced = useDebouncedCallback(async () => { | ||
| try { | ||
| const updated = await api.saveRules(buildPayload()); | ||
| appStore.update({ rulesFile: updated as any }); | ||
| } catch (err) { | ||
| toast.error(errorMessage, err instanceof Error ? err.message : String(err)); | ||
| } finally { | ||
| isPending.current = false; | ||
| } | ||
| }, delay); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Concurrent flush() calls can race and overwrite newer state with a stale response.
save()/scheduleSave() set isPending.current = true and call debounced.flush()/schedule(), but flush() only clears the pending timer — it does not prevent a second call from starting a new api.saveRules request while a prior one is still in flight (e.g., quick successive blur events on adjacent fields, or drag-sort commit followed immediately by another edit). If the second request's response resolves before the first's, appStore.update({ rulesFile: updated }) from the first (stale) response can run last and clobber the newer state, silently discarding the most recent save's result.
🔒 Proposed fix: ignore stale responses via a monotonic request id
export function useRulesSave(
buildPayload: () => Partial<RulesFile>,
delay: number,
errorMessage = "Failed to save rules"
) {
const isPending = useRef(false);
+ const requestId = useRef(0);
const debounced = useDebouncedCallback(async () => {
+ const id = ++requestId.current;
try {
const updated = await api.saveRules(buildPayload());
- appStore.update({ rulesFile: updated as any });
+ if (id === requestId.current) {
+ appStore.update({ rulesFile: updated as any });
+ }
} catch (err) {
toast.error(errorMessage, err instanceof Error ? err.message : String(err));
} finally {
isPending.current = false;
}
}, delay);📝 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
| const isPending = useRef(false); | |
| const debounced = useDebouncedCallback(async () => { | |
| try { | |
| const updated = await api.saveRules(buildPayload()); | |
| appStore.update({ rulesFile: updated as any }); | |
| } catch (err) { | |
| toast.error(errorMessage, err instanceof Error ? err.message : String(err)); | |
| } finally { | |
| isPending.current = false; | |
| } | |
| }, delay); | |
| const isPending = useRef(false); | |
| const requestId = useRef(0); | |
| const debounced = useDebouncedCallback(async () => { | |
| const id = ++requestId.current; | |
| try { | |
| const updated = await api.saveRules(buildPayload()); | |
| if (id === requestId.current) { | |
| appStore.update({ rulesFile: updated as any }); | |
| } | |
| } catch (err) { | |
| toast.error(errorMessage, err instanceof Error ? err.message : String(err)); | |
| } finally { | |
| isPending.current = false; | |
| } | |
| }, delay); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/finicky-ui/src/lib/useRulesSave.ts` around lines 18 - 29, Concurrent
saves in useRulesSave can apply stale results out of order, so add a monotonic
request/version guard around the debounced save flow. In debounced, save(), and
scheduleSave(), track the latest request id alongside isPending.current,
increment it for each save trigger, and in the async callback only call
appStore.update({ rulesFile: updated as any }) if the response still matches the
newest id; ignore older responses and still clear isPending.current in finally.
Use the existing useDebouncedCallback, api.saveRules, and appStore.update
symbols to locate the save path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full rewrite the settings window and integration
Nothing should feel different to end users.
Summary by CodeRabbit
New Features
Bug Fixes