fix(autorun): resolve paused batch session for resume/skip/abort#715
fix(autorun): resolve paused batch session for resume/skip/abort#715nehaaprasad wants to merge 1 commit intoRunMaestro:mainfrom
Conversation
Greptile SummaryThis PR fixes a bug where the error banner in the Auto Run panel persisted after clicking Resume, Skip, or Abort — caused by the handlers resolving the wrong batch session ID via the generic The fix introduces Key changes:
Issue flagged:
Confidence Score: 4/5Safe to merge; the fix correctly resolves the primary bug for both single and multi-session scenarios. The change is small and targeted, uses the established src/renderer/hooks/batch/useBatchHandlers.ts — specifically the branch-2 fallback in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks Resume / Skip / Abort in UI] --> B[resolveBatchSessionIdForPausedError called]
B --> C{activeSession has errorPaused?}
C -- Yes --> D[Return activeSession.id]
C -- No --> E{Any session in batchRunStates has errorPaused?}
E -- Yes --> F[Return that session id - may not match UI]
E -- No --> G{activeBatchSessionIds non-empty?}
G -- Yes --> H[Return activeBatchSessionIds 0 - pre-existing fallback]
G -- No --> I[Return activeSession.id - last resort]
D --> J[Call action on resolved sessionId]
F --> J
H --> J
I --> J
J --> K[handleClearAgentError on same sessionId]
Reviews (1): Last reviewed commit: "fix(autorun): resolve paused batch sessi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const pausedId = Object.keys(batchRunStates).find( | ||
| (id) => batchRunStates[id]?.errorPaused | ||
| ); | ||
| if (pausedId) return pausedId; |
There was a problem hiding this comment.
Branch 2 searches all batchRunStates for any session with errorPaused === true, regardless of whether that session is currently active (shown in the UI). The Resume/Skip/Abort buttons in RightPanel.tsx and AutoRun.tsx are only rendered when the active session has errorPaused, meaning in normal UI flow this branch can only be reached if the active session doesn't have errorPaused — in which case no error banner is visible and these handlers shouldn't fire at all.
In an edge case (e.g., programmatic invocation), this branch could silently call resumeAfterError/skipCurrentDocument/abortBatchOnError on a background session the user isn't looking at, which could corrupt its state unexpectedly.
A more conservative fallback would skip branch 2 entirely and let the function fall through to the existing activeBatchSessionIds[0] / activeSession safety net:
function resolveBatchSessionIdForPausedError(
batchRunStates: Record<string, BatchRunState>,
activeSessionId: string | undefined,
activeBatchSessionIds: string[]
): string | undefined {
if (activeSessionId && batchRunStates[activeSessionId]?.errorPaused) {
return activeSessionId;
}
if (activeBatchSessionIds.length > 0) return activeBatchSessionIds[0];
return activeSessionId;
}The root cause of #707 (wrong session chosen from activeBatchSessionIds) is fully addressed by branch 1 alone, since the RightPanel/AutoRun UI is always keyed to the active session's state.
📝 WalkthroughWalkthroughA helper function was added to intelligently resolve the target batch session ID when handling paused-on-error states, implementing fallback logic to prefer the active session if paused, then any paused session, then the first active session, then the active session ID itself. Three batch handlers were updated to use this helper instead of directly selecting session IDs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/useBatchHandlers.ts`:
- Around line 42-56: The resolver resolveBatchSessionIdForPausedError currently
falls back to a running/selected session which lets skip/resume/abort actions
target the wrong batch; change it to return undefined when no batchRunStates
entry has errorPaused (remove the activeBatchSessionIds[0]/activeSessionId
fallbacks) so paused-error actions fail closed, then update the three callers
that pass the now-unused third argument (the activeBatchSessionIds array) to
stop passing it and handle an undefined sessionId safely (abort/skip/resume
should no-op or surface an error). Optionally, for the stronger fix thread the
exact banner's sessionId into the handlers instead of inferring it from
resolveBatchSessionIdForPausedError.
🪄 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: d975ddbc-9ed7-4704-b26c-43e13c095fcf
📒 Files selected for processing (1)
src/renderer/hooks/batch/useBatchHandlers.ts
| function resolveBatchSessionIdForPausedError( | ||
| batchRunStates: Record<string, BatchRunState>, | ||
| activeSessionId: string | undefined, | ||
| activeBatchSessionIds: string[] | ||
| ): string | undefined { | ||
| if (activeSessionId && batchRunStates[activeSessionId]?.errorPaused) { | ||
| return activeSessionId; | ||
| } | ||
| const pausedId = Object.keys(batchRunStates).find( | ||
| (id) => batchRunStates[id]?.errorPaused | ||
| ); | ||
| if (pausedId) return pausedId; | ||
| if (activeBatchSessionIds.length > 0) return activeBatchSessionIds[0]; | ||
| return activeSessionId; | ||
| } |
There was a problem hiding this comment.
Make paused-error actions fail closed instead of guessing another session.
If Line 50 finds no paused batch, Line 54 and Line 55 still return a running or merely selected session, so Line 643, Line 654, and Line 665 can skip/resume/abort the wrong batch and clear the wrong agent error. And if more than one batch is paused, the current find(...) still resolves by store key order, not by the banner the user clicked. At minimum, return undefined when nothing is errorPaused; ideally, pass the displayed banner's sessionId into these handlers instead of inferring it here.
🛠️ Minimal safe fix
function resolveBatchSessionIdForPausedError(
batchRunStates: Record<string, BatchRunState>,
- activeSessionId: string | undefined,
- activeBatchSessionIds: string[]
+ activeSessionId: string | undefined
): string | undefined {
if (activeSessionId && batchRunStates[activeSessionId]?.errorPaused) {
return activeSessionId;
}
- const pausedId = Object.keys(batchRunStates).find(
- (id) => batchRunStates[id]?.errorPaused
- );
- if (pausedId) return pausedId;
- if (activeBatchSessionIds.length > 0) return activeBatchSessionIds[0];
- return activeSessionId;
+ return Object.keys(batchRunStates).find((id) => batchRunStates[id]?.errorPaused);
}Then drop the now-unused third argument at Line 643, Line 654, and Line 665. If the UI can surface more than one paused batch, the stronger fix is to thread the clicked banner's sessionId into these handlers instead of inferring it here.
Also applies to: 643-669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/batch/useBatchHandlers.ts` around lines 42 - 56, The
resolver resolveBatchSessionIdForPausedError currently falls back to a
running/selected session which lets skip/resume/abort actions target the wrong
batch; change it to return undefined when no batchRunStates entry has
errorPaused (remove the activeBatchSessionIds[0]/activeSessionId fallbacks) so
paused-error actions fail closed, then update the three callers that pass the
now-unused third argument (the activeBatchSessionIds array) to stop passing it
and handle an undefined sessionId safely (abort/skip/resume should no-op or
surface an error). Optionally, for the stronger fix thread the exact banner's
sessionId into the handlers instead of inferring it from
resolveBatchSessionIdForPausedError.
|
@nehaaprasad Please look into the feedback from coderabbit and greptile and address as needed |
pedramamini
left a comment
pedramamini
left a comment
There was a problem hiding this comment.
Review
The core idea is correct — reading errorPaused from the batch store to resolve the right session fixes the stale banner bug. One issue worth addressing:
Fallback to non-error-paused sessions undermines the fix
The fallback chain in resolveBatchSessionIdForPausedError goes:
activeSessionIdiferrorPaused✅- Any session with
errorPaused✅ activeBatchSessionIds[0]❌ — may not be error-pausedactiveSessionId❌ — may not be error-paused
Steps 3 and 4 can route skipCurrentDocument/resumeAfterError/abortBatchOnError to a session that isn't paused on error. Since these handlers are specifically for error recovery, the function should return undefined when nothing is actually error-paused and let the existing if (!sessionId) return guard handle it.
Suggested change — drop the non-error-paused fallbacks and the now-unnecessary activeBatchSessionIds parameter:
function resolveBatchSessionIdForPausedError(
batchRunStates: Record<string, BatchRunState>,
activeSessionId: string | undefined
): string | undefined {
if (activeSessionId && batchRunStates[activeSessionId]?.errorPaused) {
return activeSessionId;
}
return Object.keys(batchRunStates).find(
(id) => batchRunStates[id]?.errorPaused
);
}This also lets you remove activeBatchSessionIds from the three useCallback dependency arrays.
Minor
- Consider adding a
// Reads batchRunStates imperatively at call timecomment in each callback to signal the intentional store-outside-subscription pattern (matches the style used inonCompleteat line 184).
|
Thanks for the contribution @nehaaprasad! Great catch on the error banner persistence issue — the root cause analysis of needing to check I've merged your changes into Closing this PR in favor of the updated branch — your work is preserved in the commit history. Thanks again! |
3 participants
summary
Changes
fix : #707
Summary by CodeRabbit
Release Notes