feat(terminal): centralized auto-resume queue for terminal recovery#1835
feat(terminal): centralized auto-resume queue for terminal recovery#1835
Conversation
…on recovery When the app restarts, only the active terminal auto-resumes its Claude session. All other terminals show a pending resume badge and require manual activation. This is because the deferred resume useEffect in Terminal.tsx gates on isActive === true. Add a module-level auto-resume queue coordinator in terminal-store.ts that processes non-active terminals with staggered delays (1.5s initial, 500ms between each). Terminals enqueue themselves in the onCreated callback when their PTY is ready and they have a pending resume. Active terminals dequeue themselves since they handle their own resume. The queue is cleared on new restore operations to prevent stale entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
area/frontend
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where only the active terminal would automatically resume its Claude session after an application restart, leaving other terminals requiring manual intervention. The changes introduce a robust, centralized auto-resume queue mechanism that ensures all non-active terminals with pending Claude sessions are automatically resumed in a staggered manner, significantly improving the user experience by eliminating the need for manual interaction. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a global auto-resume queue and APIs ( Changes
Sequence Diagram(s)sequenceDiagram
participant Terminal as Terminal Component
participant Queue as Auto-Resume Queue
participant Store as Terminal Store
participant IPC as IPC Layer
Terminal->>Queue: enqueueAutoResume(id) after PTY created (if inactive & pendingClaudeResume)
Queue->>Queue: processAutoResumeQueue() (initial delay, staggered loop)
loop for each queued id
Queue->>Store: check pendingClaudeResume(id)?
alt still pending
Queue->>Store: resumeTerminalClaudeSession(id)
Store->>IPC: send resumeClaudeSession IPC
IPC->>Store: confirm / clear pending flag
else skip
end
Queue->>Queue: wait AUTO_RESUME_STAGGER_MS
end
Terminal->>Queue: dequeueAutoResume(id) on activation / cleanup / unmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized auto-resume queue to ensure all non-active terminals with pending Claude sessions are automatically resumed upon application restart. The implementation uses a coordinator in terminal-store.ts with staggered delays to manage the resume process, and terminals enqueue themselves once their PTY is ready. This is a solid architectural improvement that addresses a user experience issue where only the active terminal would auto-resume.
My review includes a high-severity fix for a potential race condition that could prevent immediate resume of a terminal when it becomes active, and a medium-severity suggestion to refactor the queue implementation to use a Set for better code clarity and idiomatic correctness.
| if (!isActive && !hasAttemptedAutoResumeRef.current) { | ||
| const currentTerminal = useTerminalStore.getState().terminals.find(t => t.id === id); | ||
| if (currentTerminal?.pendingClaudeResume) { | ||
| hasAttemptedAutoResumeRef.current = true; | ||
| enqueueAutoResume(id); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential bug here. Setting hasAttemptedAutoResumeRef.current = true when a non-active terminal is enqueued prevents it from being resumed immediately if it becomes active before the queue processor handles it. The useEffect that handles active terminal resumes will see hasAttemptedAutoResumeRef.current as true and skip the resume logic, leading to a poor user experience.
The hasAttemptedAutoResumeRef should only be set when a resume is actually triggered, not when it's just enqueued. The enqueueAutoResume function already prevents duplicate entries in the queue, and the onCreated callback only runs once per PTY creation, so there's no risk of multiple enqueues from here.
I suggest removing the check for hasAttemptedAutoResumeRef and the line that sets it.
if (!isActive) {
const currentTerminal = useTerminalStore.getState().terminals.find(t => t.id === id);
if (currentTerminal?.pendingClaudeResume) {
enqueueAutoResume(id);
}
}
| const AUTO_RESUME_INITIAL_DELAY_MS = 1500; | ||
| const AUTO_RESUME_STAGGER_MS = 500; | ||
|
|
||
| let autoResumeQueue: string[] = []; |
There was a problem hiding this comment.
For managing a collection of unique terminal IDs, using a Set would be more idiomatic and slightly more performant than an array. A Set provides O(1) average time complexity for add, delete, and has operations, compared to O(n) for Array.prototype.includes and Array.prototype.splice.
While the performance difference is negligible with a maximum of 12 terminals, using a Set better expresses the intent that the queue contains unique items and simplifies the implementation.
To implement this, you would change this line and update the related functions:
- Declaration:
let autoResumeQueue = new Set<string>(); enqueueAutoResume: ReplaceautoResumeQueue.includes(terminalId)withautoResumeQueue.has(terminalId)andautoResumeQueue.push(terminalId)withautoResumeQueue.add(terminalId).dequeueAutoResume: The body can be simplified toif (autoResumeQueue.delete(terminalId)) { ... }.clearAutoResumeQueue: UseautoResumeQueue.clear()instead ofautoResumeQueue = [].processAutoResumeQueue: The loop condition becomeswhile (autoResumeQueue.size > 0). To get and remove an item, you can do:const terminalId = autoResumeQueue.values().next().value; autoResumeQueue.delete(terminalId);.
| let autoResumeQueue: string[] = []; | |
| let autoResumeQueue = new Set<string>(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/stores/terminal-store.ts (1)
510-540:⚠️ Potential issue | 🟠 Major
resumeAllPendingClaudeduplicatesprocessAutoResumeQueuelogic with a race condition risk.Both methods iterate terminals with
pendingClaudeResume === true, clear the flag, callactivateDeferredClaudeResume, and stagger with 500ms delays. However,processAutoResumeQueueis guarded by theautoResumeProcessingflag whileresumeAllPendingClaudeis not. A user clicking "Resume All" whileprocessAutoResumeQueueis mid-execution (across its 500ms stagger window) can cause the same terminal to be resumed twice, triggering duplicate IPC calls.Recommend either removing
resumeAllPendingClaudeand triggeringprocessAutoResumeQueueon user action, or consolidating logic by havingresumeAllPendingClaudeadd pending terminals to the queue and respect theautoResumeProcessingguard.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/components/Terminal.tsx`:
- Around line 384-391: The onCreated callback closes over the render-time
isActive causing a stale check and potential unnecessary enqueue→dequeue work;
update onCreated to read the current active state from a ref (e.g.,
isActiveRef.current) or from the store (useTerminalStore.getState()) instead of
the closed-over isActive, then use that value when deciding to set
hasAttemptedAutoResumeRef.current and call enqueueAutoResume(id) so the decision
reflects the terminal's real-time active state; alternatively add a brief inline
comment in onCreated referencing that the dequeue in the active-terminal effect
is the authoritative guard to clarify the design.
In `@apps/frontend/src/renderer/stores/terminal-store.ts`:
- Around line 87-89: The module-level auto-resume queue (autoResumeQueue), timer
and flag (autoResumeTimer, autoResumeProcessing) can be mutated by re-entrant
calls to enqueueAutoResume/dequeueAutoResume while processAutoResumeQueue
awaits, so add a short comment by those declarations explaining the
single-threaded JS assumption and the potential microtask interleaving; then
make processAutoResumeQueue robust by taking a local batch copy or by setting
autoResumeProcessing = true before any awaits and re-checking the queue length
after awaits (or draining a snapshot of the queue) so that enqueueAutoResume and
dequeueAutoResume cannot violate the draining invariant — reference the
functions enqueueAutoResume, processAutoResumeQueue, dequeueAutoResume and
clearAutoResumeQueue when updating comments and logic.
- Around line 114-151: Introduce a generation counter to prevent stale
processAutoResumeQueue runs from touching newly enqueued items: add a
module-level autoResumeGeneration number that clearAutoResumeQueue increments
when it resets the queue; in processAutoResumeQueue read that generation into a
local const (e.g., currentGen) at start and whenever about to process an item or
after the stagger await, bail out if autoResumeGeneration !== currentGen; also
set autoResumeProcessing only for the run that captured the generation and
ensure enqueueAutoResume uses autoResumeProcessing and the generation semantics
consistently so only the latest processor handles newly pushed terminals. This
keeps the existing behavior of clearAutoResumeQueue, autoResumeQueue,
autoResumeTimer, AUTO_RESUME_STAGGER_MS, and
window.electronAPI.activateDeferredClaudeResume while preventing cross-run
races.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 issue(s) require attention.
1 issue(s) must be addressed (0 required, 1 recommended), 1 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 1 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (2 selected of 2 total)
🟡 [bad130323e1d] [MEDIUM] resumeAllPendingClaude doesn't clear the auto-resume queue
📁 apps/frontend/src/renderer/stores/terminal-store.ts:510
The existing resumeAllPendingClaude store action (triggered by the manual 'Resume All' button in TerminalHeader.tsx:176) performs the same staggered resume logic as the new processAutoResumeQueue but doesn't interact with the auto-resume queue at all. The PR carefully integrates the queue with other resume paths: restoreTerminalSessions calls clearAutoResumeQueue() (line 584), the active terminal path calls dequeueAutoResume(id) (line 585 in Terminal.tsx), and unmount cleanup calls dequeueAutoResume(id) (line 643). But resumeAllPendingClaude is missed. If a user clicks 'Resume All' while the auto-resume queue is still in its 1.5s initial delay, both mechanisms will process the same terminals. The guard check (!terminal?.pendingClaudeResume) prevents double-resume, so this isn't a bug, but it's an incomplete integration. Searched Grep('resumeAllPendingClaude', 'apps/frontend/src') - confirmed it's used in TerminalHeader.tsx and does not call clearAutoResumeQueue() or dequeueAutoResume(). | The existing resumeAllPendingClaude method (user-triggered via 'Resume All' button) does not call clearAutoResumeQueue(). If the user clicks 'Resume All' while the auto-resume queue is processing, both paths will attempt to resume the same terminals. The main process guards against actual double-resume (activateDeferredResume checks its own pendingClaudeResume flag on line 246 of terminal-manager.ts), so this causes no user-visible bug, but it results in redundant IPC messages and debug log noise. Adding clearAutoResumeQueue() at the start of resumeAllPendingClaude would cleanly coordinate the two paths.
Suggested fix:
Add `clearAutoResumeQueue()` at the start of `resumeAllPendingClaude` to prevent the queue from redundantly processing terminals that are already being handled by the manual resume action. This aligns with how `restoreTerminalSessions` already calls `clearAutoResumeQueue()`.
🔵 [6ab5c830de66] [LOW] Duplicated staggered-resume logic with resumeAllPendingClaude
📁 apps/frontend/src/renderer/stores/terminal-store.ts:124
The new processAutoResumeQueue (lines 124-151) and the existing resumeAllPendingClaude (lines 510-540) implement nearly identical staggered-resume patterns: iterate pending terminals, clear pendingClaudeResume, call activateDeferredClaudeResume, await a 500ms delay between each. They serve different triggers (automatic vs user-initiated) but could share a common helper function to reduce duplication and ensure consistent behavior (e.g., consistent stagger timing, consistent error patterns).
Suggested fix:
Extract a shared helper like `async function resumeTerminalWithStagger(terminalId: string)` that handles clearing the pending flag and calling the IPC, and refactor both `processAutoResumeQueue` and `resumeAllPendingClaude` to use it.
This review was generated by Auto Claude.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| return () => { | ||
| isMountedRef.current = false; | ||
| dequeueAutoResume(id); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add clearAutoResumeQueue() to resumeAllPendingClaude to prevent redundant processing - Extract shared resumeTerminalClaudeSession() helper to eliminate code duplication - Use AUTO_RESUME_STAGGER_MS constant consistently for stagger delays Fixes: - MEDIUM: resumeAllPendingClaude now clears auto-resume queue to coordinate with automatic processing - LOW: Shared helper function ensures consistent resume behavior across both code paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
size/M
| resumeAllPendingClaude: async () => { | ||
| // Clear auto-resume queue to prevent redundant processing | ||
| clearAutoResumeQueue(); | ||
|
|
||
| const state = get(); | ||
|
|
||
| // Filter terminals with pending Claude resume |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 1 previous findings remain
- 🆕 New Issues: 4 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 4 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: clearAutoResumeQueue race condition with processAutoResumeQueue
Verdict
CI Status: ✅ All 20 checks passing — no CI blockers.
Previous findings: The MEDIUM finding (bad130323e1d) about resumeAllPendingClaude not clearing the auto-resume queue is fully RESOLVED — it now calls clearAutoResumeQueue() at the start. The LOW finding (6ab5c830de66) about duplicated logic is PARTIALLY RESOLVED — a shared resumeTerminalClaudeSession helper and AUTO_RESUME_STAGGER_MS constant were extracted, but the stagger loop pattern itself remains structurally duplicated.
New MEDIUM finding blocks merge: NEW-001 identifies a race condition where clearAutoResumeQueue() resets the autoResumeProcessing flag while processAutoResumeQueue() is mid-flight (awaiting stagger delay). When the old instance's await resolves, it exits the while loop (since the queue is empty) but then sets autoResumeProcessing = false, potentially clobbering a new instance's flag. The fix is straightforward: use an epoch/generation counter instead of a boolean flag, or check for cancellation after each await. While the practical exploitation window is narrow, the pattern is incorrect and the fix is simple.
2 findings dismissed as false positives: NEW-002 (clearAllTerminals missing queue clear) was dismissed because processAutoResumeQueue already checks terminal existence before each resume, and clearAllTerminals has no callers. NEW-004 (linear scan performance) was dismissed because the queue is bounded at 12 items.
Good progress: The author addressed both original findings meaningfully — the shared helper is a clean solution and the queue clearing integration is correct. Only the newly identified race condition in the concurrent processing logic needs attention.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 4 total)
🟡 [NEW-001] [MEDIUM] clearAutoResumeQueue race condition with processAutoResumeQueue
📁 apps/frontend/src/renderer/stores/terminal-store.ts:114
When clearAutoResumeQueue() is called while processAutoResumeQueue() is mid-flight (awaiting the stagger delay), it sets autoResumeProcessing = false. The old instance's while loop exits (queue is empty), but then it sets autoResumeProcessing = false at line 157, potentially clobbering a new instance that started after the clear. This breaks the concurrency guard, allowing concurrent processAutoResumeQueue instances. Fix: use an epoch/generation counter — increment on clear, capture at processAutoResumeQueue start, check after each await to detect cancellation.
Suggested fix:
Use a generation counter: let autoResumeEpoch = 0; increment in clearAutoResumeQueue(); capture const myEpoch = autoResumeEpoch at start of processAutoResumeQueue(); check autoResumeEpoch !== myEpoch after each await to exit without resetting the flag.
🔵 [NEW-003] [LOW] Stale isActive closure in onCreated callback
📁 apps/frontend/src/renderer/components/Terminal.tsx:385
The onCreated callback captures isActive from the render closure. If the user switches tabs between PTY creation start and onCreated firing, stale isActive=true skips auto-resume enqueue. The terminal won't be background-resumed until the user manually clicks on it. Fix: read activeTerminalId from the store inside the callback instead of using the closure variable.
Suggested fix:
Replace `if (!isActive && ...)` with `const isCurrentlyActive = useTerminalStore.getState().activeTerminalId === id; if (!isCurrentlyActive && ...)`
🔵 [CMT-001] [LOW] Missing concurrency guard on resumeAllPendingClaude
📁 apps/frontend/src/renderer/stores/terminal-store.ts:518
Unlike processAutoResumeQueue which has the autoResumeProcessing guard, resumeAllPendingClaude has no concurrency guard. Rapid double-clicks on the 'Resume All' button during 500ms stagger delays can cause concurrent executions with overlapping pendingTerminals snapshots, leading to duplicate activateDeferredClaudeResume IPC calls.
Suggested fix:
Add a module-level isResumingAll flag checked at function entry and reset at exit, matching the autoResumeProcessing pattern. Alternatively, disable the button in the UI during the async operation.
🔵 [CMT-001] [LOW] [FROM COMMENTS] Missing concurrency guard on resumeAllPendingClaude
📁 apps/frontend/src/renderer/stores/terminal-store.ts:518
Validated from sentry[bot] bug prediction: resumeAllPendingClaude lacks a concurrency guard, allowing rapid double-clicks to cause duplicate IPC calls during stagger delays.
This review was generated by Auto Claude.
…tale closures Fixes identified in follow-up review (2026-02-15 18:31 UTC): HIGH PRIORITY: - [Gemini HIGH] Remove hasAttemptedAutoResumeRef from onCreated enqueue logic Previously setting the ref when enqueuing prevented immediate resume if terminal became active before queue processed. Now only active terminal resume sets the ref. - [CodeRabbit MAJOR] Add generation counter to prevent race conditions When clearAutoResumeQueue() is called during processAutoResumeQueue() await, the old instance could consume newly enqueued items. Generation counter now aborts stale processing runs. MEDIUM PRIORITY: - [NEW-001 MEDIUM] Generation counter also fixes clearAutoResumeQueue race Same fix as above - prevents cross-run race conditions. - [CMT-001 LOW] Add concurrency guard to resumeAllPendingClaude Rapid clicks could cause concurrent executions with duplicate IPC calls. Added isResumingAll flag with try/finally pattern. LOW PRIORITY: - [NEW-003 LOW] Fix stale isActive closure in onCreated callback Read activeTerminalId from store instead of using render-time closure value to ensure accurate active state check. Technical details: - autoResumeGeneration counter incremented on clearAutoResumeQueue() - processAutoResumeQueue() captures generation, checks after each await - isResumingAll concurrency guard added with proper try/finally cleanup - onCreated reads current active state from store, not closure - Removed hasAttemptedAutoResumeRef.current usage from enqueue path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts`:
- Around line 560-572: The loop in resumeAllPendingClaude snapshots
pendingTerminals and calls resumeTerminalClaudeSession after each stagger
without re-checking whether the terminal still has pendingClaudeResume, which
can trigger duplicate activateDeferredClaudeResume IPC calls; modify
resumeAllPendingClaude to, before each call to
resumeTerminalClaudeSession(terminal.id), re-fetch the current terminal state
(or check terminal?.pendingClaudeResume) from the store and only call
resumeTerminalClaudeSession if that fresh terminal.pendingClaudeResume is true —
mirror the guard used in processAutoResumeQueue and keep the
AUTO_RESUME_STAGGER_MS delay between iterations.
1. Remove autoResumeProcessing reset in clearAutoResumeQueue()
- Setting the flag to false created a race condition allowing concurrent
processAutoResumeQueue() instances
- The generation counter is sufficient to abort stale processing runs
- Fixes potential duplicate resume attempts
2. Add error handling in resumeAllPendingClaude()
- Wrap resumeTerminalClaudeSession() in try/catch
- Ensures one terminal's error doesn't block remaining terminals
- Improves robustness of manual "Resume All" button
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| autoResumeQueue = []; | ||
| if (autoResumeTimer !== null) { | ||
| clearTimeout(autoResumeTimer); | ||
| autoResumeTimer = null; | ||
| } | ||
| // Don't reset autoResumeProcessing here - the generation counter is sufficient | ||
| // to abort stale processing runs. Resetting the flag here creates a race condition | ||
| // where clearAutoResumeQueue() could allow a new processAutoResumeQueue() to start | ||
| // before the aborted one has fully exited. | ||
| autoResumeGeneration++; // Increment generation to abort any in-flight processing | ||
| debugLog('[AutoResume] Queue cleared'); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts`:
- Around line 146-181: The processor leaves autoResumeProcessing true when it
aborts on a generation mismatch, so clearAutoResumeQueue should reset that flag
to allow future processing: in clearAutoResumeQueue() (and where it increments
autoResumeGeneration) set autoResumeProcessing = false immediately after bumping
autoResumeGeneration so the stale run cannot block future enqueueAutoResume()
calls; keep the generation bump first (to avoid races) and do not change
processAutoResumeQueue() logic other than ensuring any early-return paths also
won't conflict with the reset already made by clearAutoResumeQueue().
- Around line 140-181: In processAutoResumeQueue, wrap the per-item call to
resumeTerminalClaudeSession(terminalId) in a try/catch so any synchronous or
async throw from resumeTerminalClaudeSession, setPendingClaudeResume, or
activateDeferredClaudeResume is caught and logged and does not escape the while
loop; ensure each item uses its own try/catch (so the loop continues to the next
terminal on error) and keep the existing generation checks and stagger logic,
and also ensure autoResumeProcessing is reliably reset for the current
generation by preserving the existing final check (generation ===
autoResumeGeneration) or adding a finally that only clears autoResumeProcessing
when the captured generation still matches autoResumeGeneration; reference
processAutoResumeQueue, autoResumeProcessing, autoResumeGeneration,
autoResumeQueue, resumeTerminalClaudeSession, and AUTO_RESUME_STAGGER_MS when
making the changes.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 4 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 4 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 6 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 2 findings verified as genuine issues
- 👤 Needs Human Review: 1 findings require manual verification
🚨 Blocking Issues
- quality: autoResumeProcessing permanently jams after clearAutoResumeQueue() interrupts processing
- quality: Missing try/catch in processAutoResumeQueue permanently jams queue on exception
- quality: [FROM COMMENTS] sentry[bot]: Project switching deadlock permanently disables auto-resume (same as NEW-RC-001)
- quality: [FROM COMMENTS] CodeRabbit: Missing error handling in processAutoResumeQueue (same as NEW-RC-002)
Verdict
CI Status: ✅ All 20 checks passing — no CI blockers.
Previous findings: All 4 previous findings (NEW-001, NEW-003, CMT-001, CMT-001 dup) are RESOLVED. The generation counter pattern addresses the original race condition, the stale closure fix reads from the store directly, and the isResumingAll concurrency guard prevents duplicate Resume All executions.
New findings: 2 HIGH severity findings confirmed valid that block merge:
-
[HIGH] autoResumeProcessing permanently jams after clearAutoResumeQueue() (NEW-RC-001, confirmed_valid): When clearAutoResumeQueue() interrupts an in-flight processAutoResumeQueue() during a stagger delay, autoResumeProcessing stays true permanently. The comment claims "the new run owns it" but no new run is ever started. All future enqueueAutoResume() calls fail because they check !autoResumeProcessing. This was independently flagged by sentry[bot] (deadlock on project switch) and CodeRabbit (critical). Fix: reset autoResumeProcessing=false in clearAutoResumeQueue() or on generation-mismatch return paths.
-
[HIGH] Missing try/catch in processAutoResumeQueue (NEW-RC-002, confirmed_valid): resumeTerminalClaudeSession() at line 163 is not wrapped in try/catch. If it throws, autoResumeProcessing is never reset. Inconsistent with resumeAllPendingClaude which wraps the same call in try/catch. Fix: add try/catch around resumeTerminalClaudeSession and/or try/finally around the entire function.
-
[MEDIUM → needs_human_review] Stale snapshot in resumeAllPendingClaude (NEW-RC-003): Structurally inconsistent with processAutoResumeQueue but mitigated by main process idempotency guard in activateDeferredResume.
Good progress — the author has addressed all 4 previous findings and added significant improvements (concurrency guard, generation counter, shared helper, error handling in resumeAllPendingClaude). Two remaining issues need fixes before merge.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (6 selected of 6 total)
🟠 [NEW-RC-001] [HIGH] autoResumeProcessing permanently jams after clearAutoResumeQueue() interrupts processing
📁 apps/frontend/src/renderer/stores/terminal-store.ts:148
When clearAutoResumeQueue() is called while processAutoResumeQueue() is awaiting a stagger delay, autoResumeProcessing stays true permanently. clearAutoResumeQueue increments autoResumeGeneration but does NOT reset autoResumeProcessing. The stale processor detects the mismatch and returns without resetting the flag. No mechanism exists to start a new run when autoResumeProcessing is stuck true. Future enqueueAutoResume() calls never start processing because line 100 checks !autoResumeProcessing. This permanently disables auto-resume until app restart. Triggered by: project switching during auto-resume, or clicking 'Resume All' while queue is processing.
Suggested fix:
Reset autoResumeProcessing=false in clearAutoResumeQueue() after incrementing autoResumeGeneration. The generation counter already prevents stale runs from interfering, so the reset is safe. Alternative: reset on generation-mismatch return paths in processAutoResumeQueue.
🟠 [NEW-RC-002] [HIGH] Missing try/catch in processAutoResumeQueue permanently jams queue on exception
📁 apps/frontend/src/renderer/stores/terminal-store.ts:163
resumeTerminalClaudeSession() at line 163 in processAutoResumeQueue is not wrapped in try/catch. If it throws, autoResumeProcessing is never reset to false, permanently jamming the queue. This is inconsistent with resumeAllPendingClaude (lines 568-574) which wraps the same call in try/catch and uses try/finally for its guard flag.
Suggested fix:
Wrap the resumeTerminalClaudeSession call in try/catch (log error and continue), and add try/finally around the entire function body to ensure autoResumeProcessing is reset on any unexpected exception.
🔵 [NEW-RC-003] [LOW] resumeAllPendingClaude uses stale terminal snapshot across stagger delays
📁 apps/frontend/src/renderer/stores/terminal-store.ts:565
resumeAllPendingClaude captures pendingTerminals once (line 555) and iterates with 500ms stagger delays without re-checking pendingClaudeResume from the store. Unlike processAutoResumeQueue which re-checks the flag (lines 155-160), this function may issue duplicate IPC calls for terminals already resumed during the delay. Impact is mitigated by main process idempotency guard in activateDeferredResume.
Suggested fix:
Re-check terminal.pendingClaudeResume from useTerminalStore.getState() before each resumeTerminalClaudeSession call, consistent with processAutoResumeQueue.
🟠 [CMT-SENTRY-001] [HIGH] [FROM COMMENTS] sentry[bot]: Project switching deadlock permanently disables auto-resume (same as NEW-RC-001)
📁 apps/frontend/src/renderer/stores/terminal-store.ts:148
sentry[bot] flagged that switching projects during terminal auto-resume causes a deadlock because autoResumeProcessing flag is never reset. This is the same root cause as NEW-RC-001 — the generation-mismatch abort path does not reset autoResumeProcessing. Confirmed valid by finding-validator.
🟠 [CMT-CR-001] [HIGH] [FROM COMMENTS] CodeRabbit: Missing error handling in processAutoResumeQueue (same as NEW-RC-002)
📁 apps/frontend/src/renderer/stores/terminal-store.ts:163
CodeRabbit flagged that an exception in resumeTerminalClaudeSession permanently jams the queue because autoResumeProcessing is never reset. Same root cause as NEW-RC-002. Confirmed valid by finding-validator.
🔵 [CMT-CR-002] [LOW] [FROM COMMENTS] CodeRabbit: resumeAllPendingClaude stale snapshot after stagger delay (same as NEW-RC-003)
📁 apps/frontend/src/renderer/stores/terminal-store.ts:565
CodeRabbit flagged that resumeAllPendingClaude doesn't re-check terminal state after stagger delay, risking duplicate IPC calls. Same as NEW-RC-003. Mitigated by main process idempotency but structurally inconsistent with processAutoResumeQueue.
This review was generated by Auto Claude.
…ndling
Addresses all 4 HIGH severity findings from PR review:
1. [NEW-RC-001/CMT-SENTRY-001] Fixed autoResumeProcessing permanently
jamming after clearAutoResumeQueue() interrupts processing:
- Reset autoResumeProcessing=false in clearAutoResumeQueue() after
incrementing generation counter
- Safe because stale runs check generation on each iteration
- Prevents permanent queue jam when project switches during auto-resume
2. [NEW-RC-002/CMT-CR-001] Added missing error handling in
processAutoResumeQueue:
- Wrapped resumeTerminalClaudeSession() in try/catch
- Added try/finally around entire function body
- Ensures autoResumeProcessing is always reset, even on exceptions
3. [NEW-RC-003/CMT-CR-002] Fixed stale snapshot in resumeAllPendingClaude:
- Re-check terminal.pendingClaudeResume from store before each resume
- Consistent with processAutoResumeQueue guard logic
- Prevents duplicate IPC calls during stagger delays
All changes maintain backward compatibility and existing behavior while
eliminating race conditions and error propagation issues.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts`:
- Around line 548-601: resumeAllPendingClaude currently has no cancellation
token so it can keep waiting through stagger delays even after
clearAutoResumeQueue/restoreTerminalSessions runs; capture a generation counter
(or use an existing one like the one used by processAutoResumeQueue) before
calling clearAutoResumeQueue and store it in a local const gen, then inside the
loop re-check that the global generation still equals gen (or check a dedicated
isCancelled flag) before each iteration and before awaiting the stagger delay
and break early if it changed; ensure you reference and update the same
generation variable used elsewhere (or introduce a new one) and still reset
isResumingAll = false in the finally block, while keeping calls to
resumeTerminalClaudeSession and debug logs unchanged.
| resumeAllPendingClaude: async () => { | ||
| const state = get(); | ||
|
|
||
| // Filter terminals with pending Claude resume | ||
| const pendingTerminals = state.terminals.filter(t => t.pendingClaudeResume === true); | ||
|
|
||
| if (pendingTerminals.length === 0) { | ||
| debugLog('[TerminalStore] No terminals with pending Claude resume'); | ||
| // Concurrency guard - prevent multiple simultaneous executions | ||
| if (isResumingAll) { | ||
| debugLog('[TerminalStore] Resume All already in progress — skipping'); | ||
| return; | ||
| } | ||
| isResumingAll = true; | ||
|
|
||
| try { | ||
| // Clear auto-resume queue to prevent redundant processing | ||
| clearAutoResumeQueue(); | ||
|
|
||
| debugLog(`[TerminalStore] Resuming ${pendingTerminals.length} pending Claude sessions with 500ms stagger`); | ||
| const state = get(); | ||
|
|
||
| // Iterate through terminals with staggered delays | ||
| for (let i = 0; i < pendingTerminals.length; i++) { | ||
| const terminal = pendingTerminals[i]; | ||
| // Clear the pending flag BEFORE IPC call to prevent race condition | ||
| // with auto-resume effect in Terminal.tsx (which checks this flag on a 100ms timeout) | ||
| get().setPendingClaudeResume(terminal.id, false); | ||
| // Filter terminals with pending Claude resume | ||
| const pendingTerminals = state.terminals.filter(t => t.pendingClaudeResume === true); | ||
|
|
||
| debugLog(`[TerminalStore] Activating deferred Claude resume for terminal: ${terminal.id}`); | ||
| window.electronAPI.activateDeferredClaudeResume(terminal.id); | ||
| if (pendingTerminals.length === 0) { | ||
| debugLog('[TerminalStore] No terminals with pending Claude resume'); | ||
| return; | ||
| } | ||
|
|
||
| // Wait 500ms before processing next terminal (staggered delay) | ||
| if (i < pendingTerminals.length - 1) { | ||
| await new Promise(resolve => setTimeout(resolve, 500)); | ||
| debugLog(`[TerminalStore] Resuming ${pendingTerminals.length} pending Claude sessions with ${AUTO_RESUME_STAGGER_MS}ms stagger`); | ||
|
|
||
| // Iterate through terminals with staggered delays | ||
| for (let i = 0; i < pendingTerminals.length; i++) { | ||
| const terminal = pendingTerminals[i]; | ||
|
|
||
| // Re-check terminal still needs resume (may have changed during stagger delay) | ||
| const currentTerminal = get().terminals.find(t => t.id === terminal.id); | ||
| if (!currentTerminal?.pendingClaudeResume) { | ||
| debugLog(`[TerminalStore] Skipping ${terminal.id} — no longer pending`); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| debugLog(`[TerminalStore] Activating deferred Claude resume for terminal: ${terminal.id}`); | ||
| resumeTerminalClaudeSession(terminal.id); | ||
| } catch (error) { | ||
| // Log error and continue processing remaining terminals | ||
| debugError(`[TerminalStore] Error resuming terminal ${terminal.id}:`, error); | ||
| } | ||
|
|
||
| // Wait before processing next terminal (staggered delay) | ||
| if (i < pendingTerminals.length - 1) { | ||
| await new Promise(resolve => setTimeout(resolve, AUTO_RESUME_STAGGER_MS)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| debugLog('[TerminalStore] Completed resuming all pending Claude sessions'); | ||
| debugLog('[TerminalStore] Completed resuming all pending Claude sessions'); | ||
| } finally { | ||
| isResumingAll = false; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
resumeAllPendingClaude lacks a cancellation mechanism, unlike processAutoResumeQueue.
If a project switch triggers restoreTerminalSessions → clearAutoResumeQueue() while resumeAllPendingClaude is mid-stagger, this function continues iterating through its stale snapshot. The per-item re-check (line 577) prevents duplicate IPC calls, but the function still holds isResumingAll = true and burns through stagger delays for up to n × 500ms unnecessarily. During that window a second resumeAllPendingClaude call (for the new project) would be silently dropped by the guard on line 550.
Consider reusing the generation counter (or a dedicated one) to allow early exit, matching the pattern in processAutoResumeQueue.
Sketch: add generation-based cancellation
resumeAllPendingClaude: async () => {
if (isResumingAll) {
debugLog('[TerminalStore] Resume All already in progress — skipping');
return;
}
isResumingAll = true;
+ const gen = autoResumeGeneration; // capture before clearAutoResumeQueue bumps it
try {
clearAutoResumeQueue();
// ...
for (let i = 0; i < pendingTerminals.length; i++) {
+ if (gen !== autoResumeGeneration) {
+ debugLog('[TerminalStore] Generation mismatch in resumeAll — aborting');
+ return;
+ }
// ... existing re-check + resume + stagger ...
}
} finally {
isResumingAll = false;
}
},Note: capture gen before clearAutoResumeQueue() so the clear inside the same call doesn't immediately invalidate itself, or use a separate counter for resumeAllPendingClaude.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/stores/terminal-store.ts` around lines 548 - 601,
resumeAllPendingClaude currently has no cancellation token so it can keep
waiting through stagger delays even after
clearAutoResumeQueue/restoreTerminalSessions runs; capture a generation counter
(or use an existing one like the one used by processAutoResumeQueue) before
calling clearAutoResumeQueue and store it in a local const gen, then inside the
loop re-check that the global generation still equals gen (or check a dedicated
isCancelled flag) before each iteration and before awaiting the stagger delay
and break early if it changed; ensure you reference and update the same
generation variable used elsewhere (or introduce a new one) and still reset
isResumingAll = false in the finally block, while keeping calls to
resumeTerminalClaudeSession and debug logs unchanged.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ## 🟡 Follow-up Review: Merge With Changes
🟡 Can merge - Minor suggestions noted, no blockers.
Resolution Status
- ✅ Resolved: 6 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 2 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
All 20 CI checks passing. All 6 previous findings (4 HIGH, 2 LOW) are fully resolved — commit e36163f comprehensively addresses the three root causes: (1) autoResumeProcessing deadlock via clearAutoResumeQueue reset + try/finally, (2) missing error handling via inner try/catch + outer try/finally, (3) stale terminal snapshot via per-iteration store re-check. Two new findings dismissed as false positives after validation (array replacement pattern is doubly protected; HMR limitation is already documented). Two LOW severity findings remain as optional improvements: resumeAllPendingClaude lacks generation-based cancellation for consistency (mitigated by per-iteration re-check), and undocumented mutex invariant between resume paths. Both are polish items that can be addressed post-merge. The sentry[bot] deadlock bug and all CodeRabbit critical/major/minor concerns are resolved.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
💡 Suggestions (2)
These are non-blocking suggestions for consideration:
🔵 [NEW-001] [LOW] resumeAllPendingClaude lacks generation-based cancellation unlike processAutoResumeQueue
📁 apps/frontend/src/renderer/stores/terminal-store.ts:548
processAutoResumeQueue uses a generation counter for external cancellation via clearAutoResumeQueue(). resumeAllPendingClaude has no equivalent — once started, it iterates with 500ms stagger delays and cannot be cancelled. The isResumingAll guard blocks concurrent calls. However, the per-iteration pendingClaudeResume re-check prevents functional issues: no double-resumes or incorrect resumes. This is a code consistency improvement for a future iteration.
Suggested fix:
Add a generation counter check in the stagger loop, similar to processAutoResumeQueue. Capture autoResumeGeneration at start, check after each stagger await, return early on mismatch.
🔵 [NEW-004] [LOW] Implicit mutex invariant between active-terminal effect and queue processor is undocumented
📁 apps/frontend/src/renderer/components/Terminal.tsx:586
The mutual exclusion between the active-terminal auto-resume effect (Terminal.tsx) and the background queue processor (processAutoResumeQueue) relies on pendingClaudeResume as a coordination mutex. Both paths check and clear the flag synchronously via Zustand. The invariant is sound but undocumented — a brief comment would improve maintainability.
Suggested fix:
Add a comment at the pendingClaudeResume re-check explaining that it serves as the coordination mutex between the active-terminal path and the background queue processor.
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
Addresses CodeRabbit finding (NITPICK/TRIVIAL severity): [CMT-CR-NITPICK] resumeAllPendingClaude lacked a cancellation mechanism: - Captured generation counter before clearAutoResumeQueue() call - Added generation mismatch checks before each iteration - Added generation check after stagger delay await points - Ensures early exit when project switches during processing - Prevents unnecessary stagger delays for stale resume operations - Consistent with processAutoResumeQueue cancellation pattern This prevents the function from burning through stagger delays (up to n×500ms) after a project switch, and ensures responsive handling of project changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| // doesn't immediately invalidate itself, but CAN be cancelled by external calls | ||
| const generation = autoResumeGeneration; | ||
|
|
||
| debugLog(`[TerminalStore] Resuming ${pendingTerminals.length} pending Claude sessions with 500ms stagger`); | ||
| try { | ||
| // Clear auto-resume queue to prevent redundant processing | ||
| clearAutoResumeQueue(); |
There was a problem hiding this comment.
Bug: The resumeAllPendingClaude function captures autoResumeGeneration before it's incremented, causing the function to exit immediately without resuming any terminals.
Severity: HIGH
Suggested Fix
Move the line const generation = autoResumeGeneration; to after the clearAutoResumeQueue() call. This ensures the captured generation value is the new, post-increment value, preventing the function from immediately invalidating its own operation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/frontend/src/renderer/stores/terminal-store.ts#L557-L562
Potential issue: The `resumeAllPendingClaude` function, triggered by the user-facing
"Resume All" button, contains a logic error that prevents it from working. It captures
the value of `autoResumeGeneration` before calling `clearAutoResumeQueue()`, which
increments the same counter. Consequently, a subsequent check `if (generation !==
autoResumeGeneration)` is immediately true, causing the function to abort before any
terminals are resumed. This results in the "Resume All" feature failing silently without
performing its intended action.
1 participant
Summary
terminal-store.tsthat processes non-active terminals with staggered delays (1.5s initial wait, 500ms between each resume)onCreatedcallback inTerminal.tsx)isActiveuseEffectProblem
When the app restarts, only the first terminal (the one assigned
activeTerminalId) auto-resumes its Claude session. All other terminals show a pending resume badge and require the user to manually click the tab and press Enter. Root cause: the deferred resumeuseEffectinTerminal.tsxgates onisActive === true.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit