fix: prevent infinite PTY recreation loop on terminal natural exit#2015
Open
luiggibcn wants to merge 8 commits into
Open
fix: prevent infinite PTY recreation loop on terminal natural exit#2015luiggibcn wants to merge 8 commits into
luiggibcn wants to merge 8 commits into
Conversation
When the shell exited naturally (e.g. typing `exit`), the terminal would enter an infinite mount/unmount cycle. TerminalGrid's pendingCleanup grace period (150ms) kept remounting the Terminal component while its status was still 'exited', causing usePtyProcess to spin up a new PTY on each mount before the previous one could complete Guard: skip PTY creation when status === 'exited' and isRecreatingRef is not set, which is the deliberate-recreation path (worktree switch) Adds usePtyProcess.test.ts with 6 cases covering the guard, the recreation exception, and the normal creation path
Contributor
|
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 Vitest suite for Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component / usePtyProcess
participant Store as Terminal Store
participant API as window.electronAPI
Component->>Store: read terminal by id/status
alt terminal missing OR status == "exited" AND not isRecreatingRef
Component->>Component: debug log & return (no create)
else
Component->>API: createTerminal(options)
API-->>Component: created / events
Component->>Store: updateTerminal / setTerminalStatus('running')
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a guard in the usePtyProcess hook to prevent infinite PTY recreation loops when a terminal exits naturally, and adds corresponding unit tests and documentation. Feedback suggests making the guard more robust by checking for the existence of the terminal state to avoid spawning orphaned processes.
Comment on lines
+162
to
+165
| if (terminalState?.status === 'exited' && !isRecreatingRef?.current) { | ||
| debugLog(`[usePtyProcess] Skipping PTY creation for terminal: ${terminalId} - terminal has exited naturally (status=exited)`); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
This guard is a great addition to prevent the infinite loop. However, it could be made more robust by also checking if terminalState exists. If the terminal has been removed from the store, we should skip PTY creation to avoid spawning orphaned processes.
Suggested change
| if (terminalState?.status === 'exited' && !isRecreatingRef?.current) { | |
| debugLog(`[usePtyProcess] Skipping PTY creation for terminal: ${terminalId} - terminal has exited naturally (status=exited)`); | |
| return; | |
| } | |
| if (!terminalState || (terminalState.status === 'exited' && !isRecreatingRef?.current)) { | |
| debugLog(`[usePtyProcess] Skipping PTY creation for terminal: ${terminalId} - ${!terminalState ? 'terminal not found' : 'terminal has exited naturally'}`); | |
| return; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/desktop/src/renderer/components/terminal/usePtyProcess.ts (1)
171-175: 🧹 Nitpick | 🔵 TrivialMinor: redundant status check now that the guard exists.
After the new guard at L162, this block is only reachable when
isRecreatingRef?.currentis true, so the&& terminalState?.status === 'exited'half is the only thing the condition still discriminates on. That's still correct, but the comment ("when recreating … reset status from 'exited' to 'idle'") and condition can be tightened or left as-is. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminal/usePtyProcess.ts` around lines 171 - 175, The condition in usePtyProcess that checks both isRecreatingRef?.current and terminalState?.status === 'exited' is redundant because the earlier guard ensures this block only runs when isRecreatingRef is true; remove the terminalState?.status === 'exited' part so the if only checks isRecreatingRef?.current, update the comment to "When recreating, reset terminal status to 'idle' to allow proper recreation after deliberate terminal destruction", and call store.setTerminalStatus(terminalId, 'idle') inside that block (referencing isRecreatingRef, terminalId, store.setTerminalStatus, and terminalState for context).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts`:
- Around line 109-130: The isRecreatingRef === true test is missing an assertion
that the terminal status is reset; update that test (the one asserting
recreation behavior) to also assert that mockSetTerminalStatus was called with
('term-1', 'idle') to lock in the worktree-switch contract (this corresponds to
the status reset performed in usePtyProcess.ts around the isRecreatingRef flow).
- Around line 31-32: The mockTerminalStatus variable is too loosely typed;
import and use the actual TerminalStatus union type from terminal-store.ts and
declare mockTerminalStatus: TerminalStatus = 'idle' so invalid status strings
(e.g., 'exitted') fail at compile time; leave mockIsRestored as boolean. Update
the test file to reference the TerminalStatus type and set the initial value
accordingly.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/terminal/usePtyProcess.ts`:
- Around line 171-175: The condition in usePtyProcess that checks both
isRecreatingRef?.current and terminalState?.status === 'exited' is redundant
because the earlier guard ensures this block only runs when isRecreatingRef is
true; remove the terminalState?.status === 'exited' part so the if only checks
isRecreatingRef?.current, update the comment to "When recreating, reset terminal
status to 'idle' to allow proper recreation after deliberate terminal
destruction", and call store.setTerminalStatus(terminalId, 'idle') inside that
block (referencing isRecreatingRef, terminalId, store.setTerminalStatus, and
terminalState for context).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e770a4b8-8207-44ba-995e-8e98642dbb5d
📒 Files selected for processing (2)
apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.tsapps/desktop/src/renderer/components/terminal/usePtyProcess.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts`:
- Around line 23-29: The test currently stubs the entire global window via
vi.stubGlobal('window', { electronAPI: ... }), which clobbers jsdom's window and
can break DOM/RTL behavior; instead, restore the real jsdom window and attach
just the mocked API surface: set or define window.electronAPI to an object
containing mockCreateTerminal, mockDestroyTerminal, and
mockRestoreTerminalSession before calling renderHook/usePtyProcess (or use
vi.stubGlobal on 'window.electronAPI' only), ensuring you reference the existing
global used by `@testing-library/react` rather than replacing window entirely.
- Around line 84-95: Add an assertion to the test "does not call createTerminal
on repeated mounts when status remains exited" that also verifies
destroyTerminal is never invoked; after the mount/unmount loop (where
mockTerminalStatus = 'exited' and usePtyProcess is rendered/unmounted
repeatedly) add expect(mockDestroyTerminal).not.toHaveBeenCalled() to ensure
neither createTerminal nor destroyTerminal are triggered and reference the mock
symbols mockCreateTerminal, mockDestroyTerminal and the hook usePtyProcess to
locate where to add the assertion.
- Around line 38-54: The test mock for useTerminalStore includes a
getTerminal/mocKGetTerminal that usePtyProcess doesn't call; remove the dead
plumbing by deleting the mockGetTerminal declaration and removing the
getTerminal property from the mocked useTerminalStore object so the test relies
only on store.terminals (the hook uses store.terminals.find((t) => t.id ===
terminalId)); ensure no other test references to mockGetTerminal remain.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3a4738a4-aa9d-4b4a-8342-289643528156
📒 Files selected for processing (2)
apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.tsapps/desktop/src/renderer/components/terminal/usePtyProcess.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts (2)
36-36: 🧹 Nitpick | 🔵 TrivialDrop the unused
getTerminalmock plumbing.
usePtyProcessreads terminal state viastore.terminals.find(...), notgetTerminal.mockGetTerminaland thegetTerminalfield on the mockedgetState()are never exercised; removing them keeps the test surface minimal and avoids implying a contract the hook does not depend on.♻️ Suggested change
const mockSetTerminalStatus = vi.fn(); -const mockGetTerminal = vi.fn(); vi.mock('../../../stores/terminal-store', () => ({ useTerminalStore: Object.assign(vi.fn(), { getState: () => ({ terminals: [ { id: 'term-1', status: mockTerminalStatus, isRestored: mockIsRestored, isCLIMode: false, cwd: '/test', }, ], setTerminalStatus: mockSetTerminalStatus, - getTerminal: mockGetTerminal, }), }), }));Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts` at line 36, Remove the unused mock plumbing for getTerminal: delete the declaration of mockGetTerminal and the getTerminal property on the mocked getState() in the usePtyProcess.test.ts file since usePtyProcess reads terminal state via store.terminals.find(...) and never calls getTerminal; update any related mock setup that references mockGetTerminal so the test only mocks the state shape actually consumed by usePtyProcess.
23-29: 🧹 Nitpick | 🔵 TrivialStubbing the entire
windowstill clobbers jsdom — only theelectronAPIsurface should be attached.
vi.stubGlobal('window', { electronAPI: ... })replaces jsdom'swindow, droppingdocument,location,navigator, etc. It works today only becauseusePtyProcessreacheswindow.electronAPIwhile RTL'srenderHookresolvesdocumentvia the global, but any future test (or RTL internal change) that touches DOM APIs throughwindowwill silently break. Attach the mock to the real jsdom window instead.♻️ Suggested change
-vi.stubGlobal('window', { - electronAPI: { - createTerminal: mockCreateTerminal, - destroyTerminal: mockDestroyTerminal, - restoreTerminalSession: mockRestoreTerminalSession, - }, -}); +Object.defineProperty(window, 'electronAPI', { + configurable: true, + writable: true, + value: { + createTerminal: mockCreateTerminal, + destroyTerminal: mockDestroyTerminal, + restoreTerminalSession: mockRestoreTerminalSession, + }, +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts` around lines 23 - 29, The test currently replaces the entire jsdom window via vi.stubGlobal('window', {...}), which clobbers DOM globals; instead attach the mock surface onto the real jsdom window by defining window.electronAPI to your mocks (createTerminal: mockCreateTerminal, destroyTerminal: mockDestroyTerminal, restoreTerminalSession: mockRestoreTerminalSession) before invoking usePtyProcess — e.g. use Object.defineProperty(globalThis.window, 'electronAPI', { value: { createTerminal: mockCreateTerminal, destroyTerminal: mockDestroyTerminal, restoreTerminalSession: mockRestoreTerminalSession }, configurable: true }) or an equivalent vi.stubGlobal('electronAPI', ...) approach so usePtyProcess reads window.electronAPI without replacing document/location/navigator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts`:
- Line 36: Remove the unused mock plumbing for getTerminal: delete the
declaration of mockGetTerminal and the getTerminal property on the mocked
getState() in the usePtyProcess.test.ts file since usePtyProcess reads terminal
state via store.terminals.find(...) and never calls getTerminal; update any
related mock setup that references mockGetTerminal so the test only mocks the
state shape actually consumed by usePtyProcess.
- Around line 23-29: The test currently replaces the entire jsdom window via
vi.stubGlobal('window', {...}), which clobbers DOM globals; instead attach the
mock surface onto the real jsdom window by defining window.electronAPI to your
mocks (createTerminal: mockCreateTerminal, destroyTerminal: mockDestroyTerminal,
restoreTerminalSession: mockRestoreTerminalSession) before invoking
usePtyProcess — e.g. use Object.defineProperty(globalThis.window, 'electronAPI',
{ value: { createTerminal: mockCreateTerminal, destroyTerminal:
mockDestroyTerminal, restoreTerminalSession: mockRestoreTerminalSession },
configurable: true }) or an equivalent vi.stubGlobal('electronAPI', ...)
approach so usePtyProcess reads window.electronAPI without replacing
document/location/navigator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6eaafaaf-08eb-4a31-b996-d96466731603
📒 Files selected for processing (1)
apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts`:
- Around line 37-54: Move the mockSetTerminalStatus declaration so it appears
before the vi.mock factory that closes over it (i.e., define
mockSetTerminalStatus above the useTerminalStore mock) to avoid referencing the
variable before declaration; also remove the redundant type annotation on
mockIsRestored (drop the unnecessary ": boolean") in the test setup and ensure
useTerminalStore's getState continues to return terminals and setTerminalStatus
referencing the now-predeclared mockSetTerminalStatus.
- Around line 126-134: The test's comment and assertion are misleading:
createTerminal is triggered by the "new-terminal" branch selected via isRestored
=== false (in usePtyProcess), not by status === 'running'; update the test to
either correct the comment to remove "(e.g. reconnect)" and explicitly set/mock
mockIsRestored = false and assert mockCreateTerminal was called, or better add a
separate test that sets mockIsRestored = true and status = 'running' and asserts
mockRestoreTerminalSession is called (use the same renderHook invocation with
usePtyProcess and the mockCreateTerminal / mockRestoreTerminalSession symbols to
lock branch selection).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 73618710-71ba-440d-9ff3-1d2d5300ddac
📒 Files selected for processing (1)
apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts
- Move mockSetTerminalStatus declaration before vi.mock to eliminate TDZ footgun - Remove redundant `: boolean` annotation on mockIsRestored - Complete terminal mock shape (createdAt, title, etc.) required by restore path - Rename running-status test to clarify it exercises the new-terminal branch (isRestored=false) - Add negative assertion: mockRestoreTerminalSession not called on new-terminal branch - Add restore-branch test (isRestored=true): asserts restoreTerminalSession called, createTerminal skipped, and updateTerminal resets isRestored flag Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts`:
- Around line 115-118: The test currently only checks that setTerminalStatus was
called with 'idle' which can be satisfied by a later 'running' call; update the
test in usePtyProcess.test.ts to lock the call order: assert that
mockSetTerminalStatus was first called with ('term-1', 'idle') using jest's
toHaveBeenNthCalledWith(1, 'term-1', 'idle'), then assert mockCreateTerminal was
called (toHaveBeenCalledTimes(1)) and finally assert that a subsequent call to
mockSetTerminalStatus sets ('term-1', 'running') (e.g.,
toHaveBeenNthCalledWith(2, 'term-1', 'running')), so the reset happens before
PTY creation (references: createTerminal, setTerminalStatus, usePtyProcess).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c5b20eb5-0fb0-44c5-9111-9735687d060f
📒 Files selected for processing (1)
apps/desktop/src/renderer/components/terminal/__tests__/usePtyProcess.test.ts
…With Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the shell exited naturally (e.g. typing
exit), the terminal would enter an infinite mount/unmount cycle. TerminalGrid's pendingCleanup grace period (150ms) kept remounting the Terminal component while its status was still 'exited', causing usePtyProcess to spin up a new PTY on each mount before the previous one could completeGuard: skip PTY creation when status === 'exited' and isRecreatingRef is not set, which is the deliberate-recreation path (worktree switch)
Adds usePtyProcess.test.ts with 6 cases covering the guard, the recreation exception, and the normal creation path
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Fixes an infinite mount/unmount loop triggered when the user types
exitin a terminal. TerminalGrid'spendingCleanupgrace period (150ms) kept remounting the Terminal component whilestatus === 'exited', causingusePtyProcessto spawn a new PTY on each mount.The fix adds a guard that skips PTY creation when the terminal has exited naturally, while preserving the deliberate-recreation path used by worktree switching.
Related Issue
N/A
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemAI Disclosure
Tool(s) used:
Testing level:
Untested -- AI output not yet verified
Lightly tested -- ran the app / spot-checked key paths
Fully tested -- all tests pass, manually verified behavior
I understand what this PR does and how the underlying code works
Checklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksfindExecutable()or platform abstractions)If you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Summary by CodeRabbit
Bug Fixes
Tests
Documentation