refactor(batch): decompose useBatchProcessor#971
Conversation
Splits the largest hook in the codebase into a thin coordinator + 6 internal
hooks + 4 pure helpers under src/renderer/hooks/batch/internal/. Closes
section 4.3 of the Tier 4 backlog. Public API and behaviour unchanged; the
existing 165-test integration suite passes via the barrel.
What lives where:
- useBatchProcessor.ts coordinator: shared refs, unmount cleanup, hook composition, public API
- batchFlushState.ts AutoRunFlushState + claimFlushState() atomic-claim helper
- batchLoopSummary.ts createLoopSummaryEntry pure builder
- batchFinalSummary.ts buildFinalSummary pure builder ({ summary, details, isSuccess, statusText })
- batchProgressPoll.ts createProgressPoll factory: { start, stop, restart }
- useBatchSelectors.ts derived state + customPrompts handlers
- useBatchBroadcast.ts broadcastAutoRunState + debounced updateBatchStateAndBroadcast + flushDebouncedUpdate
- useBatchControlActions.ts stopBatchRun + pause/skip/resume/abort (share errorResolutionRefs + SET_STOPPING)
- useBatchKillAction.ts killBatchRun + emergency flush via claimFlushState
- useBatchRunner.ts startBatchRun: worktree setup, per-doc/per-task loop, stall detection, recount, history, Symphony, final-summary path
Notable design choices:
- claimFlushState extracted before the runner/kill split so both call sites
share one atomic read-and-delete primitive. Removes the file's most subtle
correctness invariant (kill vs natural-completion arbitration of the final
history entry / endAutoRun stats / onComplete) from a "scattered across two
files" failure mode.
- stopBatchRun merged into useBatchControlActions rather than its own file -
shares errorResolutionRefs + SET_STOPPING with abortBatchOnError, separate
hook would be ceremony.
- Coordinator owns all shared refs (stopRequestedRefs, errorResolutionRefs,
autoRunFlushStateRefs, isMountedRef, sessionsRef, audioFeedback*Refs,
updateBatchStateAndBroadcastRef) since each has 3+ readers/writers across
the split. updateBatchStateAndBroadcastRef stays in the coordinator so the
long-running async runner survives HMR re-renders across module boundaries.
- createProgressPoll.start() is async to preserve the original
baseline-then-schedule sequence.
Behaviour delta: none. Same broadcast cadence (200 ms debounce), same stall
heuristic (3 consecutive no-progress runs), same progress-poll generation
guard (now hidden inside createProgressPoll rather than four leaked let
bindings), same kill-vs-natural-completion arbitration.
Tests:
- Existing 165-test integration suite passes unchanged via the hooks barrel.
- 63 new unit tests across 7 files covering each extracted unit.
- Full project suite: 27,603 passing / 108 skipped / 0 failing
(was 27,468 baseline).
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR extracts batch auto-run internals from useBatchProcessor into typed utilities and focused internal hooks (final/loop summaries, flush-state, progress polling, broadcasting, control actions, kill, runner), adds tests, and rewires useBatchProcessor to compose the new modules while preserving its public API. ChangesBatch Processing Extraction & Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Confidence Score: 4/5The extraction is behaviour-equivalent and well-tested; findings are limited to style and consistency gaps that do not affect correctness. The refactoring cleanly separates concerns and the kill-vs-natural-completion arbitration is now centralised and tested. The three findings — console.assert instead of the logger, an underscore prefix on a variable that is actively used, and an inline loop-summary block that duplicates the extracted createLoopSummaryEntry helper — are all style/consistency issues with no runtime impact. useBatchRunner.ts still contains a 40-line inline loop-summary block that duplicates the logic in batchLoopSummary.ts; worth revisiting if that helper is extended in the future. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
BP["useBatchProcessor\n(coordinator, 328 LOC)"]
BP --> SEL["useBatchSelectors\n(reactive store selectors)"]
BP --> BROAD["useBatchBroadcast\n(web/mobile bridge + debounce)"]
BP --> CTRL["useBatchControlActions\n(stop / pause / skip / resume / abort)"]
BP --> KILL["useBatchKillAction\n(force-kill + emergency flush)"]
BP --> RUNNER["useBatchRunner\n(startBatchRun orchestrator)"]
KILL --> FS["batchFlushState\nclaimFlushState()"]
RUNNER --> FS
RUNNER --> POLL["batchProgressPoll\ncreateProgressPoll()"]
RUNNER --> LOOP["batchLoopSummary\ncreateLoopSummaryEntry()"]
RUNNER --> FINAL["batchFinalSummary\nbuildFinalSummary()"]
BROAD --> DS["useSessionDebounce\n(200ms debounce)"]
Reviews (1): Last reviewed commit: "refactor(batch): decompose useBatchProce..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }, []); | ||
|
|
||
| const { scheduleUpdate: _scheduleDebouncedUpdate, flushUpdate: flushDebouncedUpdate } = |
There was a problem hiding this comment.
_scheduleDebouncedUpdate conventionally signals that a value is intentionally unused, but it IS used on line 161 (_scheduleDebouncedUpdate(sessionId, updater, immediate)). This is misleading and will confuse readers and linters that treat _-prefixed names as dead.
| const { scheduleUpdate: _scheduleDebouncedUpdate, flushUpdate: flushDebouncedUpdate } = | |
| const { scheduleUpdate: scheduleDebouncedUpdate, flushUpdate: flushDebouncedUpdate } = |
| const completedLoopNumber = loopIteration + 1; | ||
| const completedLoopTasks = loopTasksCompleted; | ||
|
|
||
| // Calculate loop elapsed time | ||
| const loopElapsedMs = Date.now() - loopStartTime; | ||
|
|
||
| // Add loop summary history entry | ||
| const loopSummary = `Loop ${completedLoopNumber} completed: ${completedLoopTasks} task${completedLoopTasks !== 1 ? 's' : ''} accomplished`; | ||
| const loopDetails = [ | ||
| `**Loop ${completedLoopNumber} Summary**`, | ||
| '', | ||
| `- **Tasks Accomplished:** ${completedLoopTasks}`, | ||
| `- **Duration:** ${formatElapsedTime(loopElapsedMs)}`, | ||
| loopTotalInputTokens > 0 || loopTotalOutputTokens > 0 | ||
| ? `- **Tokens:** ${(loopTotalInputTokens + loopTotalOutputTokens).toLocaleString()} (${loopTotalInputTokens.toLocaleString()} in / ${loopTotalOutputTokens.toLocaleString()} out)` | ||
| : '', | ||
| loopTotalCost > 0 ? `- **Cost:** $${loopTotalCost.toFixed(4)}` : '', | ||
| `- **Tasks Discovered for Next Loop:** ${newTotalTasks}`, | ||
| ] | ||
| .filter((line) => line !== '') | ||
| .join('\n'); | ||
|
|
||
| onAddHistoryEntry({ | ||
| type: 'AUTO', | ||
| timestamp: Date.now(), | ||
| summary: loopSummary, | ||
| fullResponse: loopDetails, | ||
| projectPath: session.cwd, | ||
| sessionId: sessionId, | ||
| success: true, | ||
| elapsedTimeMs: loopElapsedMs, | ||
| usageStats: | ||
| loopTotalInputTokens > 0 || loopTotalOutputTokens > 0 | ||
| ? { | ||
| inputTokens: loopTotalInputTokens, | ||
| outputTokens: loopTotalOutputTokens, | ||
| cacheReadInputTokens: 0, | ||
| cacheCreationInputTokens: 0, | ||
| totalCostUsd: loopTotalCost, | ||
| contextWindow: 0, | ||
| } | ||
| : undefined, | ||
| }); |
There was a problem hiding this comment.
createLoopSummaryEntry was extracted in batchLoopSummary.ts specifically to centralise loop-summary formatting, but the inter-loop entry (produced after each completed loop when another loop follows) is still built inline here with its own loopSummary/loopDetails strings and a manual onAddHistoryEntry call. Only the final loop exit path (addFinalLoopSummary) uses the helper. Any future change to loop-entry format (e.g. adding a new field) would need to be applied in both places to stay consistent.
| console.assert( | ||
| !sessionId.includes('-batch-'), | ||
| '[BatchProcessor:killBatchRun] sessionId must not contain "-batch-"' | ||
| ); |
There was a problem hiding this comment.
console.assert used for session-ID validation is inconsistent with the rest of the file, which uses the logger utility. In production, console.assert will not be stripped and silently continues execution on failure rather than routing the diagnostic through the normal logging pipeline. Replacing it with a logger.warn call (as used elsewhere in this file) would keep the signal visible in production logs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/hooks/batch/internal/batchProgressPoll.ts`:
- Around line 77-89: The baseline routine marks baselineComputed too early and
silently swallows errors: in computeBaseline() move the assignment to
baselineComputed until after all per-document readDocAndCountTasks() calls have
completed successfully (or after handling per-doc recoverable errors), and stop
using empty catch blocks — for unexpected errors re-throw them (or log with
context via the existing logger) so Sentry can capture them; apply the same
change in tick() where readDocAndCountTasks() is awaited, and ensure
otherDocsTotal and otherDocsChecked are only updated for successful reads while
unexpected IPC/read failures are logged or propagated rather than ignored.
In `@src/renderer/hooks/batch/internal/useBatchBroadcast.ts`:
- Around line 147-149: The catch block in the debounce callback inside
useBatchBroadcast.ts currently only logs the error via logger.error and swallows
it; import and call captureException from src/utils/sentry.ts with the caught
error and context (e.g. a tag or extra field referencing
'[BatchProcessor:onUpdate] debounce callback') in that catch block in addition
to the existing logger.error, and do not re-throw so the debounce scheduler
remains intact.
In `@src/renderer/hooks/batch/internal/useBatchKillAction.ts`:
- Around line 71-76: The kill flow currently sets
stopRequestedRefs.current[sessionId] only after awaiting stats/history and IPC,
which can let the batch loop continue; modify useBatchKillAction’s killBatchRun
to set stopRequestedRefs.current[sessionId] = true immediately before any await
(i.e., before calling timeTracking.getElapsedTime/claimFlushState or invoking
IPC), capture elapsedMs and flushState synchronously if needed, then perform the
awaited work and call onComplete afterwards but off the critical path (wrap
onComplete so its failure doesn’t prevent the stop flag/cleanup), ensuring
functions/refs referenced are killBatchRun, stopRequestedRefs,
timeTracking.getElapsedTime, claimFlushState, autoRunFlushStateRefs, sessionId,
and onComplete.
- Around line 140-159: The current useBatchKillAction flow calls
window.maestro.process.getActiveProcesses and uses Promise.allSettled on
window.maestro.process.kill results but always proceeds to dispatch
COMPLETE_BATCH and broadcast null even if enumeration or kills failed; change it
so that if getActiveProcesses throws you attempt a direct-kill of sessionId
(window.maestro.process.kill(sessionId)) as a fallback, and after
Promise.allSettled inspect each result — if any kill settled as rejected treat
the overall kill as failed (log/report and do NOT dispatch COMPLETE_BATCH or
broadcast null), otherwise only dispatch COMPLETE_BATCH and broadcast null when
all kills are confirmed fulfilled; update error handling/logging around
useBatchKillAction to surface kill failures instead of silently continuing.
In `@src/renderer/hooks/batch/internal/useBatchRunner.ts`:
- Around line 386-387: Wrap the portion that marks a batch active and calls
window.maestro.power.addReason(`autorun:${sessionId}`) inside a try/finally so
cleanup always runs: move flushDebouncedUpdate(), dispatch({ type:
'COMPLETE_BATCH', ... }), stopTracking(...), and
window.maestro.power.removeReason(`autorun:${sessionId}`) into the finally
block; only keep narrowly-scoped catches for explicitly recoverable errors
inside the try (do not swallow other exceptions). Apply the same pattern to the
similar active-run block around lines referenced (1279-1374) to ensure the
session cannot remain stuck and the sleep inhibitor is always removed.
- Around line 197-279: The timeTracking.startTracking(sessionId) is called too
early (before worktree setup and the "no unchecked tasks" early return), causing
elapsed time to leak if the function returns early; move the start call to just
before dispatching START_BATCH/SET_RUNNING (the moment the run is definitely
starting) — i.e., remove the current timeTracking.startTracking(sessionId) and
invoke timeTracking.startTracking(sessionId) immediately before the code that
dispatches START_BATCH/SET_RUNNING for the session (or, alternatively, add
explicit timeTracking.stopTracking(sessionId) on every early return path such as
the worktree setup failure and the initialUncheckedTasks === 0 branch) so every
path that exits also stops tracking.
- Around line 285-334: The broadcast payload sent via broadcastAutoRunState is
out of sync with the reducer state: dispatch({ type: 'START_BATCH', ...
payload.completedTasksAcrossAllDocs: initialCheckedTasks ... }) sets
completedTasksAcrossAllDocs from initialCheckedTasks, but the broadcast sets it
to 0; update the broadcastAutoRunState call in useBatchRunner (the START_BATCH
sequence) to use initialCheckedTasks for completedTasksAcrossAllDocs (and any
other matching fields such as completedTasksAcrossAllDocs / completedTasks if
present) so the initial broadcast mirrors the same pre-checked count the reducer
received.
- Around line 953-993: The catch block after processTask in useBatchRunner
currently decrements remainingTasks on unknown errors, which can silently skip
tasks; instead, when errorResolutionRefs.current[sessionId] is undefined re-read
the document via readDocAndCountTasks (using folderPath, effectiveFilename,
sshRemoteId) to recompute taskCount/checkedCount/content, update remainingTasks,
docCheckedCount, and docContent accordingly (or mark the document as
failed/stalled via a failure flag/skipCurrentDocumentAfterError if you prefer),
and only decrement remainingTasks if you have explicit confirmation a task was
consumed; update the code paths referencing errorResolutionRefs, remainingTasks,
skipCurrentDocumentAfterError, and processTask to reflect this behavior.
🪄 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: d7feedc2-60b3-49ee-b030-40abb954a147
📒 Files selected for processing (18)
src/__tests__/renderer/hooks/batch/batchFinalSummary.test.tssrc/__tests__/renderer/hooks/batch/batchFlushState.test.tssrc/__tests__/renderer/hooks/batch/batchLoopSummary.test.tssrc/__tests__/renderer/hooks/batch/batchProgressPoll.test.tssrc/__tests__/renderer/hooks/batch/useBatchBroadcast.test.tssrc/__tests__/renderer/hooks/batch/useBatchControlActions.test.tssrc/__tests__/renderer/hooks/batch/useBatchKillAction.test.tssrc/__tests__/renderer/hooks/batch/useBatchSelectors.test.tssrc/renderer/hooks/batch/internal/batchFinalSummary.tssrc/renderer/hooks/batch/internal/batchFlushState.tssrc/renderer/hooks/batch/internal/batchLoopSummary.tssrc/renderer/hooks/batch/internal/batchProgressPoll.tssrc/renderer/hooks/batch/internal/useBatchBroadcast.tssrc/renderer/hooks/batch/internal/useBatchControlActions.tssrc/renderer/hooks/batch/internal/useBatchKillAction.tssrc/renderer/hooks/batch/internal/useBatchRunner.tssrc/renderer/hooks/batch/internal/useBatchSelectors.tssrc/renderer/hooks/batch/useBatchProcessor.ts
| const computeBaseline = async () => { | ||
| if (baselineComputed) return; | ||
| baselineComputed = true; | ||
| for (const doc of documents) { | ||
| if (doc.filename === docEntry.filename) continue; | ||
| try { | ||
| const r = await readDocAndCountTasks(folderPath, doc.filename, sshRemoteId); | ||
| otherDocsTotal += r.taskCount + r.checkedCount; | ||
| otherDocsChecked += r.checkedCount; | ||
| } catch { | ||
| // Ignore — baseline is best-effort. | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't freeze a partial baseline behind silent catches.
baselineComputed is set before the per-document reads finish, so a transient readDocAndCountTasks() failure permanently zeros that document out for the lifetime of this controller. The empty catch blocks here and in tick() also hide unexpected IPC/read regressions while polling silently drifts or stalls. Only mark the baseline as computed after the reads succeed, and report unexpected polling failures with context instead of swallowing them.
As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly... For unexpected errors, re-throw them to allow Sentry to capture them."
Also applies to: 140-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/batch/internal/batchProgressPoll.ts` around lines 77 - 89,
The baseline routine marks baselineComputed too early and silently swallows
errors: in computeBaseline() move the assignment to baselineComputed until after
all per-document readDocAndCountTasks() calls have completed successfully (or
after handling per-doc recoverable errors), and stop using empty catch blocks —
for unexpected errors re-throw them (or log with context via the existing
logger) so Sentry can capture them; apply the same change in tick() where
readDocAndCountTasks() is awaited, and ensure otherDocsTotal and
otherDocsChecked are only updated for successful reads while unexpected IPC/read
failures are logged or propagated rather than ignored.
| } catch (error) { | ||
| logger.error('[BatchProcessor:onUpdate] ERROR in debounce callback:', undefined, error); | ||
| } |
There was a problem hiding this comment.
Missing Sentry capture in the debounce catch block.
The caught error is only sent to logger.error and is then discarded. Per the coding guidelines for src/renderer/**/*.{ts,tsx}, unexpected errors must be reported via captureException / captureMessage from src/utils/sentry.ts. Re-throwing here would kill the debounce scheduler, so captureException is the right call.
🛡️ Proposed fix
+import { captureException } from '../../../utils/sentry';
...
} catch (error) {
logger.error('[BatchProcessor:onUpdate] ERROR in debounce callback:', undefined, error);
+ captureException(error, { context: 'BatchProcessor:onUpdate debounce callback' });
}As per coding guidelines: "Do not silently swallow errors. Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/batch/internal/useBatchBroadcast.ts` around lines 147 -
149, The catch block in the debounce callback inside useBatchBroadcast.ts
currently only logs the error via logger.error and swallows it; import and call
captureException from src/utils/sentry.ts with the caught error and context
(e.g. a tag or extra field referencing '[BatchProcessor:onUpdate] debounce
callback') in that catch block in addition to the existing logger.error, and do
not re-throw so the debounce scheduler remains intact.
| try { | ||
| const activeProcesses = await window.maestro.process.getActiveProcesses(); | ||
| const batchProcessIds = activeProcesses | ||
| .filter( | ||
| // Intentional scope: kill the root session process and any descendant | ||
| // auto-run task processes prefixed with `${sessionId}-batch-`. | ||
| (proc) => | ||
| proc.sessionId === sessionId || proc.sessionId.startsWith(`${sessionId}-batch-`) | ||
| ) | ||
| .map((proc) => proc.sessionId); | ||
|
|
||
| // Fallback to legacy direct ID in case process listing is stale. | ||
| if (batchProcessIds.length === 0) { | ||
| batchProcessIds.push(sessionId); | ||
| } | ||
|
|
||
| await Promise.allSettled(batchProcessIds.map((id) => window.maestro.process.kill(id))); | ||
| } catch (error) { | ||
| logger.error('[BatchProcessor:killBatchRun] Failed to kill process:', undefined, error); | ||
| } |
There was a problem hiding this comment.
Don't mark the batch complete when process termination was only best-effort.
This path logs getActiveProcesses() failures and ignores every rejected result from Promise.allSettled(), but it still immediately dispatches COMPLETE_BATCH and broadcasts null. If listing fails or every kill() rejects, the UI is cleaned up while the agent process may still be running. Add a direct-kill fallback when enumeration fails, inspect the settled results, and gate the completion/reset path on confirmed termination (or surface a kill failure instead).
Also applies to: 174-182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/batch/internal/useBatchKillAction.ts` around lines 140 -
159, The current useBatchKillAction flow calls
window.maestro.process.getActiveProcesses and uses Promise.allSettled on
window.maestro.process.kill results but always proceeds to dispatch
COMPLETE_BATCH and broadcast null even if enumeration or kills failed; change it
so that if getActiveProcesses throws you attempt a direct-kill of sessionId
(window.maestro.process.kill(sessionId)) as a fallback, and after
Promise.allSettled inspect each result — if any kill settled as rejected treat
the overall kill as failed (log/report and do NOT dispatch COMPLETE_BATCH or
broadcast null), otherwise only dispatch COMPLETE_BATCH and broadcast null when
all kills are confirmed fulfilled; update error handling/logging around
useBatchKillAction to surface kill failures instead of silently continuing.
| // Prevent system sleep while Auto Run is active | ||
| window.maestro.power.addReason(`autorun:${sessionId}`); |
There was a problem hiding this comment.
Protect the active run with try/finally cleanup.
Once the batch is marked active and power.addReason() is set, an unexpected throw anywhere in the body can skip flushDebouncedUpdate, COMPLETE_BATCH, stopTracking, and removeReason. That leaves the session stuck in a running state and can keep the sleep inhibitor around indefinitely. Wrap the active-run section in try/finally, and only catch explicitly recoverable cases inside it.
As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."
Also applies to: 1279-1374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/batch/internal/useBatchRunner.ts` around lines 386 - 387,
Wrap the portion that marks a batch active and calls
window.maestro.power.addReason(`autorun:${sessionId}`) inside a try/finally so
cleanup always runs: move flushDebouncedUpdate(), dispatch({ type:
'COMPLETE_BATCH', ... }), stopTracking(...), and
window.maestro.power.removeReason(`autorun:${sessionId}`) into the finally
block; only keep narrowly-scoped catches for explicitly recoverable errors
inside the try (do not swallow other exceptions). Apply the same pattern to the
similar active-run block around lines referenced (1279-1374) to ensure the
session cannot remain stuck and the sleep inhibitor is always removed.
| } catch (error) { | ||
| progressPoll.stop(); | ||
| logger.error( | ||
| `[BatchProcessor] Error running task in ${docEntry.filename} for session ${sessionId}:`, | ||
| undefined, | ||
| error | ||
| ); | ||
|
|
||
| // Check if an error resolution promise was created (e.g., by onAgentError → pauseBatchOnError) | ||
| // This handles the case where the agent error (e.g., context limit) triggered a pause, | ||
| // but processTask threw before the next loop iteration could check for it. | ||
| const postTaskErrorResolution = errorResolutionRefs.current[sessionId]; | ||
| if (postTaskErrorResolution) { | ||
| const action = await postTaskErrorResolution.promise; | ||
| delete errorResolutionRefs.current[sessionId]; | ||
|
|
||
| if (action === 'abort') { | ||
| stopRequestedRefs.current[sessionId] = true; | ||
| break; | ||
| } | ||
|
|
||
| if (action === 'skip-document') { | ||
| skipCurrentDocumentAfterError = true; | ||
| break; | ||
| } | ||
|
|
||
| // 'resume' — re-read document to get accurate task count before continuing | ||
| const { | ||
| taskCount, | ||
| checkedCount, | ||
| content: freshContent, | ||
| } = await readDocAndCountTasks(folderPath, effectiveFilename, sshRemoteId); | ||
| remainingTasks = taskCount; | ||
| docCheckedCount = checkedCount; | ||
| docContent = freshContent; | ||
| continue; | ||
| } | ||
|
|
||
| // No error resolution pending — continue to next task on error | ||
| remainingTasks--; | ||
| } |
There was a problem hiding this comment.
Don't pretend a task disappeared after an unknown failure.
When processTask throws without creating an error-resolution promise, remainingTasks-- makes the loop advance as if one unchecked task was consumed even if the document is unchanged. That can silently skip real work or end a single-pass run early. Re-read the document/task counts here instead, or mark the document as failed/stalled explicitly.
Suggested fix
// No error resolution pending — continue to next task on error
- remainingTasks--;
+ const {
+ taskCount,
+ checkedCount,
+ content: freshContent,
+ } = await readDocAndCountTasks(folderPath, effectiveFilename, sshRemoteId);
+ remainingTasks = taskCount;
+ docCheckedCount = checkedCount;
+ docContent = freshContent;📝 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.
| } catch (error) { | |
| progressPoll.stop(); | |
| logger.error( | |
| `[BatchProcessor] Error running task in ${docEntry.filename} for session ${sessionId}:`, | |
| undefined, | |
| error | |
| ); | |
| // Check if an error resolution promise was created (e.g., by onAgentError → pauseBatchOnError) | |
| // This handles the case where the agent error (e.g., context limit) triggered a pause, | |
| // but processTask threw before the next loop iteration could check for it. | |
| const postTaskErrorResolution = errorResolutionRefs.current[sessionId]; | |
| if (postTaskErrorResolution) { | |
| const action = await postTaskErrorResolution.promise; | |
| delete errorResolutionRefs.current[sessionId]; | |
| if (action === 'abort') { | |
| stopRequestedRefs.current[sessionId] = true; | |
| break; | |
| } | |
| if (action === 'skip-document') { | |
| skipCurrentDocumentAfterError = true; | |
| break; | |
| } | |
| // 'resume' — re-read document to get accurate task count before continuing | |
| const { | |
| taskCount, | |
| checkedCount, | |
| content: freshContent, | |
| } = await readDocAndCountTasks(folderPath, effectiveFilename, sshRemoteId); | |
| remainingTasks = taskCount; | |
| docCheckedCount = checkedCount; | |
| docContent = freshContent; | |
| continue; | |
| } | |
| // No error resolution pending — continue to next task on error | |
| remainingTasks--; | |
| } | |
| } catch (error) { | |
| progressPoll.stop(); | |
| logger.error( | |
| `[BatchProcessor] Error running task in ${docEntry.filename} for session ${sessionId}:`, | |
| undefined, | |
| error | |
| ); | |
| // Check if an error resolution promise was created (e.g., by onAgentError → pauseBatchOnError) | |
| // This handles the case where the agent error (e.g., context limit) triggered a pause, | |
| // but processTask threw before the next loop iteration could check for it. | |
| const postTaskErrorResolution = errorResolutionRefs.current[sessionId]; | |
| if (postTaskErrorResolution) { | |
| const action = await postTaskErrorResolution.promise; | |
| delete errorResolutionRefs.current[sessionId]; | |
| if (action === 'abort') { | |
| stopRequestedRefs.current[sessionId] = true; | |
| break; | |
| } | |
| if (action === 'skip-document') { | |
| skipCurrentDocumentAfterError = true; | |
| break; | |
| } | |
| // 'resume' — re-read document to get accurate task count before continuing | |
| const { | |
| taskCount, | |
| checkedCount, | |
| content: freshContent, | |
| } = await readDocAndCountTasks(folderPath, effectiveFilename, sshRemoteId); | |
| remainingTasks = taskCount; | |
| docCheckedCount = checkedCount; | |
| docContent = freshContent; | |
| continue; | |
| } | |
| // No error resolution pending — continue to next task on error | |
| const { | |
| taskCount, | |
| checkedCount, | |
| content: freshContent, | |
| } = await readDocAndCountTasks(folderPath, effectiveFilename, sshRemoteId); | |
| remainingTasks = taskCount; | |
| docCheckedCount = checkedCount; | |
| docContent = freshContent; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/batch/internal/useBatchRunner.ts` around lines 953 - 993,
The catch block after processTask in useBatchRunner currently decrements
remainingTasks on unknown errors, which can silently skip tasks; instead, when
errorResolutionRefs.current[sessionId] is undefined re-read the document via
readDocAndCountTasks (using folderPath, effectiveFilename, sshRemoteId) to
recompute taskCount/checkedCount/content, update remainingTasks,
docCheckedCount, and docContent accordingly (or mark the document as
failed/stalled via a failure flag/skipCurrentDocumentAfterError if you prefer),
and only decrement remainingTasks if you have explicit confirmation a task was
consumed; update the code paths referencing errorResolutionRefs, remainingTasks,
skipCurrentDocumentAfterError, and processTask to reflect this behavior.
|
Thanks for tackling this @reachrazamair — taking a 2,281-line hook down to a 328-line coordinator is genuinely impressive work, and the unit-test coverage on the extracted helpers is excellent. The decision rationale captured inline (kill ordering, ref ownership, async CodeRabbit and Greptile raised quite a few items between them. Most of the larger correctness concerns (Sentry capture in the debounce catch, A few smaller items I'd like resolved in this PR before approval, since they're either purely artefacts of the extraction or genuinely trivial fixes:
The remaining bot findings I'm fine deferring or treating as out-of-scope, but happy to discuss if you'd prefer to address any of them here. Once 1–4 are addressed I'll approve. |
…al broadcast Three small follow-ups to the useBatchProcessor decomposition. Each addresses a pre-existing issue in code that was preserved byte-for-byte by the refactor; fixed now while the seams are fresh. useBatchKillAction: - Set stopRequestedRefs.current[sessionId] = true synchronously before any await, so the processing loop has the earliest possible chance to observe the stop request during the kill's IPC awaits (stats, history, process enumeration). Previously the flag was only set after the kill awaits completed, so a loop that recovered from a process-kill rejection could re-spawn an agent for the next task before observing the stop. - Wrap onComplete in try/catch so a callback failure can't block the COMPLETE_BATCH dispatch, web/mobile broadcast, stopTracking, or power.removeReason cleanup that follows. useBatchRunner: - Add timeTracking.stopTracking(sessionId) on the two early-return paths that occur after startTracking (worktree-setup-failed, no-unchecked-tasks). Without this, the tracker leaked across runs: a subsequent getElapsedTime(sessionId) call would return phantom time accumulated since the failed start. - Broadcast completedTasksAcrossAllDocs: initialCheckedTasks (was 0) on the initial state change, mirroring what the reducer just stored. Removes a brief 0/N flicker on mobile/web before the next progress update arrives. completedTasks remains 0 because the reducer also hardcodes that legacy field to 0 in START_BATCH. Tests: - Added 2 unit tests to useBatchKillAction.test.ts: one verifies the stop flag is set synchronously before any await fires (using a never-resolving IPC mock); one verifies COMPLETE_BATCH + broadcast + stopTracking still run when onComplete throws. - Switched the test setup's mockClear to mockReset + mockResolvedValue defaults so per-test mock implementations no longer bleed across tests. Full project suite: 27,605 passing / 108 skipped / 0 failing.
…ace console.assert
Three small follow-ups to the useBatchProcessor decomposition.
useBatchBroadcast:
- Rename _scheduleDebouncedUpdate -> scheduleDebouncedUpdate. The underscore
prefix conventionally signals "unused" but the binding is read inside
updateBatchStateAndBroadcast, so the prefix was actively misleading.
batchLoopSummary + useBatchRunner:
- Add optional tasksDiscoveredForNextLoop?: number to LoopSummaryParams.
When set, the helper appends "Tasks Discovered for Next Loop: N" to the
details. Final-loop entries (which have exitReason instead) leave it
undefined.
- Replace the ~40-line inline mid-run loop summary block in
useBatchRunner.ts with a single createLoopSummaryEntry({ ..., isFinal:
false, tasksDiscoveredForNextLoop: newTotalTasks }) call. Both the
inter-loop and final-loop paths now share one format, eliminating the
drift risk where a future format change had to be applied in two places.
- Drop the now-unused formatElapsedTime import from useBatchRunner.
useBatchKillAction:
- Replace console.assert(!sessionId.includes('-batch-'), ...) with an
explicit if + logger.warn. console.assert is a no-op in production builds
and silently continues on failure; logger.warn reaches the same telemetry
pipeline as the rest of this file.
Tests:
- Added 2 unit tests to batchLoopSummary.test.ts covering the new
tasksDiscoveredForNextLoop param (set and omitted).
- The existing integration suite continues to exercise the mid-run loop
path end-to-end with the helper now wired in.
Full project suite: 27,607 passing / 108 skipped / 0 failing.
|
@CodeRabbit review. |
|
✅ Actions performedReview triggered.
|
2 participants
Summary
Decomposes useBatchProcessor.ts — the largest hook in the codebase at 2,281 lines — into a thin coordinator (328 LOC) plus 6 internal hooks and 4 pure helpers under
internal/. Closes §4.3 of the Tier 4 backlog. Public API and behaviour are unchanged; the 165-test integration suite passes via the existing barrel.useEffect, and hook composition — nothing else.useBatchSelectors,useBatchBroadcast,useBatchControlActions,useBatchKillAction,useBatchRunner) and four pure helpers (batchFlushState,batchLoopSummary,batchFinalSummary,batchProgressPoll), each with its own SRP.claimFlushState(refs, sessionId)primitive. Both the runner's natural-completion path andkillBatchRunnow use the same atomic read-and-delete — removing the most subtle correctness invariant in the file from a "scattered across two files" failure mode.useBatchQueue / Scheduler / Dispatch / Results) was speculative — the processor is fundamentally serial-per-session with no row queue, no concurrency limits, no slot management. The actual extracted shape reflects the real seams. Doc updated to match.What's in the diff
Behaviour delta
None — byte-equivalent. Same public API, same broadcast cadence (200 ms debounce), same stall heuristic (3 consecutive no-progress runs), same progress-poll generation guard (now hidden inside
createProgressPollrather than leaked as fourletbindings), same kill-vs-natural-completion arbitration.updateBatchStateAndBroadcastRefremains in the coordinator so the long-running async runner survives HMR re-renders across module boundaries (Vite invalidates per-module).Design decisions
stopBatchRunintouseBatchControlActionsrather than extracting a fourth single-method hook — the 18-line stop callback shares botherrorResolutionRefsandSET_STOPPINGwithabortBatchOnError, so a separate file would have been pure ceremony.claimFlushStateBEFORE the runner/kill split so both call sites use the same primitive from day one.stopRequestedRefs,errorResolutionRefs,autoRunFlushStateRefs,isMountedRef,sessionsRef,audioFeedback*Refs,updateBatchStateAndBroadcastRef) because each has ≥3 readers/writers across the split — per-hook ownership would have forced getter/setter plumbing for state that's intrinsically shared.createProgressPoll.start()is async to preserve the original baseline-then-schedule sequence (the runner relies on the synchronous baseline read finishing beforeprocessTaskbegins).Test coverage
batch/batchLoopSummary.test.tsbatch/batchFinalSummary.test.tsbatch/batchFlushState.test.tsbatch/batchProgressPoll.test.tsbatch/useBatchSelectors.test.tsbatch/useBatchBroadcast.test.tsbatch/useBatchControlActions.test.tsbatch/useBatchKillAction.test.ts${sessionId}-batch-prefix, fallback to legacy direct ID, stopRequestedRefs[sessionId] intentionally not deletedFull project suite: 27,603 passing / 108 skipped / 0 failing (was 27,468 baseline).
Verification
npm run test— full suite greennpx tsc --noEmit— zero type errorsnpm run lint:eslint— zero lint errorsOut of scope
useTimeTracking,useWorktreeManager,useDocumentProcessor,useSessionDebounce) remain unchanged.countUnfinishedTasks/uncheckAllTasksare still re-exported from useBatchProcessor.ts for backwards compat — they were already extracted to batchUtils.ts in a prior pass.Summary by CodeRabbit
New Features
Bug Fixes
Tests