Handle terminal exit writes and auto-close exited terminal drawer - #115
Conversation
- Ignore late write calls after a terminal session has already exited - Add regression test for trailing writes after exit - Close terminal drawer session once an exit event is handled
📝 WalkthroughWalkthroughThese changes implement terminal session exit handling across frontend and backend. The backend prevents write errors when a session has exited, a new test validates this behavior, and the frontend threads an exit callback through terminal components to trigger drawer closure. Changes
Sequence Diagram(s)sequenceDiagram
participant Drawer as ThreadTerminalDrawer
participant Viewport as TerminalViewport
participant Terminal as Terminal Process
participant Manager as Manager
Terminal->>Manager: emit "exited" event
Manager->>Viewport: terminal "exited" event
Viewport->>Viewport: check hasHandledExitRef
Note over Viewport: Set hasHandledExitRef = true
Viewport->>Viewport: setTimeout(() => onSessionExited(), 0)
Viewport->>Drawer: onSessionExited callback
Drawer->>Drawer: onCloseTerminal(terminalId)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/ThreadTerminalDrawer.tsx (1)
347-353: Guard deferred close callback after unmount.The zero-delay callback can still fire after cleanup. Add a
disposedcheck before invokingonSessionExitedRef.current()to avoid stale close calls.Suggested patch
hasHandledExitRef.current = true; window.setTimeout(() => { + if (disposed) return; onSessionExitedRef.current(); }, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ThreadTerminalDrawer.tsx` around lines 347 - 353, The deferred zero-delay callback can run after the component unmounts; update the timeout callback in ThreadTerminalDrawer so it checks a disposal flag before calling onSessionExitedRef.current(): add or reuse a disposedRef (e.g., const disposedRef = useRef(false) and set disposedRef.current = true in the cleanup/unmount), then change the setTimeout handler to if (!disposedRef.current && !hasHandledExitRef.current) { hasHandledExitRef.current = true; onSessionExitedRef.current(); } (or at minimum check disposedRef.current before invoking onSessionExitedRef.current()) to avoid stale close calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/ThreadTerminalDrawer.tsx`:
- Around line 347-353: The deferred zero-delay callback can run after the
component unmounts; update the timeout callback in ThreadTerminalDrawer so it
checks a disposal flag before calling onSessionExitedRef.current(): add or reuse
a disposedRef (e.g., const disposedRef = useRef(false) and set
disposedRef.current = true in the cleanup/unmount), then change the setTimeout
handler to if (!disposedRef.current && !hasHandledExitRef.current) {
hasHandledExitRef.current = true; onSessionExitedRef.current(); } (or at minimum
check disposedRef.current before invoking onSessionExitedRef.current()) to avoid
stale close calls.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/Layers/Manager.tsapps/web/src/components/ThreadTerminalDrawer.tsx
- Skip `onSessionExited` if exit handling was reset before timeout runs - Prevent stale async exit callbacks from firing in terminal drawer
…eep reopened Fallout from #56, caught by re-checking the alert list after it merged rather than assuming the count only goes down. Before the sweep the tree had one nanoid@3.3.12, and GHSA pingdotgg#115/pingdotgg#116 against it sat auto-dismissed by GitHub's auto-triage rule — it was scoped as a development dependency. astro 7.2.0 restructured its tree, adding an already-fixed nanoid@3.3.17 alongside the old copy and flipping that copy's scope to runtime, which took it out from under the rule and reopened both alerts. The sweep did not introduce a vulnerability — 3.3.12 was there before and was always affected — but it turned a suppressed finding into a live one. One override, "nanoid@3": ^3.3.17, dedupes onto the version already in the tree. Both advisories want <= 3.3.17, so this clears both. Ledger figures refreshed in the same commit per SEAMS.md's self-reference rule.
…eep reopened Fallout from #56, caught by re-checking the alert list after it merged rather than assuming the count only goes down. Before the sweep the tree had one nanoid@3.3.12, and GHSA pingdotgg#115/pingdotgg#116 against it sat auto-dismissed by GitHub's auto-triage rule — it was scoped as a development dependency. astro 7.2.0 restructured its tree, adding an already-fixed nanoid@3.3.17 alongside the old copy and flipping that copy's scope to runtime, which took it out from under the rule and reopened both alerts. The sweep did not introduce a vulnerability — 3.3.12 was there before and was always affected — but it turned a suppressed finding into a live one. One override, "nanoid@3": ^3.3.17, dedupes onto the version already in the tree. Both advisories want <= 3.3.17, so this clears both. Ledger figures refreshed in the same commit per SEAMS.md's self-reference rule.
…eep reopened Fallout from #56, caught by re-checking the alert list after it merged rather than assuming the count only goes down. Before the sweep the tree had one nanoid@3.3.12, and GHSA pingdotgg#115/pingdotgg#116 against it sat auto-dismissed by GitHub's auto-triage rule — it was scoped as a development dependency. astro 7.2.0 restructured its tree, adding an already-fixed nanoid@3.3.17 alongside the old copy and flipping that copy's scope to runtime, which took it out from under the rule and reopened both alerts. The sweep did not introduce a vulnerability — 3.3.12 was there before and was always affected — but it turned a suppressed finding into a live one. One override, "nanoid@3": ^3.3.17, dedupes onto the version already in the tree. Both advisories want <= 3.3.17, so this clears both. Ledger figures refreshed in the same commit per SEAMS.md's self-reference rule.
Summary
writecalls when a session has already exited, instead of throwing an error.Testing
apps/server/src/terminal/Layers/Manager.test.ts(ignores trailing writes after terminal exit).Note
Medium Risk
Medium risk because it changes terminal I/O error handling and introduces new UI side effects (auto-closing) on
exitedevents; could mask legitimate client/server state bugs or close tabs unexpectedly if exit events are misfired/duplicated.Overview
Prevents server-side terminal
writecalls from throwing after a session has already exited by returning early whenstatus === "exited".Adds a unit test ensuring post-exit writes are ignored, and updates the web
ThreadTerminalDrawer/TerminalViewportto auto-close a terminal tab when it receives anexitedevent, with guards to avoid duplicate exit handling and to reset the exit guard on restart.Written by Cursor Bugbot for commit e6fd90d. This will update automatically on new commits. Configure here.
Note
Ignore writes after terminal exit in
TerminalManagerRuntime.writeand auto-closeThreadTerminalDrawerwhen a session exitsUpdate server write behavior to no-op on exited sessions and add client-side auto-close on terminal
exitedevents. Add a test covering trailing writes after process exit in Manager.test.ts, adjustTerminalManagerRuntime.writein Manager.ts, and wireonSessionExitedto close drawers in ThreadTerminalDrawer.tsx.📍Where to Start
Start with
TerminalManagerRuntime.writein Manager.ts, then review the exit handling flow in ThreadTerminalDrawer.tsx, and validate behavior via the new test in Manager.test.ts.Macroscope summarized e6fd90d.
Summary by CodeRabbit
Bug Fixes
New Features