-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(terminal): centralized auto-resume queue for terminal recovery #1835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
1a11eb0
f53e2e0
5461377
b2cda4f
e332416
e36163f
8dc2a60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -76,6 +76,80 @@ export function writeToTerminal(terminalId: string, data: string): void { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // === Auto-Resume Queue Coordinator === | ||||||
| // Coordinates staggered auto-resume of non-active terminals after app restart. | ||||||
| // Each terminal enqueues itself when its PTY is confirmed ready (onCreated). | ||||||
| // A single coordinator processes the queue with stagger delays. | ||||||
|
|
||||||
| const AUTO_RESUME_INITIAL_DELAY_MS = 1500; | ||||||
| const AUTO_RESUME_STAGGER_MS = 500; | ||||||
|
|
||||||
| let autoResumeQueue: string[] = []; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For managing a collection of unique terminal IDs, using a While the performance difference is negligible with a maximum of 12 terminals, using a To implement this, you would change this line and update the related functions:
Suggested change
|
||||||
| let autoResumeTimer: ReturnType<typeof setTimeout> | null = null; | ||||||
| let autoResumeProcessing = false; | ||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| export function enqueueAutoResume(terminalId: string): void { | ||||||
| if (autoResumeQueue.includes(terminalId)) return; | ||||||
| autoResumeQueue.push(terminalId); | ||||||
| debugLog(`[AutoResume] Enqueued terminal: ${terminalId}, queue size: ${autoResumeQueue.length}`); | ||||||
|
|
||||||
| // Start initial delay timer on first enqueue only | ||||||
| if (autoResumeTimer === null && !autoResumeProcessing) { | ||||||
| debugLog(`[AutoResume] Starting initial delay (${AUTO_RESUME_INITIAL_DELAY_MS}ms)`); | ||||||
| autoResumeTimer = setTimeout(() => { | ||||||
| autoResumeTimer = null; | ||||||
| processAutoResumeQueue(); | ||||||
| }, AUTO_RESUME_INITIAL_DELAY_MS); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export function dequeueAutoResume(terminalId: string): void { | ||||||
| const idx = autoResumeQueue.indexOf(terminalId); | ||||||
| if (idx !== -1) { | ||||||
| autoResumeQueue.splice(idx, 1); | ||||||
| debugLog(`[AutoResume] Dequeued terminal: ${terminalId}, queue size: ${autoResumeQueue.length}`); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| export function clearAutoResumeQueue(): void { | ||||||
| autoResumeQueue = []; | ||||||
| if (autoResumeTimer !== null) { | ||||||
| clearTimeout(autoResumeTimer); | ||||||
| autoResumeTimer = null; | ||||||
| } | ||||||
| autoResumeProcessing = false; | ||||||
| debugLog('[AutoResume] Queue cleared'); | ||||||
|
Comment on lines
118
to
128
This comment was marked as outdated.
Sorry, something went wrong. |
||||||
| } | ||||||
|
|
||||||
| async function processAutoResumeQueue(): Promise<void> { | ||||||
| if (autoResumeProcessing) return; | ||||||
| autoResumeProcessing = true; | ||||||
| debugLog(`[AutoResume] Processing queue, ${autoResumeQueue.length} terminals`); | ||||||
|
|
||||||
| while (autoResumeQueue.length > 0) { | ||||||
| const terminalId = autoResumeQueue.shift()!; | ||||||
|
|
||||||
| // Check if terminal still needs resume | ||||||
| const terminal = useTerminalStore.getState().terminals.find(t => t.id === terminalId); | ||||||
| if (!terminal?.pendingClaudeResume) { | ||||||
| debugLog(`[AutoResume] Skipping ${terminalId} — no longer pending`); | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| debugLog(`[AutoResume] Resuming terminal: ${terminalId}`); | ||||||
| useTerminalStore.getState().setPendingClaudeResume(terminalId, false); | ||||||
| window.electronAPI.activateDeferredClaudeResume(terminalId); | ||||||
|
|
||||||
| // Stagger delay between resumes | ||||||
| if (autoResumeQueue.length > 0) { | ||||||
| await new Promise(resolve => setTimeout(resolve, AUTO_RESUME_STAGGER_MS)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| autoResumeProcessing = false; | ||||||
| debugLog('[AutoResume] Queue processing complete'); | ||||||
| } | ||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| export type TerminalStatus = 'idle' | 'running' | 'claude-active' | 'exited'; | ||||||
|
|
||||||
| export interface Terminal { | ||||||
|
|
@@ -507,6 +581,7 @@ export async function restoreTerminalSessions(projectPath: string): Promise<void | |||||
| return; | ||||||
| } | ||||||
| restoringProjects.add(projectPath); | ||||||
| clearAutoResumeQueue(); | ||||||
|
|
||||||
| try { | ||||||
| const store = useTerminalStore.getState(); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a potential bug here. Setting
hasAttemptedAutoResumeRef.current = truewhen a non-active terminal is enqueued prevents it from being resumed immediately if it becomes active before the queue processor handles it. TheuseEffectthat handles active terminal resumes will seehasAttemptedAutoResumeRef.currentastrueand skip the resume logic, leading to a poor user experience.The
hasAttemptedAutoResumeRefshould only be set when a resume is actually triggered, not when it's just enqueued. TheenqueueAutoResumefunction already prevents duplicate entries in the queue, and theonCreatedcallback only runs once per PTY creation, so there's no risk of multiple enqueues from here.I suggest removing the check for
hasAttemptedAutoResumeRefand the line that sets it.