fix(terminal): enhance terminal recreation logic with retry mechanism

[AndyMik90]

Summary

• Adds a retry mechanism (up to 10 attempts with 100ms delays) for terminal recreation when dimensions aren't ready
• Prevents terminals from getting stuck in a failed state during worktree close/recreation scenarios
• Properly cleans up retry timers on component unmount to prevent memory leaks

Changes

• Introduced MAX_RECREATION_RETRIES and RECREATION_RETRY_DELAY constants
• Added recreationRetryCountRef and recreationRetryTimerRef refs to track retry state
• Enhanced error handling paths to retry during recreation instead of immediately failing
• Added cleanup effect for retry timer on unmount

Test plan

• Close a worktree while terminal is open - terminal should recreate successfully
• Verify no memory leaks from orphaned timers when navigating away
• Confirm error is reported after max retries are exhausted

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved terminal process recreation reliability with an automatic retry mechanism that handles cases when dimensions aren't ready.
  • Enhanced error handling and reporting for terminal operations to provide better feedback on failures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@AndyMik90 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 64f7d14 and a372944.

📒 Files selected for processing (1)

• apps/frontend/src/renderer/components/terminal/usePtyProcess.ts

📝 Walkthrough

Walkthrough

The change introduces a retry mechanism for PTY process recreation when terminal dimensions aren't immediately available, expands the UsePtyProcessOptions interface with new configuration fields, and enhances lifecycle handling with proper cleanup and error reporting through callbacks.

Changes

Cohort / File(s)

Summary

PTY Process Retry Mechanism apps/frontend/src/renderer/components/terminal/usePtyProcess.ts

Added retry logic for PTY recreation with MAX_RECREATION_RETRIES and RECREATION_RETRY_DELAY constants. Introduced recreationRetryCountRef and recreationRetryTimerRef to track and schedule retries. Enhanced UsePtyProcessOptions interface with projectPath, cols, rows, skipCreation, isRecreatingRef, onCreated, and onError fields. Integrated retry scheduling into error handling branches with proper cleanup on unmount and state reset on successful recreation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

bug, area/frontend, 🔄 Checking, size/M

Suggested reviewers

• MikeeBuilds
• AlexMadera

Poem

🐰 A hop through dimension delays so keen,
Retry logic, the finest we've seen!
With timers and flags in perfect array,
PTY processes won't fade away!
Terminal dreams now persist and bloom,
No more vanishing into the gloom. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: adding a retry mechanism to enhance terminal recreation logic, which is the core focus of the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Summary of Changes

Hello @AndyMik90, I'm Gemini Code Assist¹! 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 significantly improves the resilience of the terminal component by introducing a robust retry mechanism for its recreation process. This change addresses scenarios where terminals might fail to initialize correctly due to transient issues like dimensions not being immediately available, ensuring a more stable user experience and preventing terminals from becoming unresponsive. Additionally, it includes crucial cleanup logic to avoid resource leaks.

Highlights

• Enhanced Terminal Recreation: Implemented a retry mechanism for terminal recreation, allowing up to 10 attempts with 100ms delays when terminal dimensions are not immediately available.
• Improved Stability: Enhanced terminal stability by preventing it from getting stuck in a failed state during worktree close/recreation scenarios.
• Resource Management: Ensured proper cleanup of retry timers on component unmount to prevent potential memory leaks.
• Configurable Retry Behavior: Introduced MAX_RECREATION_RETRIES and RECREATION_RETRY_DELAY constants for configurable retry behavior.
• State Management: Utilized recreationRetryCountRef and recreationRetryTimerRef to manage the state and timing of recreation retries.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a retry mechanism for terminal recreation, which is a great enhancement to improve robustness. The implementation correctly uses setTimeout for delays and useEffect for cleanup. My main feedback is regarding significant code duplication of the retry logic, which appears in five different places. I've suggested refactoring this into a single helper function to improve maintainability and readability. Overall, the changes are good and address the problem described.

[gemini-code-assist]

The logic for retrying terminal recreation is duplicated across multiple failure paths in this useEffect hook. This makes the code harder to read and maintain, as any future changes to the retry mechanism would need to be applied in 5 different places.

The duplicated blocks are:

• Lines 81-96: When dimensions are not ready.
• Lines 151-164: On session restore failure.
• Lines 168-181: On session restore promise rejection.
• Lines 205-218: On new terminal creation failure.
• Lines 222-235: On new terminal creation promise rejection.

To improve this, you should extract the retry logic into a single helper function defined within the useEffect scope. This will centralize the logic, improve readability, and ensure consistency.

Here's an example of what that helper function could look like:

const scheduleRetryOrReportError = (error: string) => {
  if (isRecreatingRef?.current && recreationRetryCountRef.current < MAX_RECREATION_RETRIES) {
    recreationRetryCountRef.current += 1;
    recreationRetryTimerRef.current = setTimeout(() => {
      setRecreationTrigger((prev) => prev + 1);
    }, RECREATION_RETRY_DELAY);
  } else {
    // Not recreating or max retries exceeded - clear and report error
    if (isRecreatingRef?.current) {
      console.error(`[usePtyProcess] Recreation failed after max retries: ${error}`);
      isRecreatingRef.current = false;
    }
    recreationRetryCountRef.current = 0;
    onError?.(error);
  }
};

You could then replace all duplicated blocks with a single call to this function, for example:

scheduleRetryOrReportError('Terminal recreation failed: dimensions not ready');

[coderabbitai]

Actionable comments posted: 2

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/components/terminal/usePtyProcess.ts (1)

240-240: Add useCallback memoization for onCreated and onError callbacks.

The onCreated and onError callbacks passed to usePtyProcess are currently defined as inline arrow functions in Terminal.tsx, causing the effect to re-run on every render. Wrap these callbacks with useCallback to stabilize their identity across renders:

const onCreated = useCallback(() => {
  setStatus('success');
}, []);

const onError = useCallback((error) => {
  dispatch(someAction);
}, [/* dependencies */]);

🤖 Fix all issues with AI agents

In @apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:
- Around line 80-97: The user-facing error passed to onError is a hardcoded
string and you also may stack timers; call useTranslation() to get t and replace
the hardcoded message with t('terminal.recreationFailed.dimensionsNotReady') (or
your project’s equivalent key) when calling onError and when logging; before
scheduling a new timeout, clear any existing timer by checking and
clearTimeout(recreationRetryTimerRef.current) and then assign
recreationRetryTimerRef.current = setTimeout(...); keep existing logic around
skipCreation, isRecreatingRef, recreationRetryCountRef, MAX_RECREATION_RETRIES,
setRecreationTrigger and RECREATION_RETRY_DELAY.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fefc17a and 64f7d14.

📒 Files selected for processing (1)

• apps/frontend/src/renderer/components/terminal/usePtyProcess.ts

🧰 Additional context used

📓 Path-based instructions (2)

apps/frontend/src/**/*.{tsx,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.

Files:

• apps/frontend/src/renderer/components/terminal/usePtyProcess.ts

apps/frontend/**/*.{ts,tsx}

⚙️ CodeRabbit configuration file

apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.

Files:

• apps/frontend/src/renderer/components/terminal/usePtyProcess.ts

🧬 Code graph analysis (1)

apps/frontend/src/renderer/components/terminal/usePtyProcess.ts (2)

apps/frontend/src/renderer/stores/terminal-store.ts (1)

• useTerminalStore (69-323)

apps/frontend/src/renderer/components/settings/utils/hookProxyFactory.ts (1)

• error (18-18)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Seer Code Review
• GitHub Check: Cursor Bugbot
• GitHub Check: CodeQL (python)
• GitHub Check: CodeQL (javascript-typescript)

🔇 Additional comments (5)

apps/frontend/src/renderer/components/terminal/usePtyProcess.ts (5)

4-21: LGTM!

The retry constants are reasonable (up to 1 second total wait time), and the interface expansion with optional fields is well-typed.

---

50-58: LGTM!

Proper cleanup effect to prevent memory leaks from orphaned timers on component unmount.

---

204-237: Duplicated retry logic - same pattern as restored session handling.

This is the same retry pattern used in lines 149-183. The suggested helper function refactor would consolidate all these branches.

---

244-256: LGTM!

The prepareForRecreate and resetForRecreate functions properly coordinate the recreation lifecycle by controlling ref state and triggering the effect via state update.

---

258-262: LGTM!

Return structure is appropriate. Note that isCreated returns a snapshot of the ref value - consumers should rely on the onCreated callback for reactive updates.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use i18n translation key for user-facing error message.

The error message on line 94 is a hardcoded string. As per coding guidelines, all user-facing text should use translation keys via useTranslation().

Additionally, consider clearing any existing retry timer before scheduling a new one to prevent potential timer stacking if the effect fires rapidly:

Suggested fix

       if (recreationRetryCountRef.current < MAX_RECREATION_RETRIES) {
         recreationRetryCountRef.current += 1;
         // Schedule a retry by incrementing the trigger after a delay
+        if (recreationRetryTimerRef.current) {
+          clearTimeout(recreationRetryTimerRef.current);
+        }
         recreationRetryTimerRef.current = setTimeout(() => {
           setRecreationTrigger((prev) => prev + 1);
         }, RECREATION_RETRY_DELAY);
       } else {
         // Max retries exceeded - clear recreation state and report error
         console.error('[usePtyProcess] Recreation failed: dimensions not ready after max retries');
         isRecreatingRef.current = false;
         recreationRetryCountRef.current = 0;
-        onError?.('Terminal recreation failed: dimensions not ready');
+        onError?.(t('terminal:errors.recreationFailed'));  // Use i18n key
       }

🤖 Prompt for AI Agents

In @apps/frontend/src/renderer/components/terminal/usePtyProcess.ts around lines
80 - 97, The user-facing error passed to onError is a hardcoded string and you
also may stack timers; call useTranslation() to get t and replace the hardcoded
message with t('terminal.recreationFailed.dimensionsNotReady') (or your
project’s equivalent key) when calling onError and when logging; before
scheduling a new timeout, clear any existing timer by checking and
clearTimeout(recreationRetryTimerRef.current) and then assign
recreationRetryTimerRef.current = setTimeout(...); keep existing logic around
skipCreation, isRecreatingRef, recreationRetryCountRef, MAX_RECREATION_RETRIES,
setRecreationTrigger and RECREATION_RETRY_DELAY.

[AndyMik90]

🤖 Auto Claude PR Review

Merge Verdict: 🟠 NEEDS REVISION

🟠 Needs revision - 3 issue(s) require attention.

3 issue(s) must be addressed (0 required, 3 recommended), 3 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: 3 issue(s)
• Low: 3 issue(s)

---

Generated by Auto Claude PR Review

Findings (6 selected of 6 total)

🟡 [566476bba46a] [MEDIUM] Race condition: pending retry timer not cleared on effect re-run

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:86

When setting a retry timer (lines 86, 154, 171, 208, 225), the code overwrites recreationRetryTimerRef.current without first clearing any existing timer. If the effect re-runs due to dependency changes (cols/rows changing) before the timer fires, the old timer remains active. This can cause multiple timers to fire, triggering extra state updates. The cleanup function (lines 51-57) only runs on UNMOUNT, not on effect re-runs.

Suggested fix:

Add timer cleanup at the START of the effect, not just in the unmount cleanup. Add this at line 80:

if (recreationRetryTimerRef.current) {
  clearTimeout(recreationRetryTimerRef.current);
  recreationRetryTimerRef.current = null;
}

🟡 [3648e6e68c76] [MEDIUM] isCreatingRef cleared during retry window allows duplicate creation attempts

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:166

At line 166 (and similarly at 182, 220, 236), isCreatingRef.current is set to false unconditionally after the promise resolves/rejects, even when a retry timer has been scheduled. This means during the 100ms RECREATION_RETRY_DELAY window, isCreatingRef is false. If the effect runs again during this window (from other dependency changes), the guard at line 101 won't prevent a duplicate creation attempt.

Suggested fix:

Only set isCreatingRef.current = false when NOT scheduling a retry. Move the assignment inside the else block, or add a condition:

if (!isRecreatingRef?.current || recreationRetryCountRef.current >= MAX_RECREATION_RETRIES) {
  isCreatingRef.current = false;
}

🟡 [1b2c0ed142d3] [MEDIUM] Retry logic pattern duplicated 5 times - extract to helper function

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:79

The same retry pattern is copy-pasted across 5 locations: (1) lines 79-97 for skipCreation, (2) lines 152-165 for restore session error, (3) lines 168-182 for restore session catch, (4) lines 206-218 for create terminal error, (5) lines 223-236 for create terminal catch. Each instance contains identical logic: check if recreating and under max retries, increment counter, schedule timer, or clear state and report error. This creates maintenance burden.

Suggested fix:

Extract a reusable helper:

const scheduleRetryOrFail = useCallback((error: string): boolean => {
  if (isRecreatingRef?.current && recreationRetryCountRef.current < MAX_RECREATION_RETRIES) {
    recreationRetryCountRef.current += 1;
    recreationRetryTimerRef.current = setTimeout(() => setRecreationTrigger(p => p + 1), RECREATION_RETRY_DELAY);
    return true;
  }
  if (isRecreatingRef?.current) isRecreatingRef.current = false;
  recreationRetryCountRef.current = 0;
  onError?.(error);
  return false;
}, [isRecreatingRef, onError]);

🔵 [05eebd0d99fa] [LOW] Consider using i18n translation key for error message

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:94

Per CLAUDE.md guidelines, user-facing text should use translation keys. The error message 'Terminal recreation failed: dimensions not ready' is hardcoded. Note: All other error messages in this file are also hardcoded, so the PR author is following existing file conventions. This is a suggestion for future improvement.

Suggested fix:

Add translation key to errors.json and use t('errors:terminal.recreationFailed'). This could be addressed in a separate PR that updates all error messages in this file consistently.

🔵 [2a04c7a6365a] [LOW] Main useEffect has high cyclomatic complexity (161 lines)

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:79

The main useEffect spans 161 lines with deeply nested conditional logic: skipCreation handling, restoration vs new terminal creation, success vs error handling, and retry logic in multiple branches. This makes the code difficult to test and maintain. Related to the duplication issue - extracting helper functions would address both.

Suggested fix:

Refactor by extracting helper functions: handleSkipCreationRetry(), restoreTerminalSession(), createNewTerminal(), and a shared handleResult() callback.

🔵 [e1b5a7532e8a] [LOW] AI Triage: Cursor's 'retry counter reset' claim is FALSE POSITIVE

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:104

Cursor AI claimed line 104 resets the retry counter every time the effect runs, preventing max retries. This is INCORRECT. Lines 81-96 handle retries and return EARLY before reaching line 104. Line 104 only runs when creation PROCEEDS (dimensions are ready). The retry counter is properly incremented during retry paths.

Suggested fix:

No fix needed - the existing logic is correct.

---

This review was generated by Auto Claude.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ AndyMik90
❌ Test User

---

Test User seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[cursor]

Retry after API failure blocked by stale guard

High Severity

When an API call fails during recreation, scheduleRetryOrFail schedules a retry timer but leaves isCreatingRef.current as true. When the timer fires and the effect re-runs, the guard at line 123 (if (isCreatingRef.current || isCreatedRef.current) return) immediately exits, preventing the retry from executing. The skipCreation retry path works correctly because isCreatingRef.current is never set before the early return, but retries after actual API failures during recreation will silently fail to retry.

Additional Locations (1)

• apps/frontend/src/renderer/components/terminal/usePtyProcess.ts#L150-L157

 

[cursor]

Retry counter not reset when starting fresh recreation

Low Severity

The resetForRecreate function resets isCreatedRef and isCreatingRef but does not reset recreationRetryCountRef. If a new recreation is triggered while a previous retry cycle is in progress (e.g., rapid worktree switches while dimensions aren't ready), the counter carries over. This means the new recreation inherits the old retry count and may exhaust its retry budget prematurely, potentially causing terminal recreation to fail when it would have succeeded with the full 10 retries.

 

[AndyMik90]

🤖 Auto Claude PR Review

✅ Follow-up Review: Ready To Merge

✅ Ready to merge - All checks passing and findings addressed.

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: 0 findings verified as genuine issues
• 👤 Needs Human Review: 1 findings require manual verification

Verdict

All 3 MEDIUM severity findings from the previous review have been properly addressed through the refactored retry mechanism. The 2 findings initially marked unresolved (i18n, complexity) were validated as FALSE POSITIVES - terminal output is not subject to i18n, and complexity is acceptable at 104 lines. The new medium finding (NEW-001) was validated as NOT an active bug - Terminal.tsx always calls prepareForRecreate() first. Only 2 LOW severity quality suggestions remain. The 'Require Re-review on Push' CI check is procedural and will be satisfied by this review approval.

Review Process

Agents invoked: resolution-verifier, new-code-reviewer, finding-validator

---

This is an AI-generated follow-up review using parallel specialist analysis with finding validation.

⚠️ CI Status: 1 check(s) failing: Require Re-review on Push

Findings (2 selected of 2 total)

🔵 [NEW-002] [LOW] Hook mutates ref passed from caller

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:75

scheduleRetryOrFail mutates isRecreatingRef.current from parent. Creates hidden coupling.

🔵 [NEW-003] [LOW] Ref in useCallback deps is no-op

📁 apps/frontend/src/renderer/components/terminal/usePtyProcess.ts:82

isRecreatingRef in dependency array is misleading - refs are stable.

---

This review was generated by Auto Claude.