refactor: decompose AutoRun into modular directory structure#698
Merged
refactor: decompose AutoRun into modular directory structure#698
Conversation
…racted components and hooks Moves AutoRun.tsx (2,290 lines) into AutoRun/ directory module and extracts 5 presentational components, 5 hooks, and a types file. Shell reduced to 775 lines (−66%). Extracted components: - AutoRunToolbar: top controls (expand, edit/preview, run/stop, exchange, wizard, help) - AutoRunErrorBanner: batch error pause banner with resume/abort - AutoRunBottomPanel: task counts, save/revert, token count, owns ResizeObserver - AutoRunEmptyStates: NoFolderState + EmptyFolderState - AutoRunAttachmentsPanel: collapsible image attachment previews Extracted hooks: - useAutoRunContentSync: content state machine, save/revert, dirty tracking - useAutoRunSearch: search state, debounced match counting, navigation - useAutoRunKeyboard: textarea keyboard handler (Tab, undo/redo, save, checkbox) - useAutoRunMarkdown: prose styles, task counts, token count, remark plugins, components - useAutoRunScrollSync: mode switching with scroll position preservation Also consolidates 6 satellite files into the AutoRun/ directory via git mv, updates all import paths in consumers (RightPanel, AppUtilityModals, tests). 102 new tests across 6 test files. All 24,091 tests pass, 0 regressions.
📝 WalkthroughWalkthroughRestructures AutoRun into a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Toolbar as AutoRunToolbar
participant ContentSync as useAutoRunContentSync
participant FS as window.maestro.autorun
participant Runner as BatchRunnerModal
Toolbar->>ContentSync: onRunClick (if isDirty -> await handleSave())
ContentSync->>FS: writeDoc(folderPath, selectedFile, content)
FS-->>ContentSync: write success
ContentSync-->>Toolbar: save resolved
Toolbar->>Runner: openBatchRunner()
Runner-->>Toolbar: runner opened
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR decomposes the 2,290-line Key findings (all P2):
Confidence Score: 5/5Safe to merge — all findings are P2 style/best-practice items with no current runtime defects. Every identified issue is P2: a fire-and-forget async call guarded by modal interaction in practice, a missing useCallback dep with a stable parent callback in practice, a dead interface parameter, and an inconsistent import style. None represent broken logic, data loss, or a broken user path. The refactor is a pure structural decomposition with no behavior changes, 102 new tests, and all 24,091 tests passing. src/renderer/components/AutoRun/AutoRunToolbar.tsx (unawaited async save) and src/renderer/hooks/batch/useAutoRunScrollSync.ts (missing dep in switchMode) are the two files worth a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
AR["AutoRun shell ~775 lines\nAutoRun/AutoRun.tsx"] --> CS["useAutoRunContentSync\nsave · revert · dirty tracking · external draft sync"]
AR --> SS["useAutoRunScrollSync\nmode switching · scroll preservation"]
AR --> SR["useAutoRunSearch\ndebounced match count · prev/next navigation"]
AR --> KB["useAutoRunKeyboard\nTab · undo/redo · save · checkbox · list continuation"]
AR --> MD["useAutoRunMarkdown\nprose styles · task counts · token count · remark plugins · components"]
AR --> TB["AutoRunToolbar\nexpand · edit/preview · run/stop · exchange · wizard · help"]
AR --> EB["AutoRunErrorBanner\nbatch error pause · resume / abort"]
AR --> BP["AutoRunBottomPanel\ntask counts · save/revert · token count · ResizeObserver"]
AR --> ES["AutoRunEmptyStates\nNoFolderState · EmptyFolderState"]
AR --> AP["AutoRunAttachmentsPanel\ncollapsible image attachment previews"]
Reviews (1): Last reviewed commit: "refactor: decompose AutoRun into modular..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+136
to
+142
| onClick={() => { | ||
| // Save before opening batch runner if dirty | ||
| if (isDirty) { | ||
| onSave(); | ||
| } | ||
| onOpenBatchRunner?.(); | ||
| }} |
There was a problem hiding this comment.
onSave() not awaited before opening batch runner
onSave returns Promise<void> (it performs an async IPC write via maestro.autorun.writeDoc). Calling it without await means onOpenBatchRunner fires immediately while the file write is still in-flight. If the batch runner reads the document before the write completes, it could process stale content.
In most real-world flows the user must still interact with the batch-runner modal before the run begins, so the write has time to finish—but the race is still possible if the modal auto-starts. Awaiting the save makes the intent explicit and removes the race entirely.
Suggested change
| onClick={() => { | |
| // Save before opening batch runner if dirty | |
| if (isDirty) { | |
| onSave(); | |
| } | |
| onOpenBatchRunner?.(); | |
| }} | |
| onClick={async () => { | |
| // Save before opening batch runner if dirty | |
| if (isDirty) { | |
| await onSave(); | |
| } | |
| onOpenBatchRunner?.(); | |
| }} |
Note: the button's onClick handler must be declared async to use await.
Comment on lines
+47
to
+91
| const switchMode = useCallback( | ||
| (newMode: 'edit' | 'preview') => { | ||
| if (newMode === mode) return; | ||
|
|
||
| // Calculate scroll percentage from current mode to apply to new mode | ||
| let scrollPercent = 0; | ||
| if (mode === 'edit' && textareaRef.current) { | ||
| const { scrollTop, scrollHeight, clientHeight } = textareaRef.current; | ||
| const maxScroll = scrollHeight - clientHeight; | ||
| scrollPercent = maxScroll > 0 ? scrollTop / maxScroll : 0; | ||
| } else if (mode === 'preview' && previewRef.current) { | ||
| const { scrollTop, scrollHeight, clientHeight } = previewRef.current; | ||
| const maxScroll = scrollHeight - clientHeight; | ||
| scrollPercent = maxScroll > 0 ? scrollTop / maxScroll : 0; | ||
| } | ||
|
|
||
| setMode(newMode); | ||
|
|
||
| // Apply scroll percentage to the new mode after it renders | ||
| requestAnimationFrame(() => { | ||
| if (newMode === 'preview' && previewRef.current) { | ||
| const { scrollHeight, clientHeight } = previewRef.current; | ||
| const maxScroll = scrollHeight - clientHeight; | ||
| const newScrollTop = Math.round(scrollPercent * maxScroll); | ||
| previewRef.current.scrollTop = newScrollTop; | ||
| previewScrollPosRef.current = newScrollTop; | ||
| } else if (newMode === 'edit' && textareaRef.current) { | ||
| const { scrollHeight, clientHeight } = textareaRef.current; | ||
| const maxScroll = scrollHeight - clientHeight; | ||
| const newScrollTop = Math.round(scrollPercent * maxScroll); | ||
| textareaRef.current.scrollTop = newScrollTop; | ||
| editScrollPosRef.current = newScrollTop; | ||
| } | ||
| }); | ||
|
|
||
| if (onStateChange) { | ||
| onStateChange({ | ||
| mode: newMode, | ||
| cursorPosition: textareaRef.current?.selectionStart || 0, | ||
| editScrollPos: textareaRef.current?.scrollTop || 0, | ||
| previewScrollPos: previewRef.current?.scrollTop || 0, | ||
| }); | ||
| } | ||
| }, | ||
| [mode, onStateChange] |
There was a problem hiding this comment.
setMode missing from switchMode dependency array
setMode is destructured from params and called inside switchMode, but it is not listed in the useCallback dependency array ([mode, onStateChange]). If the parent's onModeChange prop changes identity (causing setMode to be re-created) while mode and onStateChange stay the same, switchMode will silently call the stale setMode, potentially dispatching mode changes to the wrong handler.
Suggested change
| const switchMode = useCallback( | |
| (newMode: 'edit' | 'preview') => { | |
| if (newMode === mode) return; | |
| // Calculate scroll percentage from current mode to apply to new mode | |
| let scrollPercent = 0; | |
| if (mode === 'edit' && textareaRef.current) { | |
| const { scrollTop, scrollHeight, clientHeight } = textareaRef.current; | |
| const maxScroll = scrollHeight - clientHeight; | |
| scrollPercent = maxScroll > 0 ? scrollTop / maxScroll : 0; | |
| } else if (mode === 'preview' && previewRef.current) { | |
| const { scrollTop, scrollHeight, clientHeight } = previewRef.current; | |
| const maxScroll = scrollHeight - clientHeight; | |
| scrollPercent = maxScroll > 0 ? scrollTop / maxScroll : 0; | |
| } | |
| setMode(newMode); | |
| // Apply scroll percentage to the new mode after it renders | |
| requestAnimationFrame(() => { | |
| if (newMode === 'preview' && previewRef.current) { | |
| const { scrollHeight, clientHeight } = previewRef.current; | |
| const maxScroll = scrollHeight - clientHeight; | |
| const newScrollTop = Math.round(scrollPercent * maxScroll); | |
| previewRef.current.scrollTop = newScrollTop; | |
| previewScrollPosRef.current = newScrollTop; | |
| } else if (newMode === 'edit' && textareaRef.current) { | |
| const { scrollHeight, clientHeight } = textareaRef.current; | |
| const maxScroll = scrollHeight - clientHeight; | |
| const newScrollTop = Math.round(scrollPercent * maxScroll); | |
| textareaRef.current.scrollTop = newScrollTop; | |
| editScrollPosRef.current = newScrollTop; | |
| } | |
| }); | |
| if (onStateChange) { | |
| onStateChange({ | |
| mode: newMode, | |
| cursorPosition: textareaRef.current?.selectionStart || 0, | |
| editScrollPos: textareaRef.current?.scrollTop || 0, | |
| previewScrollPos: previewRef.current?.scrollTop || 0, | |
| }); | |
| } | |
| }, | |
| [mode, onStateChange] | |
| [mode, setMode, onStateChange] |
| handleUndo: () => void; | ||
| handleRedo: () => void; | ||
| isDirty: boolean; | ||
| handleSave: () => Promise<void>; |
There was a problem hiding this comment.
scheduleUndoSnapshot parameter is declared but never used
scheduleUndoSnapshot is part of the public UseAutoRunKeyboardParams interface and is destructured as _scheduleUndoSnapshot (the _ prefix conventionally signals "intentionally unused"). Keeping a dead parameter in the interface misleads callers into thinking it has an effect.
If this is dead code left over from the extraction, remove it from both the interface and the destructuring. If it is intentionally reserved for future use, a code comment explaining that intent would help.
Comment on lines
+17
to
+31
| import { AutoRunToolbar } from './AutoRunToolbar'; | ||
| import { AutoRunErrorBanner } from './AutoRunErrorBanner'; | ||
| import { AutoRunBottomPanel } from './AutoRunBottomPanel'; | ||
| import { NoFolderState, EmptyFolderState } from './AutoRunEmptyStates'; | ||
| import { AutoRunAttachmentsPanel } from './AutoRunAttachmentsPanel'; | ||
| import { useTemplateAutocomplete, useAutoRunUndo, useAutoRunImageHandling } from '../../hooks'; | ||
| import { TemplateAutocompleteDropdown } from '../TemplateAutocompleteDropdown'; | ||
| import type { AutoRunProps, AutoRunHandle } from './types'; | ||
| import { useAutoRunContentSync } from '../../hooks/batch/useAutoRunContentSync'; | ||
| import { useAutoRunSearch } from '../../hooks/batch/useAutoRunSearch'; | ||
| import { useAutoRunKeyboard } from '../../hooks/batch/useAutoRunKeyboard'; | ||
| import { useAutoRunMarkdown } from '../../hooks/batch/useAutoRunMarkdown'; | ||
| import { useAutoRunScrollSync } from '../../hooks/batch/useAutoRunScrollSync'; | ||
|
|
||
| // Inner implementation component |
There was a problem hiding this comment.
The five extracted hooks are imported directly from their individual source files rather than through src/renderer/hooks/batch/index.ts, which already re-exports all five. This is inconsistent with how every other batch hook in the same file is consumed (e.g. useAutoRunUndo, useAutoRunImageHandling both come from '../../hooks').
Suggested change
| import { AutoRunToolbar } from './AutoRunToolbar'; | |
| import { AutoRunErrorBanner } from './AutoRunErrorBanner'; | |
| import { AutoRunBottomPanel } from './AutoRunBottomPanel'; | |
| import { NoFolderState, EmptyFolderState } from './AutoRunEmptyStates'; | |
| import { AutoRunAttachmentsPanel } from './AutoRunAttachmentsPanel'; | |
| import { useTemplateAutocomplete, useAutoRunUndo, useAutoRunImageHandling } from '../../hooks'; | |
| import { TemplateAutocompleteDropdown } from '../TemplateAutocompleteDropdown'; | |
| import type { AutoRunProps, AutoRunHandle } from './types'; | |
| import { useAutoRunContentSync } from '../../hooks/batch/useAutoRunContentSync'; | |
| import { useAutoRunSearch } from '../../hooks/batch/useAutoRunSearch'; | |
| import { useAutoRunKeyboard } from '../../hooks/batch/useAutoRunKeyboard'; | |
| import { useAutoRunMarkdown } from '../../hooks/batch/useAutoRunMarkdown'; | |
| import { useAutoRunScrollSync } from '../../hooks/batch/useAutoRunScrollSync'; | |
| // Inner implementation component | |
| import { useAutoRunContentSync, useAutoRunSearch, useAutoRunKeyboard, useAutoRunMarkdown, useAutoRunScrollSync } from '../../hooks/batch'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
src/renderer/components/AutoRun/AutoRunAttachmentsPanel.tsx (1)
32-57: Add collapse semantics to the attachments toggle button.Line 32 toggles visibility, but the control doesn’t expose expanded state to assistive tech. Add
aria-expanded+aria-controls(and anidon the panel container).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun/AutoRunAttachmentsPanel.tsx` around lines 32 - 57, The toggle button for attachments (the button using onToggleExpanded and the attachmentsExpanded state) needs accessible collapse semantics: add aria-expanded={attachmentsExpanded} to the button and add aria-controls pointing to the panel container; then give the panel container div (the one rendering attachmentsList/ImagePreview) a matching id (e.g., "attachments-panel" or a generated unique id) so assistive tech can associate the button with the collapsible region. Ensure the id referenced by aria-controls matches the panel id and only present aria-controls when the panel exists.src/renderer/components/AutoRun/AttachmentImage.tsx (1)
66-67: Replacetheme: anywithThemefor type safety.Using
anyhere removes compile-time validation for theme token usage. This should use the sharedThemetype.Also applies to: 265-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun/AttachmentImage.tsx` around lines 66 - 67, The prop typing for "theme" in AttachmentImage-related definitions currently uses "any"; replace those with the proper Theme type to restore compile-time validation: import the shared Theme type (e.g., from your theme module or the library you use) and update the prop/interface declarations that declare "theme: any" (the AttachmentImage component props and the other occurrence around lines referenced) to "theme: Theme"; ensure any places referencing those prop types still compile by updating imports to include Theme and removing the any usage.src/renderer/components/AutoRun/AutoRunErrorBanner.tsx (1)
23-29: Expose this runtime error state as an ARIA live alert.The paused/error banner should be announced when it appears. Consider
role="alert"(oraria-live="assertive") on the wrapper at Line 23.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/AutoRun/AutoRunErrorBanner.tsx` around lines 23 - 29, The error banner wrapper div in AutoRunErrorBanner (the <div> with className "mx-2 mb-2 p-3 rounded-lg border") should expose the runtime error to assistive tech by adding an ARIA live region; update that wrapper to include either role="alert" or aria-live="assertive" (and optionally aria-atomic="true") so the paused/error banner is announced when it appears.src/__tests__/renderer/components/AutoRun/AutoRunToolbar.test.tsx (1)
204-213: Add order assertion to prevent call-sequence regressions.The test checks only that both
onSaveandonOpenBatchRunnerare called once, not thatonSaveis called first. If a refactor reversed the call order, the test would still pass. UsetoHaveBeenNthCalledWith()or inspectinvocationCallOrderto enforce the intended sequence:expect(onSave).toHaveBeenNthCalledWith(1); expect(onOpenBatchRunner).toHaveBeenNthCalledWith(2);Alternatively, verify the order by checking mock call indices explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/AutoRun/AutoRunToolbar.test.tsx` around lines 204 - 213, Update the test in AutoRunToolbar.test.tsx to assert call order in addition to call counts: after triggering the Run button, assert that onSave was invoked before onOpenBatchRunner by using mock call-order assertions (e.g., expect(onSave).toHaveBeenNthCalledWith(1, ...) or compare onSave.mock.invocationCallOrder[0] < onOpenBatchRunner.mock.invocationCallOrder[0]) referencing the onSave and onOpenBatchRunner mocks used in the test for AutoRunToolbar; keep the existing call-count assertions and add the order assertion immediately after them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AutoRun/AttachmentImage.tsx`:
- Around line 12-13: In the AttachmentImage component, guard every use of
decodeURIComponent (the decodedSrc assignment and the other two decode calls
used in the effect/render) against malformed URI sequences by wrapping them in a
try/catch (or creating a small safeDecodeURIComponent helper) so a thrown
URIError falls back to the original src (or a sanitized variant) instead of
crashing; update the decodedSrc initialization and the two other
decodeURIComponent usages inside AttachmentImage to use this safe wrapper and
ensure downstream logic uses the fallback value.
- Around line 70-86: The component memoizes initialState via
getInitialImageState(src, folderPath) but never syncs local state vars when
initialState changes, causing stale dataUrl/error/loading/filename; add a
useEffect that watches initialState (or [src, folderPath]) and updates
setDataUrl(initialState.dataUrl), setError(null or initialState.error if
present), setLoading(initialState.loading), and
setFilename(initialState.filename) (preserving the current early-return logic in
the existing useEffect that skips async loading when initialState.dataUrl
exists) so the component reflects new props immediately; reference initialState,
getInitialImageState, setDataUrl, setError, setLoading, setFilename, and the
existing useEffect for where to place this sync.
In `@src/renderer/components/AutoRun/AutoRun.tsx`:
- Around line 583-592: The previewRef is attached to the wrong element so
useAutoRunScrollSync reads a non-scrolling node; move the previewRef and
onScroll={handlePreviewScroll} from the inner preview div to the outer element
that has overflow-y-auto (or alternatively move the overflow-y-auto style onto
the inner preview div) so that previewRef.current is the actual scroll container
used by useAutoRunScrollSync and the markdown anchor scroll/auto-follow logic
fires correctly; update references to previewRef, handlePreviewScroll, and
useAutoRunScrollSync accordingly.
- Around line 728-770: The memo comparator on AutoRun (AutoRun =
memo(AutoRunInner, ...)) is missing props that AutoRunInner reads—add
comparisons for batchRunState.worktreeActive and batchRunState.lockedDocuments
(used to derive isLocked), batchRunState.errorDocumentIndex and
documents/documentList (used to compute errorDocumentName), externalLocalContent
and externalSavedContent, and documentTree and documentTaskCounts; alternatively
remove the custom comparator so React does shallow prop comparison. Update the
comparator to explicitly compare those fields (e.g.,
prevProps.batchRunState?.worktreeActive ===
nextProps.batchRunState?.worktreeActive,
prevProps.batchRunState?.lockedDocuments ===
nextProps.batchRunState?.lockedDocuments,
prevProps.batchRunState?.errorDocumentIndex ===
nextProps.batchRunState?.errorDocumentIndex, prevProps.externalLocalContent ===
nextProps.externalLocalContent, prevProps.externalSavedContent ===
nextProps.externalSavedContent, prevProps.documentTree ===
nextProps.documentTree, prevProps.documentTaskCounts ===
nextProps.documentTaskCounts) or delete the custom comparator around
AutoRunInner to prevent stale UI state.
- Around line 239-254: The reset flow currently calls
window.maestro.autorun.writeDoc directly and updates setSavedContent/onShowFlash
without verifying the IPC result; replace this direct write with the same
checked save helper used by handleSave (i.e., call the shared save/check routine
that validates the { success } envelope) so the UI only updates when the save
succeeded, and remove the silent try-catch that only logs; on failure use the
app's Sentry utility (e.g., captureException or the project's error reporter)
and surface an error flash instead of reporting success. Ensure you update
references inside the AutoRun component (where writeDoc, setSavedContent, and
onShowFlash are used) to rely on the common save helper and to handle the {
success: false } branch explicitly.
In `@src/renderer/components/AutoRun/AutoRunEmptyStates.tsx`:
- Around line 121-132: The Refresh button currently calls onRefresh even while
isRefreshingEmpty is true; update the button so clicks are ignored during
refresh by guarding the handler and marking the button disabled/aria-busy when
isRefreshingEmpty is true. Specifically, in the AutoRunEmptyStates component
change the button's onClick to a guarded function (e.g., if (isRefreshingEmpty)
return; onRefresh()) and add disabled={isRefreshingEmpty} plus
aria-busy={isRefreshingEmpty} and a visual disabled style (cursor
not-allowed/opacity) so RefreshCw and the button cannot trigger duplicate
refresh work.
In `@src/renderer/components/AutoRun/AutoRunToolbar.tsx`:
- Around line 136-142: The click handler currently calls onSave() without
awaiting it, so openBatchRunner can run against stale or unsaved state; update
the handler in AutoRunToolbar.tsx to await the async onSave() when isDirty is
true and only call onOpenBatchRunner() if the awaited save resolved successfully
(handle rejection by catching errors and avoid opening the runner on save
failure); reference the existing symbols onSave, onOpenBatchRunner, and isDirty
in the updated logic.
In `@src/renderer/hooks/batch/useAutoRunContentSync.ts`:
- Around line 139-153: The handleSave in useAutoRunContentSync currently ignores
the {success, error} result from window.maestro.autorun.writeDoc and
unconditionally calls setSavedContent(localContent) and only logs in the catch;
change it to await the resolved result from writeDoc (referencing
window.maestro.autorun.writeDoc and the handleSave callback), check
result.success before calling setSavedContent(localContent), and if success is
false or result.error exists, report the failure via Sentry.captureException (or
your Sentry util) and surface/return the error instead of silently swallowing
it; keep the try/catch to handle unexpected throws but ensure only successful
writes clear the dirty state.
In `@src/renderer/hooks/batch/useAutoRunKeyboard.ts`:
- Around line 77-121: The keyboard handler in useAutoRunKeyboard.ts compares
e.key directly (e.g., e.key === 'z') so Cmd+Shift+Z fails because e.key is 'Z'
when Shift is held; fix by normalizing the key once at the top of the handler
(e.g., const key = e.key.toLowerCase()) and then use that normalized key for all
shortcut checks (undo/redo that call handleUndo/handleRedo, toggleMode,
openSearch, save, insert checkbox, etc.) so Shift-modified letters still match
correctly.
In `@src/renderer/hooks/batch/useAutoRunMarkdown.ts`:
- Around line 80-94: The token-count useEffect can race when savedContent
changes: ensure you ignore stale encoder promises by using a cancellation token
or local generation id inside the effect (e.g., capture a local "currentGen" /
"isActive" flag at the start of the useEffect and check it before calling
setTokenCount) so only the latest getEncoder().then(...) result updates state;
in the catch branch report unexpected errors to Sentry (use the project's Sentry
utility, e.g., captureException) instead of only console.error, and still guard
that you only call setTokenCount(null) when the effect is still active.
In `@src/renderer/hooks/batch/useAutoRunScrollSync.ts`:
- Around line 133-140: The edit scroll restoration is incorrectly gated by the
same check as cursor restore; inside the useEffect ensure selection and
edit-scroll checks are separate: use textareaRef.current to restore scrollTop
when initialEditScrollPos is defined (check e.g. initialEditScrollPos !==
undefined and initialEditScrollPos > 0 if negative is invalid) and only use a
cursor-specific check (initialCursorPosition !== undefined or
initialCursorPosition >= 0) when calling textareaRef.current.setSelectionRange;
similarly keep previewRef scroll restore using initialPreviewScrollPos. Update
the logic around useEffect, textareaRef, initialCursorPosition,
initialEditScrollPos, previewRef and initialPreviewScrollPos so each restore is
independently guarded by its own presence/validity check.
- Around line 63-89: The onStateChange callback is called before the
requestAnimationFrame handler updates scroll positions, causing stale values;
move the onStateChange call inside the requestAnimationFrame callback in
useAutoRunScrollSync so it runs after the branches that set
previewRef.current.scrollTop / textareaRef.current.scrollTop and update
previewScrollPosRef.current/editScrollPosRef.current, and ensure the payload
still includes mode (from setMode/newMode), cursorPosition
(textareaRef.current?.selectionStart || 0), editScrollPos
(textareaRef.current?.scrollTop || 0) and previewScrollPos
(previewRef.current?.scrollTop || 0) so the parent receives synchronized values.
In `@src/renderer/hooks/batch/useAutoRunSearch.ts`:
- Around line 47-59: The closeSearch callback in useAutoRunSearch doesn't reset
the userNavigatedToMatchRef flag, causing the next typed query to be treated as
an explicit navigation; update closeSearch to set
userNavigatedToMatchRef.current = false when clearing search state (alongside
setSearchOpen, setSearchQuery, setCurrentMatchIndex, setTotalMatches and
clearing matchElementsRef), and also ensure any other code paths that
clear/close search (the handlers around the edit-scroll effect and preview
navigation logic inside this hook) explicitly reset
userNavigatedToMatchRef.current = false so the next input is not misinterpreted
as user navigation.
---
Nitpick comments:
In `@src/__tests__/renderer/components/AutoRun/AutoRunToolbar.test.tsx`:
- Around line 204-213: Update the test in AutoRunToolbar.test.tsx to assert call
order in addition to call counts: after triggering the Run button, assert that
onSave was invoked before onOpenBatchRunner by using mock call-order assertions
(e.g., expect(onSave).toHaveBeenNthCalledWith(1, ...) or compare
onSave.mock.invocationCallOrder[0] <
onOpenBatchRunner.mock.invocationCallOrder[0]) referencing the onSave and
onOpenBatchRunner mocks used in the test for AutoRunToolbar; keep the existing
call-count assertions and add the order assertion immediately after them.
In `@src/renderer/components/AutoRun/AttachmentImage.tsx`:
- Around line 66-67: The prop typing for "theme" in AttachmentImage-related
definitions currently uses "any"; replace those with the proper Theme type to
restore compile-time validation: import the shared Theme type (e.g., from your
theme module or the library you use) and update the prop/interface declarations
that declare "theme: any" (the AttachmentImage component props and the other
occurrence around lines referenced) to "theme: Theme"; ensure any places
referencing those prop types still compile by updating imports to include Theme
and removing the any usage.
In `@src/renderer/components/AutoRun/AutoRunAttachmentsPanel.tsx`:
- Around line 32-57: The toggle button for attachments (the button using
onToggleExpanded and the attachmentsExpanded state) needs accessible collapse
semantics: add aria-expanded={attachmentsExpanded} to the button and add
aria-controls pointing to the panel container; then give the panel container div
(the one rendering attachmentsList/ImagePreview) a matching id (e.g.,
"attachments-panel" or a generated unique id) so assistive tech can associate
the button with the collapsible region. Ensure the id referenced by
aria-controls matches the panel id and only present aria-controls when the panel
exists.
In `@src/renderer/components/AutoRun/AutoRunErrorBanner.tsx`:
- Around line 23-29: The error banner wrapper div in AutoRunErrorBanner (the
<div> with className "mx-2 mb-2 p-3 rounded-lg border") should expose the
runtime error to assistive tech by adding an ARIA live region; update that
wrapper to include either role="alert" or aria-live="assertive" (and optionally
aria-atomic="true") so the paused/error banner is announced when it appears.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3759911c-2049-4a8d-9709-b65aa3464694
📒 Files selected for processing (37)
src/__tests__/renderer/components/AutoRun.test.tsxsrc/__tests__/renderer/components/AutoRun/AutoRunAttachmentsPanel.test.tsxsrc/__tests__/renderer/components/AutoRun/AutoRunBottomPanel.test.tsxsrc/__tests__/renderer/components/AutoRun/AutoRunEmptyStates.test.tsxsrc/__tests__/renderer/components/AutoRun/AutoRunErrorBanner.test.tsxsrc/__tests__/renderer/components/AutoRun/AutoRunToolbar.test.tsxsrc/__tests__/renderer/components/AutoRunDocumentSelector.test.tsxsrc/__tests__/renderer/components/AutoRunExpandedModal.test.tsxsrc/__tests__/renderer/components/AutoRunLightbox.test.tsxsrc/__tests__/renderer/components/AutoRunSearchBar.test.tsxsrc/__tests__/renderer/components/AutoRunSetupModal.test.tsxsrc/__tests__/renderer/components/AutoRunnerHelpModal.test.tsxsrc/__tests__/renderer/hooks/useAutoRunScrollSync.test.tssrc/renderer/components/AppModals/AppUtilityModals.tsxsrc/renderer/components/AutoRun.tsxsrc/renderer/components/AutoRun/AttachmentImage.tsxsrc/renderer/components/AutoRun/AutoRun.tsxsrc/renderer/components/AutoRun/AutoRunAttachmentsPanel.tsxsrc/renderer/components/AutoRun/AutoRunBottomPanel.tsxsrc/renderer/components/AutoRun/AutoRunDocumentSelector.tsxsrc/renderer/components/AutoRun/AutoRunEmptyStates.tsxsrc/renderer/components/AutoRun/AutoRunErrorBanner.tsxsrc/renderer/components/AutoRun/AutoRunExpandedModal.tsxsrc/renderer/components/AutoRun/AutoRunLightbox.tsxsrc/renderer/components/AutoRun/AutoRunSearchBar.tsxsrc/renderer/components/AutoRun/AutoRunSetupModal.tsxsrc/renderer/components/AutoRun/AutoRunToolbar.tsxsrc/renderer/components/AutoRun/AutoRunnerHelpModal.tsxsrc/renderer/components/AutoRun/index.tssrc/renderer/components/AutoRun/types.tssrc/renderer/components/RightPanel.tsxsrc/renderer/hooks/batch/index.tssrc/renderer/hooks/batch/useAutoRunContentSync.tssrc/renderer/hooks/batch/useAutoRunKeyboard.tssrc/renderer/hooks/batch/useAutoRunMarkdown.tssrc/renderer/hooks/batch/useAutoRunScrollSync.tssrc/renderer/hooks/batch/useAutoRunSearch.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/AutoRun.tsx
Comment on lines
+12
to
+13
| const decodedSrc = decodeURIComponent(src); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "AttachmentImage.tsx" | head -5Repository: RunMaestro/Maestro
Length of output: 115
🏁 Script executed:
cat -n ./src/renderer/components/AutoRun/AttachmentImage.tsx | head -250Repository: RunMaestro/Maestro
Length of output: 9235
Guard decodeURIComponent against malformed URI sequences.
At lines 12, 96, and 226, decodeURIComponent can throw a URIError when encountering invalid percent-encoded sequences (e.g., %XY with invalid hex), crashing the component initialization, effect, or render.
✅ Suggested fix
+const safeDecodeURIComponent = (value: string): string => {
+ try {
+ return decodeURIComponent(value);
+ } catch {
+ return value;
+ }
+};
+
export function getInitialImageState(src: string | undefined, folderPath: string | null) {
...
- const decodedSrc = decodeURIComponent(src);
+ const decodedSrc = safeDecodeURIComponent(src);
...
- const decodedSrc = decodeURIComponent(src);
+ const decodedSrc = safeDecodeURIComponent(src);
...
- const decodedSrcForClick = src ? decodeURIComponent(src) : '';
+ const decodedSrcForClick = src ? safeDecodeURIComponent(src) : '';📝 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 decodedSrc = decodeURIComponent(src); | |
| const safeDecodeURIComponent = (value: string): string => { | |
| try { | |
| return decodeURIComponent(value); | |
| } catch { | |
| return value; | |
| } | |
| }; | |
| export function getInitialImageState(src: string | undefined, folderPath: string | null) { | |
| const decodedSrc = safeDecodeURIComponent(src); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AutoRun/AttachmentImage.tsx` around lines 12 - 13, In
the AttachmentImage component, guard every use of decodeURIComponent (the
decodedSrc assignment and the other two decode calls used in the effect/render)
against malformed URI sequences by wrapping them in a try/catch (or creating a
small safeDecodeURIComponent helper) so a thrown URIError falls back to the
original src (or a sanitized variant) instead of crashing; update the decodedSrc
initialization and the two other decodeURIComponent usages inside
AttachmentImage to use this safe wrapper and ensure downstream logic uses the
fallback value.
Comment on lines
+239
to
+254
| try { | ||
| await window.maestro.autorun.writeDoc( | ||
| folderPath, | ||
| selectedFile + '.md', | ||
| resetContent, | ||
| sshRemoteId | ||
| ); | ||
| setSavedContent(resetContent); | ||
|
|
||
| // Show flash notification with the count of reset tasks | ||
| if (onShowFlash && resetCount > 0) { | ||
| onShowFlash(`${resetCount} task${resetCount !== 1 ? 's' : ''} reverted to incomplete`); | ||
| } | ||
| } catch (err) { | ||
| console.error('Failed to save after reset:', err); | ||
| } |
There was a problem hiding this comment.
The reset flow repeats the same unverified save path.
Lines 240-250 update savedContent and flash success without checking the writeDoc result envelope first. If the IPC layer resolves { success: false }, the UI will say tasks were reset on disk when they were not. This should go through the same checked save helper as handleSave instead of duplicating the write logic here.
As per coding guidelines "Do not silently swallow errors with try-catch blocks that only log" and "Use Sentry utilities for explicit reporting."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AutoRun/AutoRun.tsx` around lines 239 - 254, The
reset flow currently calls window.maestro.autorun.writeDoc directly and updates
setSavedContent/onShowFlash without verifying the IPC result; replace this
direct write with the same checked save helper used by handleSave (i.e., call
the shared save/check routine that validates the { success } envelope) so the UI
only updates when the save succeeded, and remove the silent try-catch that only
logs; on failure use the app's Sentry utility (e.g., captureException or the
project's error reporter) and surface an error flash instead of reporting
success. Ensure you update references inside the AutoRun component (where
writeDoc, setSavedContent, and onShowFlash are used) to rely on the common save
helper and to handle the { success: false } branch explicitly.
Comment on lines
+583
to
+592
| <div | ||
| className="flex-1 min-h-0 overflow-y-auto mx-2 rounded-lg transition-colors" | ||
| style={{ | ||
| backgroundColor: isDirty && !isLocked ? `${theme.colors.warning}08` : 'transparent', | ||
| border: | ||
| isDirty && !isLocked | ||
| ? `2px solid ${theme.colors.warning}40` | ||
| : '2px solid transparent', | ||
| }} | ||
| > |
There was a problem hiding this comment.
previewRef is attached to the wrong scroll container.
Line 584 makes the outer wrapper the scrolling element, but Lines 640-663 put previewRef and onScroll={handlePreviewScroll} on the inner preview div. useAutoRunScrollSync and the markdown anchor-scrolling path will read a node whose scrollTop never changes, so preview scroll restore/auto-follow won't fire reliably. Move the ref/onScroll to the element with overflow-y-auto, or make the preview div own the overflow.
Also applies to: 639-663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AutoRun/AutoRun.tsx` around lines 583 - 592, The
previewRef is attached to the wrong element so useAutoRunScrollSync reads a
non-scrolling node; move the previewRef and onScroll={handlePreviewScroll} from
the inner preview div to the outer element that has overflow-y-auto (or
alternatively move the overflow-y-auto style onto the inner preview div) so that
previewRef.current is the actual scroll container used by useAutoRunScrollSync
and the markdown anchor scroll/auto-follow logic fires correctly; update
references to previewRef, handlePreviewScroll, and useAutoRunScrollSync
accordingly.
Comment on lines
+728
to
+770
| export const AutoRun = memo(AutoRunInner, (prevProps, nextProps) => { | ||
| // Only re-render when these specific props actually change | ||
| return ( | ||
| prevProps.content === nextProps.content && | ||
| prevProps.sessionId === nextProps.sessionId && | ||
| prevProps.mode === nextProps.mode && | ||
| prevProps.theme === nextProps.theme && | ||
| // Document state | ||
| prevProps.folderPath === nextProps.folderPath && | ||
| prevProps.selectedFile === nextProps.selectedFile && | ||
| prevProps.documentList === nextProps.documentList && | ||
| prevProps.isLoadingDocuments === nextProps.isLoadingDocuments && | ||
| // Compare batch run state values, not object reference | ||
| prevProps.batchRunState?.isRunning === nextProps.batchRunState?.isRunning && | ||
| prevProps.batchRunState?.isStopping === nextProps.batchRunState?.isStopping && | ||
| prevProps.batchRunState?.currentTaskIndex === nextProps.batchRunState?.currentTaskIndex && | ||
| prevProps.batchRunState?.totalTasks === nextProps.batchRunState?.totalTasks && | ||
| // Error state (Phase 5.10) | ||
| prevProps.batchRunState?.errorPaused === nextProps.batchRunState?.errorPaused && | ||
| prevProps.batchRunState?.error?.type === nextProps.batchRunState?.error?.type && | ||
| prevProps.batchRunState?.error?.message === nextProps.batchRunState?.error?.message && | ||
| // Session state affects UI (busy disables Run button) | ||
| prevProps.sessionState === nextProps.sessionState && | ||
| // Callbacks are typically stable, but check identity | ||
| prevProps.onContentChange === nextProps.onContentChange && | ||
| prevProps.onModeChange === nextProps.onModeChange && | ||
| prevProps.onStateChange === nextProps.onStateChange && | ||
| prevProps.onOpenBatchRunner === nextProps.onOpenBatchRunner && | ||
| prevProps.onStopBatchRun === nextProps.onStopBatchRun && | ||
| prevProps.onOpenSetup === nextProps.onOpenSetup && | ||
| prevProps.onRefresh === nextProps.onRefresh && | ||
| prevProps.onSelectDocument === nextProps.onSelectDocument && | ||
| prevProps.onShowFlash === nextProps.onShowFlash && | ||
| // UI control props | ||
| prevProps.hideTopControls === nextProps.hideTopControls && | ||
| // External change detection | ||
| prevProps.contentVersion === nextProps.contentVersion && | ||
| // Auto-follow state | ||
| prevProps.autoFollowEnabled === nextProps.autoFollowEnabled | ||
| // Note: initialCursorPosition, initialEditScrollPos, initialPreviewScrollPos | ||
| // are intentionally NOT compared - they're only used on mount | ||
| // Note: documentTree is derived from documentList, comparing documentList is sufficient | ||
| ); |
There was a problem hiding this comment.
The custom comparator is dropping live prop updates.
Lines 81-86 derive isLocked from batchRunState.worktreeActive and batchRunState.lockedDocuments, and Lines 97-100 derive errorDocumentName from errorDocumentIndex/documents, but the comparator never checks any of those fields. The same omission exists for externalLocalContent/externalSavedContent and documentTree/documentTaskCounts, which AutoRunInner also reads. In RightPanel.tsx those values come from shared parent state, so this memo can freeze stale draft content, lock state, or error metadata. Either compare every consumed prop or remove the custom comparator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AutoRun/AutoRun.tsx` around lines 728 - 770, The memo
comparator on AutoRun (AutoRun = memo(AutoRunInner, ...)) is missing props that
AutoRunInner reads—add comparisons for batchRunState.worktreeActive and
batchRunState.lockedDocuments (used to derive isLocked),
batchRunState.errorDocumentIndex and documents/documentList (used to compute
errorDocumentName), externalLocalContent and externalSavedContent, and
documentTree and documentTaskCounts; alternatively remove the custom comparator
so React does shallow prop comparison. Update the comparator to explicitly
compare those fields (e.g., prevProps.batchRunState?.worktreeActive ===
nextProps.batchRunState?.worktreeActive,
prevProps.batchRunState?.lockedDocuments ===
nextProps.batchRunState?.lockedDocuments,
prevProps.batchRunState?.errorDocumentIndex ===
nextProps.batchRunState?.errorDocumentIndex, prevProps.externalLocalContent ===
nextProps.externalLocalContent, prevProps.externalSavedContent ===
nextProps.externalSavedContent, prevProps.documentTree ===
nextProps.documentTree, prevProps.documentTaskCounts ===
nextProps.documentTaskCounts) or delete the custom comparator around
AutoRunInner to prevent stale UI state.
Comment on lines
+80
to
+94
| useEffect(() => { | ||
| if (!savedContent) { | ||
| setTokenCount(null); | ||
| return; | ||
| } | ||
|
|
||
| getEncoder() | ||
| .then((encoder) => { | ||
| const tokens = encoder.encode(savedContent); | ||
| setTokenCount(tokens.length); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Failed to count tokens:', err); | ||
| setTokenCount(null); | ||
| }); |
There was a problem hiding this comment.
Guard the token-count effect against stale resolves.
Lines 86-89 can complete out of order when savedContent changes quickly, so an older encode can still overwrite the newest count. Also, Lines 91-94 only log the failure. Add a cancellation guard here, and if you want to keep the null fallback, report unexpected encoder failures via Sentry instead of only console.error.
As per coding guidelines "Do not silently swallow errors with try-catch blocks that only log" and "Use Sentry utilities for explicit reporting."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useAutoRunMarkdown.ts` around lines 80 - 94, The
token-count useEffect can race when savedContent changes: ensure you ignore
stale encoder promises by using a cancellation token or local generation id
inside the effect (e.g., capture a local "currentGen" / "isActive" flag at the
start of the useEffect and check it before calling setTokenCount) so only the
latest getEncoder().then(...) result updates state; in the catch branch report
unexpected errors to Sentry (use the project's Sentry utility, e.g.,
captureException) instead of only console.error, and still guard that you only
call setTokenCount(null) when the effect is still active.
Address 11 inline findings and 3 nitpicks from code review: - AttachmentImage: guard decodeURIComponent with try/catch fallback, sync state when src/folderPath props change, fix theme: any → Theme - useAutoRunKeyboard: normalize e.key to lowercase so Cmd+Shift+Z (redo) works when Shift produces uppercase key - useAutoRunMarkdown: fix token count race condition with isActive flag - useAutoRunScrollSync: move onStateChange inside requestAnimationFrame so parent receives synchronized scroll values; separate cursor and scroll restore into independent guards - useAutoRunSearch: reset userNavigatedToMatchRef on closeSearch - AutoRunToolbar: await onSave before opening batch runner - AutoRunEmptyStates: disable refresh button while refreshing - AutoRunAttachmentsPanel: add aria-expanded and aria-controls - AutoRunErrorBanner: add role="alert" for assistive tech - AutoRunToolbar test: add call-order assertion and save-failure test
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/renderer/hooks/batch/useAutoRunSearch.ts (1)
95-116:⚠️ Potential issue | 🟡 MinorOnly arm the edit-navigation latch when edit navigation can actually run.
Lines 109-116 set
userNavigatedToMatchRef.currentbefore we know whether navigation will do anything, but Lines 123-125 and Line 174 are the only reset paths. In preview mode—or whentotalMatches === 0and Lines 96/103 return early—that leaves the latch stucktrue, so a later edit-mode render can jump/select unexpectedly.🔧 Possible fix
const goToNextMatchWithFlag = useCallback(() => { - userNavigatedToMatchRef.current = true; + userNavigatedToMatchRef.current = mode === 'edit' && totalMatches > 0; goToNextMatch(); - }, [goToNextMatch]); + }, [mode, totalMatches, goToNextMatch]); const goToPrevMatchWithFlag = useCallback(() => { - userNavigatedToMatchRef.current = true; + userNavigatedToMatchRef.current = mode === 'edit' && totalMatches > 0; goToPrevMatch(); - }, [goToPrevMatch]); + }, [mode, totalMatches, goToPrevMatch]);Also applies to: 123-125, 174-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunSearch.ts` around lines 95 - 116, The userNavigatedToMatchRef latch is being set in goToNextMatchWithFlag and goToPrevMatchWithFlag before knowing if navigation will run, which can leave the latch true in preview/zero-match cases; change those wrappers (goToNextMatchWithFlag, goToPrevMatchWithFlag) to first check the same guard used in goToNextMatch/goToPrevMatch (e.g., ensure totalMatches > 0 or that navigation would proceed) and only then set userNavigatedToMatchRef.current = true and call goToNextMatch/goToPrevMatch; also verify the existing reset paths that clear userNavigatedToMatchRef (the code referenced at the reset locations) still execute in all code paths so the latch cannot remain stuck true.
🧹 Nitpick comments (3)
src/renderer/hooks/batch/useAutoRunKeyboard.ts (2)
155-159: Consider using consistent cursor restoration timing.Tab handler (line 72) uses
requestAnimationFrame, while checkbox and Enter handlers usesetTimeout(..., 0). While both work,requestAnimationFrameis generally preferred for DOM-related updates as it's synchronized with the browser's paint cycle.♻️ Proposed consistency fix
- setTimeout(() => { - if (textareaRef.current) { - textareaRef.current.setSelectionRange(newCursorPos, newCursorPos); - } - }, 0); + requestAnimationFrame(() => { + if (textareaRef.current) { + textareaRef.current.setSelectionRange(newCursorPos, newCursorPos); + } + });Apply similar changes to lines 185-190, 201-206, and 217-222.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunKeyboard.ts` around lines 155 - 159, Replace the setTimeout(..., 0) cursor-restoration calls with requestAnimationFrame to match the Tab handler's timing; specifically update the blocks that call textareaRef.current.setSelectionRange (the occurrences around the shown diff and the similar blocks around the other handlers referenced) so they use requestAnimationFrame(() => { if (textareaRef.current) textareaRef.current.setSelectionRange(newCursorPos, newCursorPos); }); to ensure consistent DOM paint-synced cursor restoration in useAutoRunKeyboard.ts.
37-37: Remove unused parameterscheduleUndoSnapshot.The parameter is destructured with an underscore prefix but never used in the hook body. This is dead code from refactoring—remove it from both the interface definition and the destructuring to clean up the unused parameter.
🧹 Proposed cleanup
export interface UseAutoRunKeyboardParams { localContent: string; setLocalContent: (content: string) => void; textareaRef: RefObject<HTMLTextAreaElement | null>; pushUndoState: (content?: string, cursor?: number) => void; lastUndoSnapshotRef: MutableRefObject<string>; - scheduleUndoSnapshot: (previousContent: string, previousCursor: number) => void; handleUndo: () => void;const { localContent, setLocalContent, textareaRef, pushUndoState, lastUndoSnapshotRef, - scheduleUndoSnapshot: _scheduleUndoSnapshot, handleUndo,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunKeyboard.ts` at line 37, Remove the dead parameter scheduleUndoSnapshot from the useAutoRunKeyboard hook by deleting its destructured entry (remove "scheduleUndoSnapshot: _scheduleUndoSnapshot" or similar) and removing it from the hook's input/interface/type definition so the parameter is not expected anywhere; search for the symbol scheduleUndoSnapshot and its underscored variant _scheduleUndoSnapshot (and the interface/type that declares it) and remove those declarations/usages since the hook body does not use it.src/renderer/hooks/batch/useAutoRunSearch.ts (1)
20-21:matchElementsReflooks like leftover API surface from the extraction.This hook exposes the ref, initializes it, clears it on close, and returns it, but nothing here ever populates or reads it. If this is meant to stay public,
handleMatchRenderedneeds to own it; otherwise, should I remove these now-unused elements:UseAutoRunSearchReturn.matchElementsRef,const matchElementsRef, and the clear at Line 52?Based on learnings: Applies to **/*.{ts,tsx} : After refactoring, identify now-unreachable code, list it explicitly, ask 'Should I remove these now-unused elements: [list]?' Don't leave corpses and don't delete without asking.
Also applies to: 35-35, 52-52, 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunSearch.ts` around lines 20 - 21, The hook currently exposes and manages matchElementsRef but nothing reads or writes it—either make handleMatchRendered own and populate this ref (update handleMatchRendered to push/assign elements into matchElementsRef.current) or remove the unused API surface; specifically remove UseAutoRunSearchReturn.matchElementsRef, the const matchElementsRef declaration, and the clear/reset logic currently invoked on close (the clear at the place currently resetting matchElementsRef). Update handleMatchRendered or delete the unused symbols consistently and run type checks to ensure no remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/AutoRun/AutoRunToolbar.tsx`:
- Around line 139-143: The catch block around await onSave() in
AutoRunToolbar.tsx currently swallows errors silently; verify whether onSave
implementations (consumers of AutoRunToolbar) already display errors—if they do,
add a comment asserting that and leave the catch to simply return; otherwise,
update the catch to surface a brief user notification (using the app's
notification/toast helper) with a concise message like "Save failed" and the
error message, and then return to avoid opening the runner; reference the onSave
invocation and the try/catch in AutoRunToolbar to locate the change.
In `@src/renderer/hooks/batch/useAutoRunKeyboard.ts`:
- Around line 95-97: The call to handleSave() in the isDirty branch is not
awaited and lacks error handling, causing unhandled promise rejections; update
the block in useAutoRunKeyboard.ts so that you await handleSave() (or attach
.catch()) and handle errors explicitly — e.g. wrap await handleSave() in a
try/catch and on error call your error reporter (Sentry.captureException or the
app's reportError) and/or rethrow/return a controlled failure; locate the
isDirty check and the handleSave() invocation to apply this change.
In `@src/renderer/hooks/batch/useAutoRunSearch.ts`:
- Around line 72-80: The debounced count handler captures a stale
currentMatchIndex when the timeout fires; update the clamp to use a functional
state updater so it reads the latest currentMatchIndex at execution time. Inside
the setTimeout callback (where escapedQuery/regex/matches/count are computed and
setTotalMatches is called), replace the direct comparison against
currentMatchIndex with setCurrentMatchIndex(prev => (count > 0 && prev >= count)
? 0 : prev) (i.e., use the functional updater form) so the clamp uses the
freshest state; keep searchCountTimeoutRef.current, escapedQuery,
setTotalMatches and setCurrentMatchIndex as the target locations to change.
---
Duplicate comments:
In `@src/renderer/hooks/batch/useAutoRunSearch.ts`:
- Around line 95-116: The userNavigatedToMatchRef latch is being set in
goToNextMatchWithFlag and goToPrevMatchWithFlag before knowing if navigation
will run, which can leave the latch true in preview/zero-match cases; change
those wrappers (goToNextMatchWithFlag, goToPrevMatchWithFlag) to first check the
same guard used in goToNextMatch/goToPrevMatch (e.g., ensure totalMatches > 0 or
that navigation would proceed) and only then set userNavigatedToMatchRef.current
= true and call goToNextMatch/goToPrevMatch; also verify the existing reset
paths that clear userNavigatedToMatchRef (the code referenced at the reset
locations) still execute in all code paths so the latch cannot remain stuck
true.
---
Nitpick comments:
In `@src/renderer/hooks/batch/useAutoRunKeyboard.ts`:
- Around line 155-159: Replace the setTimeout(..., 0) cursor-restoration calls
with requestAnimationFrame to match the Tab handler's timing; specifically
update the blocks that call textareaRef.current.setSelectionRange (the
occurrences around the shown diff and the similar blocks around the other
handlers referenced) so they use requestAnimationFrame(() => { if
(textareaRef.current) textareaRef.current.setSelectionRange(newCursorPos,
newCursorPos); }); to ensure consistent DOM paint-synced cursor restoration in
useAutoRunKeyboard.ts.
- Line 37: Remove the dead parameter scheduleUndoSnapshot from the
useAutoRunKeyboard hook by deleting its destructured entry (remove
"scheduleUndoSnapshot: _scheduleUndoSnapshot" or similar) and removing it from
the hook's input/interface/type definition so the parameter is not expected
anywhere; search for the symbol scheduleUndoSnapshot and its underscored variant
_scheduleUndoSnapshot (and the interface/type that declares it) and remove those
declarations/usages since the hook body does not use it.
In `@src/renderer/hooks/batch/useAutoRunSearch.ts`:
- Around line 20-21: The hook currently exposes and manages matchElementsRef but
nothing reads or writes it—either make handleMatchRendered own and populate this
ref (update handleMatchRendered to push/assign elements into
matchElementsRef.current) or remove the unused API surface; specifically remove
UseAutoRunSearchReturn.matchElementsRef, the const matchElementsRef declaration,
and the clear/reset logic currently invoked on close (the clear at the place
currently resetting matchElementsRef). Update handleMatchRendered or delete the
unused symbols consistently and run type checks to ensure no remaining
references.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b40ee3f-07f1-48ca-9b75-da7cbbc4599b
📒 Files selected for processing (12)
src/__tests__/renderer/components/AutoRun.test.tsxsrc/__tests__/renderer/components/AutoRun/AutoRunToolbar.test.tsxsrc/__tests__/renderer/hooks/useAutoRunScrollSync.test.tssrc/renderer/components/AutoRun/AttachmentImage.tsxsrc/renderer/components/AutoRun/AutoRunAttachmentsPanel.tsxsrc/renderer/components/AutoRun/AutoRunEmptyStates.tsxsrc/renderer/components/AutoRun/AutoRunErrorBanner.tsxsrc/renderer/components/AutoRun/AutoRunToolbar.tsxsrc/renderer/hooks/batch/useAutoRunKeyboard.tssrc/renderer/hooks/batch/useAutoRunMarkdown.tssrc/renderer/hooks/batch/useAutoRunScrollSync.tssrc/renderer/hooks/batch/useAutoRunSearch.ts
✅ Files skipped from review due to trivial changes (2)
- src/tests/renderer/hooks/useAutoRunScrollSync.test.ts
- src/renderer/components/AutoRun/AttachmentImage.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tests/renderer/components/AutoRun.test.tsx
- src/renderer/components/AutoRun/AutoRunAttachmentsPanel.tsx
- src/renderer/components/AutoRun/AutoRunEmptyStates.tsx
- src/renderer/components/AutoRun/AutoRunErrorBanner.tsx
- src/tests/renderer/components/AutoRun/AutoRunToolbar.test.tsx
- src/renderer/hooks/batch/useAutoRunMarkdown.ts
- src/renderer/hooks/batch/useAutoRunScrollSync.ts
Comment on lines
+139
to
+143
| try { | ||
| await onSave(); | ||
| } catch { | ||
| return; // Don't open runner if save failed | ||
| } |
There was a problem hiding this comment.
Consider providing user feedback when save fails.
The catch block silently swallows the error and returns, which means the user gets no indication that the save failed—the "Run" button simply appears to do nothing. Per coding guidelines, expected/recoverable errors should be handled gracefully with user feedback.
If onSave() already displays its own error notification, this is fine. Otherwise, consider either:
- Letting
onSave()handle and display its own errors (verify this is the case), or - Adding a brief notification here.
#!/bin/bash
# Check if onSave implementations in consumers handle their own error display
rg -nA 10 'onSave.*Promise' --type=ts --type=tsx src/renderer/components/AutoRun/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/AutoRun/AutoRunToolbar.tsx` around lines 139 - 143,
The catch block around await onSave() in AutoRunToolbar.tsx currently swallows
errors silently; verify whether onSave implementations (consumers of
AutoRunToolbar) already display errors—if they do, add a comment asserting that
and leave the catch to simply return; otherwise, update the catch to surface a
brief user notification (using the app's notification/toast helper) with a
concise message like "Save failed" and the error message, and then return to
avoid opening the runner; reference the onSave invocation and the try/catch in
AutoRunToolbar to locate the change.
- useAutoRunSearch: fix stale currentMatchIndex in debounced count via functional updater; guard navigation latch behind totalMatches > 0 check; remove unused matchElementsRef from return type and internals - useAutoRunKeyboard: handle unhandled promise rejection on Cmd+S by adding .catch(); remove dead scheduleUndoSnapshot parameter; replace setTimeout(..., 0) with requestAnimationFrame for cursor restoration
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/renderer/hooks/batch/useAutoRunSearch.ts (1)
104-114:⚠️ Potential issue | 🟡 MinorDon't latch edit-only navigation state while preview is active.
userNavigatedToMatchRefis still set in preview mode, but only the edit-mode scroll effect clears it. After navigating in preview, a later query change or mode switch back to edit can still be treated as explicit navigation and jump/select unexpectedly.Suggested fix
const goToNextMatchWithFlag = useCallback(() => { if (totalMatches === 0) return; - userNavigatedToMatchRef.current = true; + userNavigatedToMatchRef.current = mode === 'edit'; goToNextMatch(); -}, [goToNextMatch, totalMatches]); +}, [goToNextMatch, totalMatches, mode]); const goToPrevMatchWithFlag = useCallback(() => { if (totalMatches === 0) return; - userNavigatedToMatchRef.current = true; + userNavigatedToMatchRef.current = mode === 'edit'; goToPrevMatch(); -}, [goToPrevMatch, totalMatches]); +}, [goToPrevMatch, totalMatches, mode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/batch/useAutoRunSearch.ts` around lines 104 - 114, The handlers goToNextMatchWithFlag and goToPrevMatchWithFlag currently set userNavigatedToMatchRef.current unconditionally, which latches an “edit-only” navigation state while preview is active; update both callbacks to only set userNavigatedToMatchRef.current = true when preview is not active (check the existing preview flag used in this hook, e.g., isPreviewActive / isPreviewMode), and include that preview flag in the callbacks' dependency arrays so the behavior updates correctly when preview state changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/batch/useAutoRunKeyboard.ts`:
- Around line 90-97: The save shortcut currently swallows save rejections in
useAutoRunKeyboard: when (e.metaKey || e.ctrlKey) && key === 's' it calls
handleSave().catch(() => {}) which hides failures; change this to report the
error to Sentry instead—import captureException (or captureMessage) and in the
catch handler call captureException(error) (and optionally captureMessage with
context like "auto-save keyboard shortcut failed" and include isDirty state or
other context), then preserve the fire-and-forget behavior (do not rethrow).
Ensure references are to useAutoRunKeyboard, handleSave, isDirty and the Sentry
helpers captureException/captureMessage.
---
Duplicate comments:
In `@src/renderer/hooks/batch/useAutoRunSearch.ts`:
- Around line 104-114: The handlers goToNextMatchWithFlag and
goToPrevMatchWithFlag currently set userNavigatedToMatchRef.current
unconditionally, which latches an “edit-only” navigation state while preview is
active; update both callbacks to only set userNavigatedToMatchRef.current = true
when preview is not active (check the existing preview flag used in this hook,
e.g., isPreviewActive / isPreviewMode), and include that preview flag in the
callbacks' dependency arrays so the behavior updates correctly when preview
state changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f26ac9bb-3bf0-43f0-b280-a9611f3c24e4
📒 Files selected for processing (3)
src/renderer/components/AutoRun/AutoRun.tsxsrc/renderer/hooks/batch/useAutoRunKeyboard.tssrc/renderer/hooks/batch/useAutoRunSearch.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/components/AutoRun/AutoRun.tsx
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
Moves AutoRun.tsx (2,290 lines) into
AutoRun/ directory module and extracts 5 presentational components, 5 hooks, and a types file. Shell reduced to 775 lines (−66%).
Extracted components:
Extracted hooks:
Also consolidates 6 satellite files into the AutoRun/ directory via git mv, updates all import paths in consumers (RightPanel, AppUtilityModals, tests).
102 new tests across 6 test files. All 24,091 tests pass, 0 regressions.
Summary by CodeRabbit
New Features
Hooks
Tests