fix(ui): exit cleanly on Ctrl-C#292
Conversation
Greptile SummaryRoutes Ctrl-C through Hunk's full
Confidence Score: 4/5Safe to merge; the shutdown path is idempotent and both the keypress and signal routes are correctly guarded against double-invocation. The core logic is sound — disabling OpenTUI's renderer-only teardown and wiring the full shutdown path through both Ctrl-C keypresses and process signals. The lazy no-op stub pattern ensures the closure in shutdown() always reads the final support objects. The only gap is a missing test for the renderer.isDestroyed branch inside the new keypress listener. src/core/jobControl.test.ts — the isDestroyed guard in installJobControlInterruptSupport is untested. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Terminal
participant OpenTUI as OpenTUI Renderer
participant JCI as installJobControlInterruptSupport
participant Main as main() shutdown()
participant Process as Node.js Process
Note over User,Process: Ctrl-C in raw mode (new path)
User->>Terminal: press Ctrl-C
Terminal->>OpenTUI: keypress event (raw mode)
OpenTUI->>JCI: keypressListener(key)
JCI->>JCI: isCtrlC(key)? disposed? isDestroyed?
JCI->>Main: onInterrupt() → shutdown()
Main->>Process: process.off("SIGINT", shutdown)
Main->>Process: process.off("SIGTERM", shutdown)
Main->>JCI: jobControlInterruptSupport.dispose()
Main->>Main: shutdownSession()
Note over User,Process: External SIGINT/SIGTERM (new path)
User->>Process: kill -INT / kill -TERM
Process->>Main: shutdown() (via process.once handler)
Main->>Main: "shuttingDown = true"
Main->>Process: process.off(signal, shutdown) × 2
Main->>JCI: jobControlInterruptSupport.dispose()
Main->>Main: shutdownSession()
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/core/jobControl.test.ts:108-141
**Missing test for `renderer.isDestroyed` guard**
`installJobControlInterruptSupport`'s keypress listener bails out when `renderer.isDestroyed` is `true`, but neither of the new tests exercises that branch. If the renderer is destroyed before the keypress fires (e.g. the renderer tears itself down before the keypress event is fully dispatched), `onInterrupt` should stay silent — worth covering alongside the existing `disposed` path to lock in that contract.
Reviews (1): Last reviewed commit: "fix(ui): exit cleanly on Ctrl-C" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("installJobControlInterruptSupport", () => { | ||
| test("routes Ctrl-C through the provided shutdown callback", () => { | ||
| const renderer = createMockRenderer(); | ||
| let interruptCalls = 0; | ||
|
|
||
| installJobControlInterruptSupport(renderer, () => { | ||
| interruptCalls += 1; | ||
| }); | ||
|
|
||
| const ctrlC = createTestKey({ ctrl: true, name: "c" }); | ||
| renderer.emitKeypress(ctrlC); | ||
|
|
||
| expect(ctrlC.defaultPrevented).toBe(true); | ||
| expect(ctrlC.propagationStopped).toBe(true); | ||
| expect(interruptCalls).toBe(1); | ||
| }); | ||
|
|
||
| test("ignores non-Ctrl-C keys and removes its listener on dispose", () => { | ||
| const renderer = createMockRenderer(); | ||
| let interruptCalls = 0; | ||
| const support = installJobControlInterruptSupport(renderer, () => { | ||
| interruptCalls += 1; | ||
| }); | ||
|
|
||
| renderer.emitKeypress(createTestKey({ ctrl: true, name: "z" })); | ||
| expect(interruptCalls).toBe(0); | ||
|
|
||
| support.dispose(); | ||
| expect(renderer.keypressListeners.size).toBe(0); | ||
|
|
||
| renderer.emitKeypress(createTestKey({ ctrl: true, name: "c" })); | ||
| expect(interruptCalls).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
renderer.isDestroyed guard
installJobControlInterruptSupport's keypress listener bails out when renderer.isDestroyed is true, but neither of the new tests exercises that branch. If the renderer is destroyed before the keypress fires (e.g. the renderer tears itself down before the keypress event is fully dispatched), onInterrupt should stay silent — worth covering alongside the existing disposed path to lock in that contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/jobControl.test.ts
Line: 108-141
Comment:
**Missing test for `renderer.isDestroyed` guard**
`installJobControlInterruptSupport`'s keypress listener bails out when `renderer.isDestroyed` is `true`, but neither of the new tests exercises that branch. If the renderer is destroyed before the keypress fires (e.g. the renderer tears itself down before the keypress event is fully dispatched), `onInterrupt` should stay silent — worth covering alongside the existing `disposed` path to lock in that contract.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Added a regression test that sets renderer.isDestroyed = true before emitting Ctrl-C and verifies the interrupt callback is not called and the event is left untouched.
This comment was generated by Pi using OpenAI GPT-5.1 Codex
1 participant
Summary
Tests
bun test src/core/jobControl.test.tsbun run typecheckThis PR description was generated by Pi using OpenAI GPT-5.1 Codex