Feat/separate command editor windows#35
Merged
Merged
Conversation
…memo - Export CommandDetail wrapped in React.memo to prevent inactive tab re-renders - Rely on default shallow prop comparison (no custom compare function) - React DevTools display name preserved automatically
…ories - Add makeHandleExecute, makeHandleRunInTerminal factories - Add makeHandleFillVariables, makeHandleDelete factories - Add makeHandleAddPreset, makeHandleRenamePreset, makeHandleDeletePreset factories - Add makeHandleReorderPresets, makeHandleSavePresetValues factories - Add makeHandleSaveScript factory with tabId parameter - Replace refreshSelectedCommand with parameterized refreshCommand - Update CommandDetail JSX to use factory functions keyed by activeTabId - Keep legacy handlers (handleExecute, handleFillVariables, handleDeleteCommand, handleSaveScriptDirect) for keyboard shortcuts, Sidebar, and AlertDialog - Remove dead code: handleManagePresets, handleAddPresetFromDetail, handleRenamePresetFromDetail, handleDeletePresetFromDetail, handleReorderPresetsFromDetail, handleSavePresetValuesFromDetail
…ctory refactor plan - Add 01-02-SUMMARY.md with execution details, decisions, and self-check
- Replace single CommandDetail mount with iteration over openTabs - Each tab gets its own .command-tab-shell wrapper with display toggle - Inactive tabs hidden via inline style display:none (DOM persists) - Welcome and loading views preserved as separate conditional blocks - Gate onResolvedValuesChange to active tab only (undefined for inactive) - Inactive tabs use draft-based variableDefinitionsToPrompts fallback - Per-tab FloatingSaveBar with tab-specific dirty state - Remove dead code: isNew and saveDisabled variables (now computed per-tab)
- Add comment explaining animation applies to entire main-content area - Note that inactive shells are display:none during fade - Verified no breaking :only-child or :first-child CSS selectors exist
Contributor
📝 WalkthroughWalkthroughReworks tab handling to mount a CommandDetail per open tab, introduces per-tab handler factories cached in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App.tsx
participant Detail as CommandDetail
participant API as Backend API
participant Toast
User->>App: Trigger action in tab (execute/save/run/fill/delete)
App->>Detail: Lookup per-tab handler from tabHandlerCacheRef and invoke
Detail->>API: Call backend with tabId
API-->>Detail: Respond success/error
Detail-->>App: Return result (tab-scoped)
App->>App: Update executingTabIdState (start/clear) and refresh only that tab's data
App->>Toast: Show success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (2)
frontend/src/App.tsx (1)
1036-1047: UnusedtinmakeHandleFillVariablesdeps.The callback body doesn't reference
t, so including it in the dep array needlessly recreates the factory on every language change and invalidatesReact.memoon downstreamCommandDetailmore often than required. Dropt(and the surrounding eslint disable isn't needed here since the deps would then be empty).♻️ Proposed fix
const makeHandleFillVariables = useCallback((tabId: string) => { return async (initialValues: Record<string, string>) => { if (isNewCommandTabId(tabId)) return; const vars = await GetVariables(tabId); setModal({ type: 'fillVariables', variables: vars || [], commandId: tabId, initialValues, }); }; - }, [t]); + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1036 - 1047, The useCallback factory makeHandleFillVariables unnecessarily lists the translation hook `t` in its dependency array even though the callback body doesn't reference it; remove `t` from the deps (use an empty array or only real dependencies) so makeHandleFillVariables is stable across language changes, and also remove the surrounding eslint-disable comment since it will no longer be needed; locate makeHandleFillVariables, which calls isNewCommandTabId, GetVariables, and setModal, and update its dependency array accordingly.frontend/src/components/CommandDetail.tsx (1)
1355-1355:React.memoeffectiveness is limited by unstable callback props.The memoization will only skip renders when all props are shallowly equal. However,
App.tsxpasses freshly-created callbacks on every render (e.g.onDraftChange={(partial) => updateDraft(tab.id, partial)}inline arrow, plusmakeHandleExecute(tab.id),makeHandleRunInTerminal(tab.id), etc.—eachmakeHandle*factory returns a new closure per invocation). As a result,CommandDetailwill re-render on virtually every App render regardless ofReact.memo.The DOM-persistence benefit from
display: noneper-tab mounts is still achieved, but if you wantReact.memoto actually skip renders, consider either:
- Caching the per-tab handler bundles in a
useRef<Map<string, Handlers>>keyed bytabId, or- Passing
tabIdas a prop and lifting the per-tab factory calls insideCommandDetail(so the child closes over a stableupdateDraft/factory function received once).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/CommandDetail.tsx` at line 1355, CommandDetail is wrapped with React.memo but still re-renders because App.tsx passes freshly-created callbacks (e.g., onDraftChange={(partial) => updateDraft(tab.id, partial)}, makeHandleExecute(tab.id), makeHandleRunInTerminal(tab.id)), so memoization is ineffective; fix by creating a stable per-tab handlers cache in App using a useRef<Map<string, Handlers>> keyed by tab.id (store/reuse the closures for makeHandleExecute, makeHandleRunInTerminal, onDraftChange, etc.) and pass the cached handler object reference to CommandDetail (or alternatively pass only tabId and move the makeHandle* calls into CommandDetail so it closes over stable updateDraft/factory functions), ensuring the props received by CommandDetail are referentially stable so React.memo can skip renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 1296-1300: handleSaveScriptDirect currently reads activeTabId but
the confirm dialog flow stores the intended tab in modal.tabId and the body in
pendingDirectSaveBodyRef, so a tab switch causes the wrong target; change
handleSaveScriptDirect signature to accept a tabId (e.g. handleSaveScriptDirect
= useCallback(async (tabId: string, scriptBody: string) => { ... }) ), use that
tabId to validate (isNewCommandTabId(tabId)) and to call
makeHandleSaveScript(tabId), and update the confirm dialog action (where
pendingDirectSaveBodyRef.current is used) to call
handleSaveScriptDirect(modal.tabId, pendingDirectSaveBodyRef.current); ensure
any other callers are updated accordingly and keep the existing
handleSaveTab(modal.tabId) branch consistent.
- Around line 1192-1249: Wrap all async Wails binding calls in the preset
factory functions (makeHandleAddPreset, makeHandleRenamePreset,
makeHandleDeletePreset, makeHandleReorderPresets, makeHandleSavePresetValues)
with try/catch and surface failures to the user via toast.error; specifically,
in each returned async handler, perform the
SavePreset/UpdatePreset/DeletePreset/ReorderPresets call inside try { await ...;
await refreshCommand(tabId); (any success toasts) } catch (err) {
console.error(..., err); toast.error(t('toast.someErrorKey') or err.message); }
(for reorder/others keep refreshCommand in finally or call it in both paths as
appropriate) so that backend rejections no longer silently fail and users get
feedback.
- Around line 1183-1190: refreshCommand currently unconditionally calls
setSelectedCommand(refreshed) which can overwrite whatever tab the user is
viewing; change it to only update selectedCommand when the refreshed command
matches the currently selected one (compare refreshed.id to the current
selectedCommand.id) so background refreshes don't clobber another tab, and
likewise update makeHandleReorderPresets so its setSelectedCommand((prev) =>
...) only applies the optimistic reordered presets when prev && prev.id ===
tabId (no-op otherwise); use the existing allCommandsRef/current selectedCommand
accessor to perform the id checks before calling setSelectedCommand.
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Around line 1036-1047: The useCallback factory makeHandleFillVariables
unnecessarily lists the translation hook `t` in its dependency array even though
the callback body doesn't reference it; remove `t` from the deps (use an empty
array or only real dependencies) so makeHandleFillVariables is stable across
language changes, and also remove the surrounding eslint-disable comment since
it will no longer be needed; locate makeHandleFillVariables, which calls
isNewCommandTabId, GetVariables, and setModal, and update its dependency array
accordingly.
In `@frontend/src/components/CommandDetail.tsx`:
- Line 1355: CommandDetail is wrapped with React.memo but still re-renders
because App.tsx passes freshly-created callbacks (e.g., onDraftChange={(partial)
=> updateDraft(tab.id, partial)}, makeHandleExecute(tab.id),
makeHandleRunInTerminal(tab.id)), so memoization is ineffective; fix by creating
a stable per-tab handlers cache in App using a useRef<Map<string, Handlers>>
keyed by tab.id (store/reuse the closures for makeHandleExecute,
makeHandleRunInTerminal, onDraftChange, etc.) and pass the cached handler object
reference to CommandDetail (or alternatively pass only tabId and move the
makeHandle* calls into CommandDetail so it closes over stable
updateDraft/factory functions), ensuring the props received by CommandDetail are
referentially stable so React.memo can skip renders.
🪄 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: 2430a785-f1f2-4553-9298-ad676fa96bda
⛔ Files ignored due to path filters (2)
.planning/PROJECT.mdis excluded by!.planning/*.planning/ROADMAP.mdis excluded by!.planning/*
📒 Files selected for processing (2)
frontend/src/App.tsxfrontend/src/components/CommandDetail.tsx
- handleSaveScriptDirect now accepts explicit tabId to prevent wrong-target saves during tab switches - Add try/catch + toast.error to all preset factory functions (add/rename/delete/reorder/save-values) - Gate refreshCommand and optimistic reorder updates to only affect matching selectedCommand - Remove unnecessary 't' dependency from makeHandleFillVariables - Add tabHandlerCacheRef to provide stable callback references for React.memo effectiveness
…rm dialog Gates setSelectedCommand inside makeHandleSaveScript so it only updates when the saved tab is still active. Without this, switching tabs while the variable-removal confirm dialog is open would cause the UI to snap back to the saved tab after the dialog confirms.
…nt UI jumps on tab switch When saving a command (new or existing), if the user switches tabs while the async save is in flight, the UI was snapping back to the saved command after the operation completed. This gates both setActiveTabId and setSelectedCommand with activeTabIdRef.current === tabId so the UI only updates when the user is still viewing the tab that was saved.
…uesChange The resolvedValues useMemo in CommandDetail depends on the 'variables' prop, which is recreated on every App.tsx render. This caused onResolvedValuesChange to fire with a new object reference each render, triggering App re-render and creating an infinite loop (Maximum update depth exceeded). Added a shallow-comparison guard using a ref to only call onResolvedValuesChange when the actual key/value pairs change, breaking the loop while preserving correct state synchronization.
…s prop, and shared isExecuting prop
Owner
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 1368-1373: The post-commit useEffect invalidates handlerGenRef and
tabHandlerCacheRef too late, so renders that triggered factory-dep changes
(e.g., `t`) still forward stale handlers; fix by making invalidation synchronous
during render: create a memoized generation key (useMemo) based on the factory
deps (`makeHandleExecute`, `makeHandleRunInTerminal`, `makeHandleFillVariables`,
`makeHandleDelete`, `makeHandleRenamePreset`, `makeHandleDeletePreset`,
`makeHandleAddPreset`, `makeHandleSavePresetValues`, `makeHandleReorderPresets`,
`makeHandleSaveScript`) and compare it to a `lastHandlerGenRef` to clear
`tabHandlerCacheRef.current` and update `lastHandlerGenRef.current` when they
differ (or alternately refactor each tab into a child component that computes
handlers with useMemo keyed by `tabId` + factories); this ensures the cache is
invalidated synchronously so the loop that looks up handlers (and components
using `React.memo`) never sees stale closures.
- Around line 1606-1608: The executingTabId is incorrectly derived from
activeTabId causing isExecuting to follow tab switches; change executingTabId to
be driven by a state variable (e.g., executingTabIdState via useState) instead
of deriving it from activeTabId, and update the places that currently set/clear
executingTabIdRef (where execution starts/stops) to set/clear this new state as
well (you can keep executingTabIdRef for imperative uses but ensure the JSX and
CommandDetail props read the state value); ensure isExecuting ?
executingTabIdState : null is used in the render so the executing tab remains
pinned during execution and React.memo optimizations are preserved.
🪄 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: 5a274c6d-1d56-4139-a8bb-d66a81c4941e
📒 Files selected for processing (2)
frontend/src/App.tsxfrontend/src/components/CommandDetail.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/CommandDetail.tsx
…ingTabId state - WR-01: Replace useEffect-based handler cache invalidation with synchronous render-time comparison of factory refs via prevFactoriesRef, ensuring stale closures are never returned between factory-dep changes and effect firing - WR-03: Add executingTabIdState (useState) alongside executingTabIdRef so the executing tab remains pinned during execution even when user switches tabs, instead of incorrectly following activeTabId
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/App.tsx (1)
1371-1408: Consider reducing boilerplate in factory-generation bookkeeping.The hand-rolled 10-field object + 10-condition identity check is verbose and easy to get out of sync when a new
makeHandle*factory is added. Using an array simplifies the logic while maintaining the same effect:♻️ Terser encoding
- const prevFactoriesRef = useRef<Record<string, ((...args: unknown[]) => unknown) | null>>({ - makeHandleExecute: null, - makeHandleRunInTerminal: null, - makeHandleFillVariables: null, - makeHandleDelete: null, - makeHandleRenamePreset: null, - makeHandleDeletePreset: null, - makeHandleAddPreset: null, - makeHandleSavePresetValues: null, - makeHandleReorderPresets: null, - makeHandleSaveScript: null, - }); - - const pf = prevFactoriesRef.current; - if ( - pf.makeHandleExecute !== makeHandleExecute || - pf.makeHandleRunInTerminal !== makeHandleRunInTerminal || - /* ...8 more... */ - pf.makeHandleSaveScript !== makeHandleSaveScript - ) { - tabHandlerCacheRef.current.clear(); - pf.makeHandleExecute = makeHandleExecute; - /* ...9 more assignments... */ - } + const factories = [ + makeHandleExecute, makeHandleRunInTerminal, makeHandleFillVariables, + makeHandleDelete, makeHandleRenamePreset, makeHandleDeletePreset, + makeHandleAddPreset, makeHandleSavePresetValues, makeHandleReorderPresets, + makeHandleSaveScript, + ] as const; + const prevFactoriesRef = useRef<readonly unknown[]>([]); + if ( + prevFactoriesRef.current.length !== factories.length || + factories.some((f, i) => f !== prevFactoriesRef.current[i]) + ) { + tabHandlerCacheRef.current.clear(); + prevFactoriesRef.current = factories; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1371 - 1408, Replace the hand-rolled 10-field object and long equality check with a compact array-driven approach: turn prevFactoriesRef.current (and local pf) into a map updated by iterating a string array of factory names (e.g. ["makeHandleExecute","makeHandleRunInTerminal",..., "makeHandleSaveScript"]), check if any current function identity differs via Array.prototype.some comparing pf[name] vs the corresponding variable (e.g. using (name: string) => pf[name] !== (eval? or better map name->variable)), and if any changed call tabHandlerCacheRef.current.clear() then update pf[name] = the new factory for each name in the same loop; keep the same symbols (prevFactoriesRef, pf, tabHandlerCacheRef, and the makeHandle* identifiers) so behavior is identical but boilerplate is removed and adding new makeHandle* entries requires only adding the name to the array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 1022-1034: makeHandleDelete currently closes over a stale closeTab
because its deps are only [loadData, t]; update the factory to avoid the stale
closure by either (a) adding closeTab (and any functions it closes over like
finalizeCloseTab) to the useCallback dependency array for makeHandleDelete, or
(b) ref-forwarding closeTab into makeHandleDelete (change its signature to
makeHandleDelete(closeTab) or makeHandleDelete(tabId, closeTab)) so the current
closeTab is used when the handler is created; also ensure DeleteCommand errors
surface to the user by invoking toast.error(...) inside the catch in
makeHandleDelete (in addition to console.error) to match the app’s Wails async
call guidelines.
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Around line 1371-1408: Replace the hand-rolled 10-field object and long
equality check with a compact array-driven approach: turn
prevFactoriesRef.current (and local pf) into a map updated by iterating a string
array of factory names (e.g. ["makeHandleExecute","makeHandleRunInTerminal",...,
"makeHandleSaveScript"]), check if any current function identity differs via
Array.prototype.some comparing pf[name] vs the corresponding variable (e.g.
using (name: string) => pf[name] !== (eval? or better map name->variable)), and
if any changed call tabHandlerCacheRef.current.clear() then update pf[name] =
the new factory for each name in the same loop; keep the same symbols
(prevFactoriesRef, pf, tabHandlerCacheRef, and the makeHandle* identifiers) so
behavior is identical but boilerplate is removed and adding new makeHandle*
entries requires only adding the name to the array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…rive factory cache invalidation
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/App.tsx (2)
1384-1407: Type erasure onfactoriesrecord weakens safety for no runtime benefit.The
Record<(typeof factoryNames)[number], (...args: unknown[]) => unknown>annotation forces every factory to be treated as(...args: unknown[]) => unknown. The only operation actually performed here is reference equality on function identities — no arguments are applied — so an opaqueobject/unknownkeyed map would retain identity comparison while skipping the misleading callable signature.♻️ Proposed tightening
- const factories: Record<(typeof factoryNames)[number], (...args: unknown[]) => unknown> = { + const factories: Record<(typeof factoryNames)[number], unknown> = { makeHandleExecute, makeHandleRunInTerminal, makeHandleFillVariables, makeHandleDelete, makeHandleRenamePreset, makeHandleDeletePreset, makeHandleAddPreset, makeHandleSavePresetValues, makeHandleReorderPresets, makeHandleSaveScript, }; - const prevFactoriesRef = useRef<Record<string, ((...args: unknown[]) => unknown) | null>>( + const prevFactoriesRef = useRef<Record<string, unknown>>( Object.fromEntries(factoryNames.map((name) => [name, null])) );Only reference equality is used downstream, so
unknownis sufficient and avoids implying these can be called withunknown[].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1384 - 1407, The factories Record is overly specific about callable signatures; change its type from Record<(typeof factoryNames)[number], (...args: unknown[]) => unknown> to a non-callable identity-preserving type (e.g., Record<(typeof factoryNames)[number], unknown>) and update prevFactoriesRef to match (e.g., Record<string, unknown | null>) so the identity comparison pf[name] !== factories[name] remains valid; touch the symbols factoryNames, factories, prevFactoriesRef, and tabHandlerCacheRef to keep typings consistent without implying these entries are invocable functions.
1709-1747: New-command tab placeholder recreated every App render — defeatsReact.memoon that tab.For new-command tabs,
command = makePlaceholderCommand(tab.id, draft?.categoryId)produces a brand‑new object on every App render, so thecommandprop handed to the memoizedCommandDetailchanges identity every time (even whentab.idanddraft.categoryIdare unchanged). This undoes the memoization win the surrounding handler cache andtabVariablesMapare designed to preserve — any unrelated App state change (e.g.,executingTabIdStatetoggling on a different tab,selectedRecordupdates, pane-open toggles) will cause the new-command tab'sCommandDetailto re-render.Existing (saved) tabs are fine because
allCommandsRef.current.find(...)returns the same object reference untilloadData()actually produces a new array.♻️ Proposed refactor — memoize placeholder commands per tab
const tabVariablesMap = useMemo(() => { const map: Record<string, VarPromptType[]> = {}; for (const tab of openTabs) { if (tab.id === '__welcome__') continue; const draft = tabDrafts[tab.id]; if (draft && !isNewCommandTabId(tab.id)) { map[tab.id] = variableDefinitionsToPrompts(draft.variables); } } return map; }, [tabDrafts, openTabs]); + + // Stable placeholder commands for new-command tabs so React.memo isn't bypassed + // on every App render. + const tabPlaceholderMap = useMemo(() => { + const map: Record<string, Command> = {}; + for (const tab of openTabs) { + if (!isNewCommandTabId(tab.id)) continue; + const draft = tabDrafts[tab.id]; + map[tab.id] = makePlaceholderCommand(tab.id, draft?.categoryId); + } + return map; + }, [openTabs, tabDrafts]);- const command = isTabNew - ? makePlaceholderCommand(tab.id, draft?.categoryId) - : allCommandsRef.current.find((c) => c.id === tab.id) ?? null; + const command = isTabNew + ? tabPlaceholderMap[tab.id] ?? null + : allCommandsRef.current.find((c) => c.id === tab.id) ?? null;If
makePlaceholderCommandonly actually depends oncategoryId, you can further narrow the memo dep todraft?.categoryIdper tab.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 1709 - 1747, The placeholder command object created in the render (`command = makePlaceholderCommand(tab.id, draft?.categoryId)` for isTabNew tabs) is re-created every render breaking React.memo; fix by memoizing the placeholder per tab id (e.g., store in a ref map like placeholderCommandRef.current keyed by tab.id) and only recreate it when the inputs change (draft?.categoryId or tab.id), then use that cached object as `command`; keep existing logic for saved tabs (allCommandsRef.current.find(...)) and ensure cache is updated/invalidated when categoryId changes or the tab is closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/App.tsx`:
- Around line 1338-1356: The catch block in makeHandleSaveScript currently only
logs errors to console and should show a user-facing toast like the other
handlers; update the catch to call toast.error(...) with a localized message
(e.g., t('toast.commandSaveFailed') if available, or String(err)) and keep
console.error for debugging, ensuring failures from UpdateCommand/GetScriptBody
surface to the user; locate makeHandleSaveScript and modify its try/catch to
mirror the other factories' error handling (use toast.error and the err value).
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Around line 1384-1407: The factories Record is overly specific about callable
signatures; change its type from Record<(typeof factoryNames)[number], (...args:
unknown[]) => unknown> to a non-callable identity-preserving type (e.g.,
Record<(typeof factoryNames)[number], unknown>) and update prevFactoriesRef to
match (e.g., Record<string, unknown | null>) so the identity comparison pf[name]
!== factories[name] remains valid; touch the symbols factoryNames, factories,
prevFactoriesRef, and tabHandlerCacheRef to keep typings consistent without
implying these entries are invocable functions.
- Around line 1709-1747: The placeholder command object created in the render
(`command = makePlaceholderCommand(tab.id, draft?.categoryId)` for isTabNew
tabs) is re-created every render breaking React.memo; fix by memoizing the
placeholder per tab id (e.g., store in a ref map like
placeholderCommandRef.current keyed by tab.id) and only recreate it when the
inputs change (draft?.categoryId or tab.id), then use that cached object as
`command`; keep existing logic for saved tabs (allCommandsRef.current.find(...))
and ensure cache is updated/invalidated when categoryId changes or the tab is
closed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1338
to
1356
| try { | ||
| await UpdateCommand(tabId, d.title.trim(), d.description.trim(), strippedBody, d.categoryId, d.tags.map(tag => tag.trim()).filter(Boolean), vars, d.workingDir); | ||
| await loadData(); | ||
| const cmd = allCommandsRef.current.find(c => c.id === tabId); | ||
| if (cmd) { | ||
| const body = await GetScriptBody(cmd.id); | ||
| const saved = draftFromCommand(cmd, body); | ||
| setTabDrafts(prev => ({ ...prev, [tabId]: saved })); | ||
| setTabBaselines(prev => ({ ...prev, [tabId]: cloneDraft(saved) })); | ||
| // Only update selectedCommand if this tab is still active, | ||
| // so tab switches during confirm dialogs don't clobber the UI. | ||
| if (activeTabIdRef.current === tabId) { | ||
| setSelectedCommand(cmd); | ||
| } | ||
| } | ||
| toast.success(t('toast.commandSaved')); | ||
| } catch (err) { | ||
| console.error('Failed to save script:', err); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing toast.error in makeHandleSaveScript catch — inconsistent with peer preset factories.
All other newly added factories (makeHandleAddPreset, makeHandleRenamePreset, makeHandleDeletePreset, makeHandleReorderPresets, makeHandleSavePresetValues, makeHandleDelete, makeHandleRunInTerminal) surface failures via toast.error. makeHandleSaveScript only logs to console.error, so a backend rejection of UpdateCommand/GetScriptBody during the preview-panel "save script" path is silent to the user — no pending changes are cleared, but no feedback is given either.
As per coding guidelines: "Async Wails calls in React must use .then/.catch or try/catch patterns with user feedback via toast from sonner."
🛡 Proposed fix
} catch (err) {
console.error('Failed to save script:', err);
+ toast.error(t('toast.commandSaveFailed') ?? String(err));
}(Uses an existing toast.commandSaveFailed key if available; otherwise String(err) — align with whatever key the i18n bundle exposes for save failures.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/App.tsx` around lines 1338 - 1356, The catch block in
makeHandleSaveScript currently only logs errors to console and should show a
user-facing toast like the other handlers; update the catch to call
toast.error(...) with a localized message (e.g., t('toast.commandSaveFailed') if
available, or String(err)) and keep console.error for debugging, ensuring
failures from UpdateCommand/GetScriptBody surface to the user; locate
makeHandleSaveScript and modify its try/catch to mirror the other factories'
error handling (use toast.error and the err value).
This was referenced Apr 27, 2026
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
New Features
Bug Fixes
Performance