Refactor/react best practises#38
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR comprehensively refactors the frontend architecture by extracting reusable React hooks, centralizing theme/font/density application logic into dedicated helpers, reorganizing utility functions from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/GETTING-STARTED.md (1)
12-12:⚠️ Potential issue | 🟡 MinorNode.js prerequisite is inconsistent with the rest of the PR.
This file still requires
Node.js >= 25, but in this same PRCONTRIBUTING.mdLine 40 anddocs/DEVELOPMENT.mdLine 16 were updated to>=20.19.0 || >=22.13.0 || >=24(matchingfrontend/package.jsonengines). Please align this file too so contributors don't get conflicting requirements.-- **Node.js** `>= 25` +- **Node.js** `>=20.19.0 || >=22.13.0 || >=24`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/GETTING-STARTED.md` at line 12, Update the Node.js prerequisite in the GETTING-STARTED.md entry that currently reads "**Node.js** `>= 25`" to match the project's unified engines constraint (use "`>=20.19.0 || >=22.13.0 || >=24`") so it aligns with the changes made to CONTRIBUTING.md and docs/DEVELOPMENT.md and frontend/package.json; simply replace the version string in that Node.js line to the unified expression.frontend/src/main.tsx (1)
37-62:⚠️ Potential issue | 🟠 MajorCustom theme colors not applied on initial load.
applyTheme(t)at Line 51 is called without the secondcustomColorsargument, and thecustomThemesarray isn't even parsed until lines 54–60. If the persisteds.themeis a custom theme ID, the custom CSS variables will not be set on initial render — the user would see an unstyled / default-theme UI until they re-pick the theme viahandleThemeChange.Consider parsing
customThemesfirst and then resolving the custom colors before applying:🔧 Suggested fix
GetSettings().then(s => { if (!s) return; const t = s.theme || 'vscode-dark' setTheme(t) setDensity(s.density || 'comfortable') setUiFont(s.uiFont || 'Inter') setMonoFont(s.monoFont || 'JetBrains Mono') setLocale(s.locale || 'en') setTerminal(s.terminal || '') if (s.windowX !== undefined) setWindowX(s.windowX) if (s.windowY !== undefined) setWindowY(s.windowY) if (s.windowWidth !== undefined) setWindowWidth(s.windowWidth) if (s.windowHeight !== undefined) setWindowHeight(s.windowHeight) - applyTheme(t) - applyDensity(s.density || 'comfortable') - applyFonts(s.uiFont || 'Inter', s.monoFont || 'JetBrains Mono') + let parsedCustomThemes: CustomTheme[] = [] if (s.customThemes && s.customThemes !== '[]') { try { const parsed = JSON.parse(s.customThemes) - setCustomThemes(Array.isArray(parsed) ? parsed : []) + parsedCustomThemes = Array.isArray(parsed) ? parsed : [] + setCustomThemes(parsedCustomThemes) customThemesStrRef.current = s.customThemes } catch { /* ignore parse error */ } } + const customMatch = parsedCustomThemes.find(c => c.id === t) + applyTheme(t, customMatch?.colors ?? null) + applyDensity(s.density || 'comfortable') + applyFonts(s.uiFont || 'Inter', s.monoFont || 'JetBrains Mono') }).catch(() => {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/main.tsx` around lines 37 - 62, The initial theme application calls applyTheme(t) before parsing persisted customThemes, so custom theme CSS variables are never applied on load; update the GetSettings().then handler in main.tsx to parse s.customThemes (use JSON.parse with try/catch) and call setCustomThemes(...) and set customThemesStrRef.current before calling applyTheme; then, when resolving the theme ID t, derive the matching customColors from the parsed customThemes array and pass those as the second argument to applyTheme(t, customColors) (ensure applyTheme is still called with defaults when no matching custom theme is found).docs/TESTING.md (1)
9-16:⚠️ Potential issue | 🟡 MinorSection 1 contradicts the new Section 6 — Go tests do exist.
Section 1 still asserts "Cmdex currently has no automated tests.", "Go backend: No
*_test.gofiles exist", and "CI ... do not run any test suites", but Section 6 (added in this PR) documentsdb_test.gowithTestFreshDBMigrations,TestExistingDBIdempotent, andTestRollbackTo, and Section 7 even includes their expectedgo testoutput. Either pre-existing readers will trust Section 1 and miss the real tests, or skim Section 6 and conclude the doc is stale.Recommend rewording Section 1 (and the "Verification is currently done entirely through manual QA" line) to reflect that Go tests now exist for the DB layer while frontend remains untested. The stale "Use a temporary SQLite file or
:memory:database in tests" note in the priority-targets table at line 75 should also be updated sincedb_test.goalready does this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TESTING.md` around lines 9 - 16, Update the docs to remove the contradictory blanket statement in Section 1 that "Cmdex currently has no automated tests" and instead state that Go DB tests now exist; specifically mention db_test.go and its tests TestFreshDBMigrations, TestExistingDBIdempotent, and TestRollbackTo are present and exercised in the doc, while frontend/unit tests are still missing and CI should be noted to not run frontend tests. Also update the priority-targets table note about using a temporary SQLite file or ":memory:" to reflect that db_test.go already uses an in-memory/temporary SQLite setup so the guidance is no longer required.
🧹 Nitpick comments (14)
frontend/src/utils/path.ts (1)
37-43: Minor: separator detection on mixed-separator paths.The split handles both
/and\, but the rejoin separator is chosen by the presence of any\in the input. For a mixed path likeC:\Users/project/foo/bar(which can occur when users paste Windows paths into POSIX-style tooling), the output uses\even though most segments were/-separated. Consider picking the separator by majority/first occurrence, or normalizing before split.♻️ Optional refactor
- const parts = path.split(/[\\/]/).filter(Boolean); - if (parts.length <= segments) return path; - const sep = path.includes('\\') ? '\\' : '/'; - return '...' + sep + parts.slice(-segments).join(sep); + const parts = path.split(/[\\/]/).filter(Boolean); + if (parts.length <= segments) return path; + // Prefer the separator that appears first in the original path. + const firstSlash = path.indexOf('/'); + const firstBackslash = path.indexOf('\\'); + const sep = + firstBackslash !== -1 && (firstSlash === -1 || firstBackslash < firstSlash) + ? '\\' + : '/'; + return '...' + sep + parts.slice(-segments).join(sep);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/path.ts` around lines 37 - 43, The separator detection in shortenPath is brittle for mixed-separator inputs (e.g., "C:\Users/project/..."): update shortenPath to choose the rejoin separator deterministically by inspecting the path's separators (for example, count occurrences of '/' vs '\' and pick the majority, or pick the first separator character found) or normalize the input before splitting; change the logic around const sep = ... in shortenPath to compute sep by majority/first-occurrence (or normalize separators) so the returned '...' + sep + parts.slice(-segments).join(sep) uses the appropriate separator consistently.frontend/src/utils/tab.ts (2)
27-29: Minor: trim before appending the ellipsis.If character 50 lands on whitespace, the result is
"…some text ...". A.trimEnd()on the slice keeps truncation visually tidy.- if (body.length <= 50) return body; - return body.slice(0, 50) + '...'; + if (body.length <= 50) return body; + return body.slice(0, 50).trimEnd() + '…';(Also worth considering the single‑char
…to match the ellipsis already used byscriptSnippet.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/tab.ts` around lines 27 - 29, In the truncation logic in frontend/src/utils/tab.ts (the function that returns a snippet for body, containing the lines that check body.length and slice to 50), replace the current slice+ '...' with a trimmed slice to avoid trailing whitespace before the ellipsis — e.g. use body.slice(0, 50).trimEnd() plus the ellipsis; optionally switch the appended "..." to the single‑char "…" to match scriptSnippet's style so update the return that currently does return body.slice(0, 50) + '...' accordingly.
18-23: Shebang stripping is too narrow; use a generic regex for parity withscriptSnippet.
getCommandDisplayTitleonly recognizes the literal#!/bin/bashshebang, so commands using#!/usr/bin/env bash,#!/bin/sh,#!/usr/bin/env python, etc. will surface their shebang line as the displayed title (and consume part of the 50‑char budget). Note thatscriptSnippetinfrontend/src/components/CommandPalette.tsx(line 52) already strips any shebang via/^#!.*\n?/, so the two derivations diverge for the same script.♻️ Suggested generic stripping
- let body = cmd.scriptContent; - if (body.startsWith('#!/bin/bash\n')) { - body = body.slice('#!/bin/bash\n'.length); - } else if (body.startsWith('#!/bin/bash')) { - body = body.slice('#!/bin/bash'.length); - } - - body = body.replace(/\n/g, ' ').trim(); + const body = cmd.scriptContent + .replace(/^#!.*\r?\n?/, '') + .replace(/\n/g, ' ') + .trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/tab.ts` around lines 18 - 23, The shebang stripping in getCommandDisplayTitle is too specific (only '#!/bin/bash') and should use a generic regex to match any shebang like scriptSnippet does; update the logic in frontend/src/utils/tab.ts (inside getCommandDisplayTitle / where body is derived from cmd.scriptContent) to remove a leading shebang using a regex such as /^#!.*\n?/ so that any interpreter line (e.g., #!/usr/bin/env bash, #!/bin/sh, #!/usr/bin/env python) is stripped before truncating/displaying the title.frontend/src/hooks/useResizable.ts (2)
22-29: Replace the size→ref sync effect with the newuseSyncedRef.This file already pairs well with the new helper introduced in the same PR — the
sizeRef+useEffectblock is exactly whatuseSyncedRefwas designed to eliminate.- const [size, setSize] = useState<number>(...); - ... - const sizeRef = useRef(size); - - useEffect(() => { - sizeRef.current = size; - }, [size]); + const [size, setSize] = useState<number>(...); + ... + const sizeRef = useSyncedRef(size);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useResizable.ts` around lines 22 - 29, Replace the manual size→ref syncing logic by using the new helper: remove the useRef(size) + useEffect block and instead create sizeRef with useSyncedRef(size) so sizeRef always mirrors the latest size; update references that use sizeRef (symbol: sizeRef) and ensure useSyncedRef is imported and used inside the useResizable hook (symbols: useResizable, size).
12-13: Add a JSDoc block describing options and return shape.As per coding guidelines: "JSDoc-style blocks must be used on non-obvious hooks". The
axis/direction/storageKeysemantics (and the appended-sizesuffix) are not obvious to consumers and warrant documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useResizable.ts` around lines 12 - 13, Add a JSDoc block to the useResizable hook that documents the UseResizableOptions properties (axis, direction, minSize, maxSize, defaultSize, storageKey) and the hook's return shape; explicitly explain allowed values for axis and direction, the semantics of storageKey (it is used as a prefix and has "-size" appended), and the units/constraints for minSize/maxSize and defaultSize so callers understand expected types and behavior; place the JSDoc immediately above the export function useResizable and reference UseResizableOptions and the returned tuple/object shape in the description.frontend/src/hooks/useCopyToClipboard.ts (1)
3-3: Add a JSDoc block describing the hook’s behavior.Per coding guideline
frontend/src/hooks/**/*.ts: JSDoc-style blocks must be used on non-obvious hooks. Document the API contract (return shape,resetMssemantics, error handling), especially since the resolution of the error-handling question above will affect callers.+/** + * Copy text to the system clipboard with a transient `copied` indicator. + * `@param` resetMs How long (ms) `copied` stays true after a successful copy. Default 1500. + * `@returns` `{ copied, copy(text) }`. `copy` resolves on success and rejects on failure. + */ export function useCopyToClipboard(resetMs = 1500) {As per coding guidelines: "JSDoc-style blocks must be used on non-obvious hooks (e.g., useKeyboardShortcuts behavior)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useCopyToClipboard.ts` at line 3, Add a JSDoc block for the hook function useCopyToClipboard that documents the API contract: describe the resetMs parameter (default 1500ms) and its effect (how long the success/err state is cleared), the return shape (e.g., tuple or object containing the copy function, a boolean success flag, and any error/message), and how errors are surfaced/handled (whether the hook throws, returns an error value, or sets an error state). Place the JSDoc immediately above the export function useCopyToClipboard declaration and include types, parameter description, return description, and any edge-case behavior callers must handle.frontend/src/hooks/useSyncedRef.ts (1)
8-13: Consider an explicit return type for the public hook.Adding
: React.RefObject<T>(React 19 madeRefObjectmutable, replacingMutableRefObject) to the signature documents intent and prevents accidental inference drift if the body changes:♻️ Suggested signature
-export function useSyncedRef<T>(value: T) { - const ref = useRef(value); +export function useSyncedRef<T>(value: T): React.RefObject<T> { + const ref = useRef<T>(value); // eslint-disable-next-line react-hooks/refs -- render-phase sync is intentional and documented ref.current = value; return ref; }Also worth confirming
react-hooks/refsis the correct rule id used byeslint-plugin-react-hooksv6 — if it isn't, the disable is silently ineffective.eslint-plugin-react-hooks v6 rule names list (react-hooks/refs render-phase mutation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useSyncedRef.ts` around lines 8 - 13, Add an explicit return type to the public hook by changing the signature of useSyncedRef<T> to return React.RefObject<T> (ensure React is imported or use import type if your setup supports it), keep the intentional render-phase assignment to ref.current, and update the ESLint disable comment: verify the exact rule id for render-phase ref mutation in your eslint-plugin-react-hooks v6 (if the rule name is different than react-hooks/refs then replace the comment with the correct rule id) so the disable is effective.frontend/src/main.tsx (1)
79-85: Direct CSS variable mutations remain alongside the helper.Lines 81 and 84 still write
--cmdex-last-dark-theme/--cmdex-last-light-themedirectly ondocument.documentElement, while the rest of the theme application has been delegated toapplyTheme. For consistency with the centralization goal of this PR, consider moving these two writes into thetheme-applyhelper (or a dedicated helper) so DOM mutations live in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/main.tsx` around lines 79 - 85, The code directly sets CSS vars '--cmdex-last-dark-theme' and '--cmdex-last-light-theme' in the if/else branch alongside calling setLastDarkTheme/setLastLightTheme; move those DOM mutations into the centralized theme application helper (e.g., the applyTheme or theme-apply helper) so that only setLastDarkTheme/setLastLightTheme update state here and the helper updates document.documentElement.style.setProperty for both vars; update applyTheme (or create a small function in the theme-apply module) to accept the newTheme and themeType and perform the two setProperty calls, then remove the direct setProperty calls from main.tsx.frontend/src/components/CommandDetailTab.tsx (2)
95-98: Unnecessary spread inboundDraftChange.Spreading
partialinto a new object on every call adds allocations without behavior change —onDraftChangealready receives aPartial<TabDraft>. You can passpartialthrough directly (or drop the wrapper entirely and forwardonDraftChangefrom props).♻️ Proposed simplification
- const boundDraftChange = useCallback( - (partial: Partial<TabDraft>) => onDraftChange({ ...partial }), - [onDraftChange], - ); + const boundDraftChange = useCallback( + (partial: Partial<TabDraft>) => onDraftChange(partial), + [onDraftChange], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CommandDetailTab.tsx` around lines 95 - 98, The boundDraftChange callback currently spreads the incoming Partial<TabDraft> into a new object which creates unnecessary allocations; update the useCallback for boundDraftChange to call onDraftChange(partial) directly (or remove boundDraftChange and forward the onDraftChange prop instead) so that Partial<TabDraft> is passed through without cloning; look for the boundDraftChange declaration and the onDraftChange prop to implement the direct pass-through.
128-128: Dead!draftguard.
draftis typed as a requiredTabDraftonCommandDetailTabProps, so!draftcan never be truthy here. Drop it (or makedraftoptional in the props if undefined is actually expected).- saveDisabled={!draft || !draft.scriptBody.trim()} + saveDisabled={!draft.scriptBody.trim()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CommandDetailTab.tsx` at line 128, The expression uses a redundant `!draft` guard in the `saveDisabled` prop of `CommandDetailTab` because `draft` is declared required on `CommandDetailTabProps` (type `TabDraft`); either remove `!draft` from the `saveDisabled` expression so it becomes `saveDisabled={!draft.scriptBody.trim()}`-style (adjusting for truthiness) or, if `draft` can legitimately be undefined, make `draft` optional in the `CommandDetailTabProps` type (`draft?: TabDraft`) and add a runtime check where `saveDisabled` is computed (e.g., `!draft || !draft.scriptBody.trim()`) so `CommandDetailTab` and the `saveDisabled` prop remain correct; update the `CommandDetailTabProps` type and the `saveDisabled` usage accordingly.frontend/src/App.tsx (4)
1237-1271:handleSaveScriptlargely duplicates the update branch ofhandleSaveTab.Both functions perform the same flow against
UpdateCommand: trim the script body, runbuildVariablesFromScript, run the var-removal-with-presets gate (skipVarRemovalCheckRef+pendingDirectSaveBodyRef), callUpdateCommandwith all draft fields, refresh fromGetScriptBody, updatetabDrafts/tabBaselines, and conditionallysetSelectedCommand. The only meaningful difference is thathandleSaveScriptuses an explicitscriptBodyargument instead ofd.scriptBody.Worth extracting a shared
saveExistingCommand(tabId, draft, scriptBodyOverride?)helper so the two stay in sync (e.g. if you ever change toast messages, error handling, or the post-save sync). Not blocking, but the duplicatedpendingDirectSaveBodyRef/skipVarRemovalCheckReforchestration in particular is easy to drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1237 - 1271, handleSaveScript duplicates the update flow found in handleSaveTab; extract the shared logic into a new helper (e.g. saveExistingCommand(tabId, draft, scriptBodyOverride?)) that: accepts the tabId and draft (and optional scriptBody override), trims and builds vars (using buildVariablesFromScript), runs the skipVarRemovalCheckRef / pendingDirectSaveBodyRef gate and returns early when needed, calls UpdateCommand with draft fields, reloads data and refreshes the saved draft from GetScriptBody, updates tabDrafts/tabBaselines and selected command, and emits the toast and error handling; replace the duplicate bodies in handleSaveScript and handleSaveTab to call this helper so both flows remain identical and avoid drift.
1521-1533: Inline-arrow props recreate on everyApprender and defeat the newCommandDetailTabmemoization.
onDraftChange,onSave, andonDiscardare passed as fresh arrow functions on every render ofApp. Combined withReact.memoonCommandDetailTab(frontend/src/components/CommandDetailTab.tsx), this means each open command tab re-renders on every state change inApp— losing the perf benefit the per-tab mounts + memo were intended to deliver.Consider switching these three to the same
tabId-injection pattern already used foronExecute/onSaveScript(parent passes the stable handler, child'suseCallbackinjectstabId):♻️ Proposed change
- onDraftChange={(partial) => updateDraft(tab.id, partial)} + onDraftChange={updateDraft} ... - onSave={() => void handleSaveTab(tab.id)} - onDiscard={() => handleDiscardTab(tab.id)} + onSave={handleSaveTab} + onDiscard={handleDiscardTab}Then in
CommandDetailTab.tsx, addtabIdinjection and adjust the prop types to(tabId, ...)/(tabId).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1521 - 1533, Replace the inline arrow props that recreate each render (onDraftChange, onSave, onDiscard) by passing stable handlers from App and letting CommandDetailTab inject the tab id; specifically, stop calling updateDraft(tab.id, ...) and handleSaveTab(tab.id)/handleDiscardTab(tab.id) inline and instead pass the stable functions (e.g., updateDraft, handleSaveTab, handleDiscardTab) as props, then modify CommandDetailTab (frontend/src/components/CommandDetailTab.tsx) to accept those stable functions and use useCallback to wrap them with the local tabId (matching the existing pattern used for onExecute/onSaveScript), and update the prop types for CommandDetailTab accordingly so memoization is effective.
1090-1110:handleFillVariablesandhandleFillVariablesByTabare near-identical.
handleFillVariables(line 1090) andhandleFillVariablesByTab(line 1101) do the same thing modulo where the command id comes from. The first is only used by theCmd/Ctrl+Entershortcut at line 1323; you can drop it and callhandleFillVariablesByTab(selectedCommand.id, currentResolvedValues)instead, after the existingselectedCommand && !isNewCommandTabId(...)guard at line 1317.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1090 - 1110, Remove the duplicate handler by deleting handleFillVariables and reuse handleFillVariablesByTab: update the Cmd/Ctrl+Enter shortcut logic (where the code checks selectedCommand && !isNewCommandTabId(...)) to call handleFillVariablesByTab(selectedCommand.id, currentResolvedValues) instead of the removed function; ensure handleFillVariablesByTab remains exported/defined with the same signature (tabId: string, initialValues: Record<string,string>) and keeps the isNewCommandTabId(tabId) guard and GetVariables call so behavior is unchanged.
719-719: PreferString.prototype.trim()over the equivalent regex.
s.replace(/^\s+|\s+$/g, '')is justs.trim(). Both occurrences (inhandleSaveTabandhandleSaveScript) can be simplified for readability with no behavior change.Also applies to: 1241-1241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` at line 719, Replace the regex-based trimming with String.prototype.trim() in the save handlers: change occurrences of d.scriptBody.replace(/^\s+|\s+$/g, '') to d.scriptBody.trim() inside the handleSaveTab and handleSaveScript code paths (also update the other occurrence noted around the second location) to improve readability without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 40: The code span currently contains escaped pipes (`>=20.19.0 \|\|
>=22.13.0 \|\| >=24`) which will render the backslashes literally; update that
code span to use unescaped pipes (`>=20.19.0 || >=22.13.0 || >=24`) so the
Markdown renders correctly and matches the format used in docs/DEVELOPMENT.md
(see the `>=20.19.0 \|\| >=22.13.0 \|\| >=24` text to locate the span).
In `@docs/API.md`:
- Line 402: The per-service API sections use H4 for method headings (e.g., "####
GetVariables(commandID: string)") which skips heading levels and breaks the TOC;
update each service block (e.g., "## CommandService API", "## ExecutionService
API", "## SettingsService API", "## ImportExportService API", "## App Service
API", "## EventService API") so method entries are under a consistent H3 section
— either change each method heading (e.g., "GetVariables", "GetCategories",
etc.) from H4 to H3 or insert a "### Methods" subheading and keep the method
entries as H4 beneath it to match the pattern used by CommandService API and
satisfy markdownlint MD001.
- Around line 425-443: The import path for eventNames in the streaming example
is incorrect for typical component callers; update the import to match the
documented layout (use the relative path used elsewhere from components) — e.g.
replace the current "import { eventNames } from './wails/events';" with the
depth-correct import (from a components TSX consumer) such as "import {
eventNames } from '../wails/events';" and make the same change for the repeated
snippet that currently uses the wrong path; leave the rest of the example
(Events.On, RunCommand, cleanup) unchanged.
In `@docs/ARCHITECTURE.md`:
- Line 342: The document contains conflicting schema versions: the "Schema
version" paragraph (the earlier section mentioning "Schema version: 9, with
incremental migrations from v1 through v9") and the `DB` struct summary (which
says migrations "currently version 10"); choose one resolution and make the
texts consistent by either (A) bumping the earlier "Schema version" section to
v10 and adding a brief description of the v10 migration (what it changes/why)
and updating the incremental migrations list to include v10, or (B) reverting
the `DB` struct summary back to v9 so both sections state v9; update the "Schema
version" paragraph and the `DB` struct line together to match and ensure the
migration list and any mention of FTS5/WAL/migration version numbers reflect the
chosen version.
In `@docs/CONFIGURATION.md`:
- Around line 262-263: The table rows for `windowWidth` and `windowHeight` are
contradictory because they state minimums of 480/400 but cite `main.go`'s
`MinWidth: 900` / `MinHeight: 600` (which apply to the main window) while the
documented defaults (e.g., `windowWidth: 640`, `windowHeight: 520`) are for the
settings window; update the docs by either removing the misleading enforcement
reference or split into separate rows: one for the main window (referencing
`MinWidth`/`MinHeight` in `main.go` and the main-window minima) and one for the
settings window (showing the settings defaults and any re-centering logic), and
ensure the `windowWidth`/`windowHeight` labels and descriptions explicitly
indicate which window each constraint applies to.
In `@docs/DEPLOYMENT.md`:
- Line 228: Remove the leftover author/draft HTML comments like "<!-- VERIFY:
Server mode port and host defaults are from Dockerfile.server — confirm these
match production deployment expectations -->" (and the similar comments at the
other occurrences) from the published document; either delete them or move them
into a maintainer-only checklist (e.g., a separate MAINTAINERS.md or an internal
review TODO list) and replace any public-facing guidance with finalized
verification text, ensuring references such as the verification about server
mode port/host defaults are resolved and the docs contain only user-facing
content.
In `@docs/DEVELOPMENT.md`:
- Line 335: Update the broken Markdown link for "CI workflow" in the
documentation: change the link target currently written as
(.github/workflows/ci.yml) so it points to the repository root relative path
(../.github/workflows/ci.yml) so the "CI workflow" link resolves correctly from
the docs/DEVELOPMENT.md location.
In `@docs/GETTING-STARTED.md`:
- Around line 131-139: The docs currently reference the old Wails import path
"../wailsjs/go/main/App" which is stale for Wails v3; update the example text to
use the actual generated bindings path (e.g. "frontend/bindings" and examples
like "../bindings/cmdex/settingsservice" as used in frontend/src/main.tsx) so
the instructions and import example match the project's Wails v3 layout and the
regeneration command output.
In `@frontend/src/components/CommandDetail.tsx`:
- Around line 606-611: The copy-failure toast is unreachable because
useCopyToClipboard swallows promise rejections; update the hook
(useCopyToClipboard) so that the copy function it returns does not swallow
errors but either re-throws the caught error or returns a rejected Promise when
the underlying clipboard operation fails, allowing callers (like handleCopy in
CommandDetail.tsx) to catch and handle the error; ensure the exported copy
function preserves the original rejection behavior instead of swallowing it.
In `@frontend/src/components/CommandDetailTab.tsx`:
- Around line 126-131: CommandDetailTab wrapped in React.memo is being
invalidated because App.tsx passes fresh inline arrow props (onDraftChange,
onSave, onDiscard) each render; stabilize them by either: 1) stop passing
per-tab inline arrows and instead pass the memoized handlers (updateDraft,
handleSaveTab, handleDiscardTab) plus the tab id (tab.id) and let
CommandDetailTab bind those to produce stable callbacks internally (similar to
onExecute/onSaveScript), or 2) memoize the per-tab callbacks in App.tsx using
useCallback keyed on tab.id so the function identity is stable across renders;
update references to onDraftChange, onSave, and onDiscard accordingly so
React.memo can short-circuit re-renders.
In `@frontend/src/components/OutputPane.tsx`:
- Around line 106-114: Update handleCopy and handleCopyCommand to show a failure
toast when the copy promise rejects: wrap copyOutput(allOutputText) and
copyCommand(record.finalCmd) in try/catch (or await them) and on error call the
same toast pattern used in CommandDetail.tsx with the new i18n key
"outputPane.copyFailed". Keep early returns for missing data (allOutputText /
record?.finalCmd), and reference the existing functions handleCopy,
handleCopyCommand, copyOutput, copyCommand, and record.finalCmd so the error
path surfaces clipboard failures to the user.
In `@frontend/src/components/SettingsPage.tsx`:
- Around line 322-329: The cleanup currently runs on every dependency change
because useEffect returns a cleanup but lists
savedTheme/savedDensity/savedUiFont/savedMonoFont/customThemes as deps; change
this so the restore logic only runs on unmount by creating a synced ref of the
latest saved values (use the new useSyncedRef hook) that stores {savedTheme,
savedDensity, savedUiFont, savedMonoFont, customThemes}, then replace the effect
with useEffect(() => { return () => { const {savedTheme, savedDensity,
savedUiFont, savedMonoFont, customThemes} = syncedRef.current; const custom =
customThemes?.find(c => c.id === savedTheme); applyTheme(savedTheme,
custom?.colors ?? null); applyDensity(savedDensity); applyFonts(savedUiFont,
savedMonoFont); }; }, []); reference useSyncedRef, applyTheme, applyDensity,
applyFonts, customThemes, and the saved* variables to locate and update the
code.
In `@frontend/src/hooks/useCopyToClipboard.ts`:
- Around line 7-16: The copy function inside useCopyToClipboard (the async
callback created with useCallback) currently swallows errors in its try/catch so
callers like CommandDetail.tsx never see rejections; modify the copy
implementation to allow consumers to observe failures by either re-throwing the
caught error after logging (or removing the try/catch and letting the rejection
propagate), keeping the existing setCopied, timerRef, and resetMs logic intact
so the success path still behaves the same.
In `@frontend/src/lib/theme-apply.ts`:
- Around line 26-32: The applyFonts function currently interpolates uiFont and
monoFont directly into single-quoted CSS values which breaks if names contain
single quotes or backslashes and also misbehaves for empty strings; update
applyFonts to treat empty uiFont as 'System Default', and sanitize/escape any
single quotes or backslashes in uiFont and monoFont (or wrap values in double
quotes and escape any embedded double quotes/backslashes) before calling
document.documentElement.style.setProperty('--font-sans', ...) and
setProperty('--font-mono', ...), ensuring the final CSS token is always a valid
quoted font-family string.
In `@README.md`:
- Line 31: The README and DEPLOYMENT docs disagree on the macOS app bundle name:
README uses /Applications/CmDex.app while DEPLOYMENT.md references
/Applications/cmdex.app; align them to the canonical name from wails.json (name:
"CmDex") by updating the occurrence of "cmdex.app" in DEPLOYMENT.md to
"CmDex.app" so both docs use the exact same bundle path
(/Applications/CmDex.app).
---
Outside diff comments:
In `@docs/GETTING-STARTED.md`:
- Line 12: Update the Node.js prerequisite in the GETTING-STARTED.md entry that
currently reads "**Node.js** `>= 25`" to match the project's unified engines
constraint (use "`>=20.19.0 || >=22.13.0 || >=24`") so it aligns with the
changes made to CONTRIBUTING.md and docs/DEVELOPMENT.md and
frontend/package.json; simply replace the version string in that Node.js line to
the unified expression.
In `@docs/TESTING.md`:
- Around line 9-16: Update the docs to remove the contradictory blanket
statement in Section 1 that "Cmdex currently has no automated tests" and instead
state that Go DB tests now exist; specifically mention db_test.go and its tests
TestFreshDBMigrations, TestExistingDBIdempotent, and TestRollbackTo are present
and exercised in the doc, while frontend/unit tests are still missing and CI
should be noted to not run frontend tests. Also update the priority-targets
table note about using a temporary SQLite file or ":memory:" to reflect that
db_test.go already uses an in-memory/temporary SQLite setup so the guidance is
no longer required.
In `@frontend/src/main.tsx`:
- Around line 37-62: The initial theme application calls applyTheme(t) before
parsing persisted customThemes, so custom theme CSS variables are never applied
on load; update the GetSettings().then handler in main.tsx to parse
s.customThemes (use JSON.parse with try/catch) and call setCustomThemes(...) and
set customThemesStrRef.current before calling applyTheme; then, when resolving
the theme ID t, derive the matching customColors from the parsed customThemes
array and pass those as the second argument to applyTheme(t, customColors)
(ensure applyTheme is still called with defaults when no matching custom theme
is found).
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Around line 1237-1271: handleSaveScript duplicates the update flow found in
handleSaveTab; extract the shared logic into a new helper (e.g.
saveExistingCommand(tabId, draft, scriptBodyOverride?)) that: accepts the tabId
and draft (and optional scriptBody override), trims and builds vars (using
buildVariablesFromScript), runs the skipVarRemovalCheckRef /
pendingDirectSaveBodyRef gate and returns early when needed, calls UpdateCommand
with draft fields, reloads data and refreshes the saved draft from
GetScriptBody, updates tabDrafts/tabBaselines and selected command, and emits
the toast and error handling; replace the duplicate bodies in handleSaveScript
and handleSaveTab to call this helper so both flows remain identical and avoid
drift.
- Around line 1521-1533: Replace the inline arrow props that recreate each
render (onDraftChange, onSave, onDiscard) by passing stable handlers from App
and letting CommandDetailTab inject the tab id; specifically, stop calling
updateDraft(tab.id, ...) and handleSaveTab(tab.id)/handleDiscardTab(tab.id)
inline and instead pass the stable functions (e.g., updateDraft, handleSaveTab,
handleDiscardTab) as props, then modify CommandDetailTab
(frontend/src/components/CommandDetailTab.tsx) to accept those stable functions
and use useCallback to wrap them with the local tabId (matching the existing
pattern used for onExecute/onSaveScript), and update the prop types for
CommandDetailTab accordingly so memoization is effective.
- Around line 1090-1110: Remove the duplicate handler by deleting
handleFillVariables and reuse handleFillVariablesByTab: update the
Cmd/Ctrl+Enter shortcut logic (where the code checks selectedCommand &&
!isNewCommandTabId(...)) to call handleFillVariablesByTab(selectedCommand.id,
currentResolvedValues) instead of the removed function; ensure
handleFillVariablesByTab remains exported/defined with the same signature
(tabId: string, initialValues: Record<string,string>) and keeps the
isNewCommandTabId(tabId) guard and GetVariables call so behavior is unchanged.
- Line 719: Replace the regex-based trimming with String.prototype.trim() in the
save handlers: change occurrences of d.scriptBody.replace(/^\s+|\s+$/g, '') to
d.scriptBody.trim() inside the handleSaveTab and handleSaveScript code paths
(also update the other occurrence noted around the second location) to improve
readability without changing behavior.
In `@frontend/src/components/CommandDetailTab.tsx`:
- Around line 95-98: The boundDraftChange callback currently spreads the
incoming Partial<TabDraft> into a new object which creates unnecessary
allocations; update the useCallback for boundDraftChange to call
onDraftChange(partial) directly (or remove boundDraftChange and forward the
onDraftChange prop instead) so that Partial<TabDraft> is passed through without
cloning; look for the boundDraftChange declaration and the onDraftChange prop to
implement the direct pass-through.
- Line 128: The expression uses a redundant `!draft` guard in the `saveDisabled`
prop of `CommandDetailTab` because `draft` is declared required on
`CommandDetailTabProps` (type `TabDraft`); either remove `!draft` from the
`saveDisabled` expression so it becomes
`saveDisabled={!draft.scriptBody.trim()}`-style (adjusting for truthiness) or,
if `draft` can legitimately be undefined, make `draft` optional in the
`CommandDetailTabProps` type (`draft?: TabDraft`) and add a runtime check where
`saveDisabled` is computed (e.g., `!draft || !draft.scriptBody.trim()`) so
`CommandDetailTab` and the `saveDisabled` prop remain correct; update the
`CommandDetailTabProps` type and the `saveDisabled` usage accordingly.
In `@frontend/src/hooks/useCopyToClipboard.ts`:
- Line 3: Add a JSDoc block for the hook function useCopyToClipboard that
documents the API contract: describe the resetMs parameter (default 1500ms) and
its effect (how long the success/err state is cleared), the return shape (e.g.,
tuple or object containing the copy function, a boolean success flag, and any
error/message), and how errors are surfaced/handled (whether the hook throws,
returns an error value, or sets an error state). Place the JSDoc immediately
above the export function useCopyToClipboard declaration and include types,
parameter description, return description, and any edge-case behavior callers
must handle.
In `@frontend/src/hooks/useResizable.ts`:
- Around line 22-29: Replace the manual size→ref syncing logic by using the new
helper: remove the useRef(size) + useEffect block and instead create sizeRef
with useSyncedRef(size) so sizeRef always mirrors the latest size; update
references that use sizeRef (symbol: sizeRef) and ensure useSyncedRef is
imported and used inside the useResizable hook (symbols: useResizable, size).
- Around line 12-13: Add a JSDoc block to the useResizable hook that documents
the UseResizableOptions properties (axis, direction, minSize, maxSize,
defaultSize, storageKey) and the hook's return shape; explicitly explain allowed
values for axis and direction, the semantics of storageKey (it is used as a
prefix and has "-size" appended), and the units/constraints for minSize/maxSize
and defaultSize so callers understand expected types and behavior; place the
JSDoc immediately above the export function useResizable and reference
UseResizableOptions and the returned tuple/object shape in the description.
In `@frontend/src/hooks/useSyncedRef.ts`:
- Around line 8-13: Add an explicit return type to the public hook by changing
the signature of useSyncedRef<T> to return React.RefObject<T> (ensure React is
imported or use import type if your setup supports it), keep the intentional
render-phase assignment to ref.current, and update the ESLint disable comment:
verify the exact rule id for render-phase ref mutation in your
eslint-plugin-react-hooks v6 (if the rule name is different than
react-hooks/refs then replace the comment with the correct rule id) so the
disable is effective.
In `@frontend/src/main.tsx`:
- Around line 79-85: The code directly sets CSS vars '--cmdex-last-dark-theme'
and '--cmdex-last-light-theme' in the if/else branch alongside calling
setLastDarkTheme/setLastLightTheme; move those DOM mutations into the
centralized theme application helper (e.g., the applyTheme or theme-apply
helper) so that only setLastDarkTheme/setLastLightTheme update state here and
the helper updates document.documentElement.style.setProperty for both vars;
update applyTheme (or create a small function in the theme-apply module) to
accept the newTheme and themeType and perform the two setProperty calls, then
remove the direct setProperty calls from main.tsx.
In `@frontend/src/utils/path.ts`:
- Around line 37-43: The separator detection in shortenPath is brittle for
mixed-separator inputs (e.g., "C:\Users/project/..."): update shortenPath to
choose the rejoin separator deterministically by inspecting the path's
separators (for example, count occurrences of '/' vs '\' and pick the majority,
or pick the first separator character found) or normalize the input before
splitting; change the logic around const sep = ... in shortenPath to compute sep
by majority/first-occurrence (or normalize separators) so the returned '...' +
sep + parts.slice(-segments).join(sep) uses the appropriate separator
consistently.
In `@frontend/src/utils/tab.ts`:
- Around line 27-29: In the truncation logic in frontend/src/utils/tab.ts (the
function that returns a snippet for body, containing the lines that check
body.length and slice to 50), replace the current slice+ '...' with a trimmed
slice to avoid trailing whitespace before the ellipsis — e.g. use body.slice(0,
50).trimEnd() plus the ellipsis; optionally switch the appended "..." to the
single‑char "…" to match scriptSnippet's style so update the return that
currently does return body.slice(0, 50) + '...' accordingly.
- Around line 18-23: The shebang stripping in getCommandDisplayTitle is too
specific (only '#!/bin/bash') and should use a generic regex to match any
shebang like scriptSnippet does; update the logic in frontend/src/utils/tab.ts
(inside getCommandDisplayTitle / where body is derived from cmd.scriptContent)
to remove a leading shebang using a regex such as /^#!.*\n?/ so that any
interpreter line (e.g., #!/usr/bin/env bash, #!/bin/sh, #!/usr/bin/env python)
is stripped before truncating/displaying the title.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1b288bd-ac43-4142-9481-761bf28b8b9d
📒 Files selected for processing (25)
CONTRIBUTING.mdREADME.mddocs/API.mddocs/ARCHITECTURE.mddocs/CONFIGURATION.mddocs/DEPLOYMENT.mddocs/DEVELOPMENT.mddocs/GETTING-STARTED.mddocs/TESTING.mdfrontend/src/App.tsxfrontend/src/components/CommandDetail.tsxfrontend/src/components/CommandDetailTab.tsxfrontend/src/components/CommandPalette.tsxfrontend/src/components/OutputPane.tsxfrontend/src/components/ResizablePanel.tsxfrontend/src/components/SettingsPage.tsxfrontend/src/components/Sidebar.tsxfrontend/src/hooks/useCopyToClipboard.tsfrontend/src/hooks/useResizable.tsfrontend/src/hooks/useSyncedRef.tsfrontend/src/lib/theme-apply.tsfrontend/src/main.tsxfrontend/src/types.tsfrontend/src/utils/path.tsfrontend/src/utils/tab.ts
💤 Files with no reviewable changes (1)
- frontend/src/types.ts
|
|
||
| | Abstraction | File | Description | | ||
| |---|---|---| | ||
| | `DB` struct | `db.go` | Database wrapper around `database/sql` with `modernc.org/sqlite` driver. Manages migrations (currently version 10), FTS5 full-text search triggers, WAL journal mode, and provides all CRUD query methods used by services. | |
Contributor
There was a problem hiding this comment.
Schema version conflicts with earlier section.
Line 342 states the DB struct manages migrations "currently version 10", but line 83 above in the same doc still says "Schema version: 9, with incremental migrations from v1 through v9." Please reconcile — either bump the earlier section to v10 (and add the v10 migration description) or correct line 342 back to v9.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ARCHITECTURE.md` at line 342, The document contains conflicting schema
versions: the "Schema version" paragraph (the earlier section mentioning "Schema
version: 9, with incremental migrations from v1 through v9") and the `DB` struct
summary (which says migrations "currently version 10"); choose one resolution
and make the texts consistent by either (A) bumping the earlier "Schema version"
section to v10 and adding a brief description of the v10 migration (what it
changes/why) and updating the incremental migrations list to include v10, or (B)
reverting the `DB` struct summary back to v9 so both sections state v9; update
the "Schema version" paragraph and the `DB` struct line together to match and
ensure the migration list and any mention of FTS5/WAL/migration version numbers
reflect the chosen version.
Comment on lines
+262
to
+263
| | `windowWidth` | Minimum 480px | Main window config in `main.go` (`MinWidth: 900`) and settings window re-centering logic | | ||
| | `windowHeight` | Minimum 400px | Main window config in `main.go` (`MinHeight: 600`) and settings window re-centering logic | |
Contributor
There was a problem hiding this comment.
Validation constraints contradict the cited values.
The windowWidth/windowHeight rows claim a minimum of 480/400 but cite main.go's MinWidth: 900 / MinHeight: 600 as the enforcement point. Those are also for the main window, while the documented windowWidth: 640 / windowHeight: 520 defaults are for the settings window. Either drop the contradictory enforcement reference or split into separate rows for main vs. settings windows so readers don't end up with conflicting numbers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CONFIGURATION.md` around lines 262 - 263, The table rows for
`windowWidth` and `windowHeight` are contradictory because they state minimums
of 480/400 but cite `main.go`'s `MinWidth: 900` / `MinHeight: 600` (which apply
to the main window) while the documented defaults (e.g., `windowWidth: 640`,
`windowHeight: 520`) are for the settings window; update the docs by either
removing the misleading enforcement reference or split into separate rows: one
for the main window (referencing `MinWidth`/`MinHeight` in `main.go` and the
main-window minima) and one for the settings window (showing the settings
defaults and any re-centering logic), and ensure the
`windowWidth`/`windowHeight` labels and descriptions explicitly indicate which
window each constraint applies to.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/components/SettingsPage.tsx (1)
328-328: Nit: the inlineeslint-disable-next-line react-hooks/exhaustive-depson Line 328 is misplaced.
react-hooks/exhaustive-depsonly fires on the dependency array (already suppressed at Line 335). ReadingsavedSettingsRef.currentinside a cleanup function does not trigger that rule, so this comment is a no-op and just adds noise. Safe to drop:♻️ Suggested cleanup
useEffect(() => { return () => { - // eslint-disable-next-line react-hooks/exhaustive-deps -- syncedRef.current is intentionally read in cleanup for latest value const { savedTheme: st, savedDensity: sd, savedUiFont: suf, savedMonoFont: smf, customThemes: ct } = savedSettingsRef.current; const custom = ct?.find(c => c.id === st); applyTheme(st, custom?.colors ?? null); applyDensity(sd); applyFonts(suf, smf); }; }, []); // eslint-disable-line react-hooks/exhaustive-deps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/SettingsPage.tsx` at line 328, Remove the misplaced inline eslint-disable-next-line react-hooks/exhaustive-deps that sits before the cleanup reading savedSettingsRef.current in SettingsPage.tsx; the rule only targets dependency arrays (the suppression already applied on the useEffect dependency array) so drop the no-op disable comment to clean up noise while leaving the existing suppression on the dependency array intact.frontend/src/hooks/useResizable.ts (2)
12-12: Add a JSDoc block for this non-obvious hook.Per the repo guideline for
frontend/src/hooks/**/*.ts, non-obvious hooks should carry JSDoc.useResizablehas several behavioral nuances worth documenting (axis/direction semantics, persisted bounds clamping on init, thathandleStartmust be wired to amousedownand receives the axis-relevant client coordinate, persistence happens on mouseup).📝 Suggested JSDoc
+/** + * Reusable resize hook backed by `localStorage`. + * + * Tracks a single dimension (`axis: 'x' | 'y'`) clamped to `[minSize, maxSize]`, + * initialized from `localStorage[storageKey]` (falling back to `defaultSize`). + * `direction` controls whether positive cursor movement grows (`1`) or shrinks + * (`-1`) the size — e.g. a top-edge resize handle uses `-1`. + * + * Wire `handleStart(e.clientX | e.clientY)` to a `mousedown` handler on the + * resize handle. The hook attaches `window` `mousemove`/`mouseup` listeners + * for the duration of the drag and persists the final size on mouseup. + */ export function useResizable(options: UseResizableOptions) {As per coding guidelines: "JSDoc-style blocks must be used on non-obvious hooks (e.g., useKeyboardShortcuts behavior)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useResizable.ts` at line 12, Add a JSDoc block above the useResizable function documenting its semantics: explain axis vs direction behavior, which coordinate is used for each axis, that persisted bounds are clamped on initialization, and that persistence occurs on mouseup; also call out that handleStart must be wired to a mousedown listener and that it receives the axis-relevant client coordinate (e.g., clientX for horizontal, clientY for vertical). Reference the exported useResizable function and its handleStart handler in the comment, list the main options in UseResizableOptions and the shape of the returned API, and include brief examples of wiring handleStart and expected lifecycle (start → move → mouseup persistence).
49-56: Consider extracting localStorage persistence to an effect rather than using a state updater.The current approach of reading
localStorageinside a state updater works but triggers the side effect during the commit phase. Under React 19 StrictMode, state updaters intentionally run twice in development to detect impure functions. While the localStorage writes are idempotent today, this pattern invites footguns if logic changes.The proposed fix using
sizeRef.currentwould be incorrect: the ref lags behind the actual state during the event handler (effects run after the commit phase). A safer approach is to use an effect that depends onsizeandisDragging:♻️ Suggested improvement
+ useEffect(() => { + if (!isDragging && size !== undefined) { + localStorage.setItem(storageKey, String(size)); + } + }, [size, isDragging, storageKey]);This removes I/O from the updater, runs after the commit phase, and eliminates side effects during re-render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useResizable.ts` around lines 49 - 56, The onMouseUp handler currently writes to localStorage inside the setSize state updater (in onMouseUp), which causes a side effect during the commit phase; remove localStorage.setItem from the setSize updater so setSize only updates state, and add an effect that persists size after commits: create a useEffect that watches [size, isDragging] and when isDragging is false write localStorage.setItem(storageKey, String(size)); keep the existing isDragging state (setIsDragging) and isDraggingRef toggles in the handlers but move all persistence out of setSize/onMouseUp into the effect to avoid I/O inside state updaters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/SettingsPage.tsx`:
- Line 328: Remove the misplaced inline eslint-disable-next-line
react-hooks/exhaustive-deps that sits before the cleanup reading
savedSettingsRef.current in SettingsPage.tsx; the rule only targets dependency
arrays (the suppression already applied on the useEffect dependency array) so
drop the no-op disable comment to clean up noise while leaving the existing
suppression on the dependency array intact.
In `@frontend/src/hooks/useResizable.ts`:
- Line 12: Add a JSDoc block above the useResizable function documenting its
semantics: explain axis vs direction behavior, which coordinate is used for each
axis, that persisted bounds are clamped on initialization, and that persistence
occurs on mouseup; also call out that handleStart must be wired to a mousedown
listener and that it receives the axis-relevant client coordinate (e.g., clientX
for horizontal, clientY for vertical). Reference the exported useResizable
function and its handleStart handler in the comment, list the main options in
UseResizableOptions and the shape of the returned API, and include brief
examples of wiring handleStart and expected lifecycle (start → move → mouseup
persistence).
- Around line 49-56: The onMouseUp handler currently writes to localStorage
inside the setSize state updater (in onMouseUp), which causes a side effect
during the commit phase; remove localStorage.setItem from the setSize updater so
setSize only updates state, and add an effect that persists size after commits:
create a useEffect that watches [size, isDragging] and when isDragging is false
write localStorage.setItem(storageKey, String(size)); keep the existing
isDragging state (setIsDragging) and isDraggingRef toggles in the handlers but
move all persistence out of setSize/onMouseUp into the effect to avoid I/O
inside state updaters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9ed7978-d633-4f48-a121-089bbebc4757
📒 Files selected for processing (5)
frontend/src/components/OutputPane.tsxfrontend/src/components/SettingsPage.tsxfrontend/src/hooks/useCopyToClipboard.tsfrontend/src/hooks/useResizable.tsfrontend/src/locales/en.json
✅ Files skipped from review due to trivial changes (1)
- frontend/src/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/hooks/useCopyToClipboard.ts
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.
Summary by CodeRabbit
Documentation
Bug Fixes
Refactor