fix: terminal drawer#1966
Merged
Merged
Conversation
Contributor
Greptile SummaryThis PR refactors both
Confidence Score: 5/5Safe to merge — the changes are structurally sound, previously identified reaction-disposer leaks are fixed, and session lifecycle is now correctly lazy. The refactor is well-executed: both managers now properly store and call their reaction disposers, session objects are no longer eagerly connected for every inactive conversation and terminal, and task.ts disposal now reaches all sessions. The two remaining observations — missing stale-entry pruning in the conversation manager reaction and the absent list.dispose() call — are both defensive-code gaps rather than present defects given the current demand-only loading strategy. src/renderer/features/tasks/conversations/conversation-manager.ts — the reaction and dispose() are asymmetric with the terminal manager counterpart.
|
| Filename | Overview |
|---|---|
| src/renderer/features/tasks/conversations/conversation-manager.ts | Same Resource + reaction refactor as terminal manager; reaction disposer properly stored and called, but the reaction lacks the stale-entry pruning present in TerminalManagerStore, and dispose() is missing list.dispose() for symmetry. |
| src/renderer/features/tasks/terminals/terminal-manager.ts | Refactored to use Resource + reaction pattern; sessions separated into their own map, reaction disposer stored and called in new dispose() method — both previously flagged leaks are now fixed. |
| src/renderer/features/tasks/stores/task-view.tsx | Auto-creation of default terminal moved from setTerminalDrawerOpen into a MobX reaction that reads isLoaded; also adds setFocusedRegion calls for correct focus management on drawer open/close. |
| src/renderer/features/tasks/stores/task.ts | Disposal fixed to delegate to terminals.dispose() instead of manually iterating terminals.terminals, which previously skipped session cleanup. |
| src/renderer/features/tasks/terminals/terminal-panel.tsx | Session references updated to use terminalMgr.sessions map; adds onHoverTerminal to eagerly connect sessions when the user hovers a sidebar entry before selecting it. |
| src/renderer/features/tasks/main-panel.tsx | onResize callback now guards against the initial call (prevPanelSize === undefined), preventing setTerminalDrawerOpen from being triggered on mount. |
| src/renderer/lib/pty/pty-session.ts | Comment updated to reflect new lazy-connect semantics; no logic changes. |
| src/renderer/features/tasks/conversations/context-bar.tsx | activeSessionId and focus call updated to look up session from conversations.sessions map instead of the now-removed ConversationStore.session property. |
| src/renderer/features/tasks/conversations/conversations-panel.tsx | allSessionIds and activeSession updated to read from conversations.sessions map; useMemo dependency arrays adjusted accordingly. |
| src/renderer/features/tasks/terminals/terminal-drawer-sidebar.tsx | Added optional onHover prop to SidebarRow and TerminalDrawerSidebar, wired to onMouseEnter; used by the panel to trigger eager session connect on hover. |
Sequence Diagram
sequenceDiagram
participant TMS as TerminalManagerStore
participant R as Resource
participant Rx as MobX Reaction
participant TV as TaskViewStore
participant PS as PtySession
TMS->>R: new Resource demand strategy
TMS->>Rx: reaction on list.data fireImmediately
Rx-->>R: observes list.data triggers demand load
R->>R: load via rpc.terminals.getTerminalsForTask
R-->>Rx: list.data changes null to Terminal array
Rx->>TMS: create TerminalStore and PtySession per id no connect
TV->>TV: reaction isOpen and isLoaded and tabs empty
TV->>TMS: createDefaultTerminal when condition true
TMS->>PS: new PtySession optimistic
Note over PS: status disconnected
PS-->>PS: onBecomeObserved status fires connect
PS->>PS: connect via FrontendPty
Note over PS: status ready
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/renderer/features/tasks/conversations/conversation-manager.ts:48-71
**Stale conversation entries not pruned on list refresh**
The `TerminalManagerStore` reaction explicitly removes terminals whose IDs are absent from the refreshed `list.data`, but the equivalent reaction here only adds new entries and never removes deleted ones. If `list.load()` is called again (e.g., a future poll/event strategy or an explicit reload) and a conversation has been deleted server-side, the old `ConversationStore` and `PtySession` will remain in their maps indefinitely — leaking IPC handles until `dispose()` is eventually called. Adding the same stale-ID sweep that exists in the terminal manager would keep the two stores consistent and safe against strategy changes.
### Issue 2 of 2
src/renderer/features/tasks/conversations/conversation-manager.ts:213-222
**Missing `list.dispose()` in `dispose()`**
`TerminalManagerStore.dispose()` calls `this.list.dispose()` to clean up any active strategies, but `ConversationManagerStore.dispose()` does not. While the current `demand`-only strategy has no stop functions and makes this a no-op today, omitting the call is inconsistent and leaves a silent gap if an event or poll strategy is ever added to the list.
```suggestion
dispose(): void {
this._disposeReaction();
this.offAgentEvents?.();
this.offAgentEvents = null;
this.offSessionExited?.();
this.offSessionExited = null;
for (const session of this.sessions.values()) {
session.dispose();
}
this.list.dispose();
}
```
Reviews (2): Last reviewed commit: "fix: pty disposal" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+37
to
+67
| reaction( | ||
| () => this.list.data, | ||
| (data) => { | ||
| if (!data) return; | ||
| runInAction(() => { | ||
| const incomingIds = new Set(data.map((t) => t.id)); | ||
|
|
||
| // Add new entries (no connect()). | ||
| for (const terminal of data) { | ||
| if (!this.terminals.has(terminal.id)) { | ||
| this.terminals.set(terminal.id, new TerminalStore(terminal)); | ||
| } | ||
| if (!this.sessions.has(terminal.id)) { | ||
| this.sessions.set( | ||
| terminal.id, | ||
| new PtySession(makePtySessionId(terminal.projectId, terminal.taskId, terminal.id)) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Remove stale entries. | ||
| const staleIds = Array.from(this.terminals.keys()).filter((id) => !incomingIds.has(id)); | ||
| for (const id of staleIds) { | ||
| this.sessions.get(id)?.dispose(); | ||
| this.sessions.delete(id); | ||
| this.terminals.delete(id); | ||
| } | ||
| }); | ||
| }, | ||
| { fireImmediately: true } | ||
| ); |
Contributor
There was a problem hiding this comment.
reaction(...) returns a disposer that must be called to stop the reaction. Because it isn't stored, dispose() never tears it down. If Resource.load() is in-flight when dispose() is called, the _inFlight promise completes after list.dispose() (which only stops poll/event strategies, not the pending fetch), sets list.data, and fires this live reaction — creating new PtySession objects inside a store that has already been torn down. Those sessions are never disposed, leaking IPC handles and event listeners.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/terminals/terminal-manager.ts
Line: 37-67
Comment:
**Reaction disposer not stored or cleaned up**
`reaction(...)` returns a disposer that must be called to stop the reaction. Because it isn't stored, `dispose()` never tears it down. If `Resource.load()` is in-flight when `dispose()` is called, the `_inFlight` promise completes after `list.dispose()` (which only stops poll/event strategies, not the pending fetch), sets `list.data`, and fires this live reaction — creating new `PtySession` objects inside a store that has already been torn down. Those sessions are never disposed, leaking IPC handles and event listeners.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+49
to
+70
| reaction( | ||
| () => this.list.data, | ||
| (data) => { | ||
| if (!data) return; | ||
| runInAction(() => { | ||
| for (const conversation of data) { | ||
| if (!this.conversations.has(conversation.id)) { | ||
| this.conversations.set(conversation.id, new ConversationStore(conversation)); | ||
| } | ||
| if (!this.sessions.has(conversation.id)) { | ||
| this.sessions.set( | ||
| conversation.id, | ||
| new PtySession( | ||
| makePtySessionId(conversation.projectId, conversation.taskId, conversation.id) | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| }); | ||
| }, | ||
| { fireImmediately: true } | ||
| ); |
Contributor
There was a problem hiding this comment.
Same issue as the terminal manager: the reaction() disposer is discarded. ConversationManagerStore.dispose() calls session disposals and unsubscribes agent-event listeners, but the MobX reaction that watches list.data keeps running. If list.load() is in-flight at dispose time, it will complete, write to list.data, and trigger the reaction — adding ConversationStore and PtySession instances to a store that has already been torn down, with no cleanup path for those objects.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/conversations/conversation-manager.ts
Line: 49-70
Comment:
**Reaction disposer not stored or cleaned up**
Same issue as the terminal manager: the `reaction()` disposer is discarded. `ConversationManagerStore.dispose()` calls session disposals and unsubscribes agent-event listeners, but the MobX reaction that watches `list.data` keeps running. If `list.load()` is in-flight at dispose time, it will complete, write to `list.data`, and trigger the reaction — adding `ConversationStore` and `PtySession` instances to a store that has already been torn down, with no cleanup path for those objects.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+16
to
+39
| /** Session layer keyed by terminal id — created alongside data, connected lazily. */ | ||
| sessions = observable.map<string, PtySession>(); | ||
|
|
||
| constructor(projectId: string, taskId: string) { | ||
| this.projectId = projectId; | ||
| this.taskId = taskId; | ||
|
|
||
| this.list = new Resource<Terminal[]>( | ||
| () => rpc.terminals.getTerminalsForTask(projectId, taskId), | ||
| [{ kind: 'demand' }] | ||
| ); | ||
|
|
||
| makeObservable(this, { | ||
| terminals: observable, | ||
| sessions: observable, | ||
| isLoaded: computed, | ||
| }); | ||
| onBecomeObserved(this, 'terminals', () => { | ||
| if (this._loaded) return; | ||
| void this.load(); | ||
| }); | ||
|
|
||
| // Sync terminals and sessions maps whenever the resource data changes. | ||
| // fireImmediately ensures the reaction runs once on construction to establish | ||
| // the dependency on list.data, which triggers the demand-strategy load. | ||
| reaction( | ||
| () => this.list.data, | ||
| (data) => { |
Contributor
There was a problem hiding this comment.
dispose() to prevent the reaction from firing after the store has been torn down.
Suggested change
| /** Session layer keyed by terminal id — created alongside data, connected lazily. */ | |
| sessions = observable.map<string, PtySession>(); | |
| constructor(projectId: string, taskId: string) { | |
| this.projectId = projectId; | |
| this.taskId = taskId; | |
| this.list = new Resource<Terminal[]>( | |
| () => rpc.terminals.getTerminalsForTask(projectId, taskId), | |
| [{ kind: 'demand' }] | |
| ); | |
| makeObservable(this, { | |
| terminals: observable, | |
| sessions: observable, | |
| isLoaded: computed, | |
| }); | |
| onBecomeObserved(this, 'terminals', () => { | |
| if (this._loaded) return; | |
| void this.load(); | |
| }); | |
| // Sync terminals and sessions maps whenever the resource data changes. | |
| // fireImmediately ensures the reaction runs once on construction to establish | |
| // the dependency on list.data, which triggers the demand-strategy load. | |
| reaction( | |
| () => this.list.data, | |
| (data) => { | |
| /** Session layer keyed by terminal id — created alongside data, connected lazily. */ | |
| sessions = observable.map<string, PtySession>(); | |
| private readonly _disposeReaction: () => void; | |
| constructor(projectId: string, taskId: string) { | |
| this.projectId = projectId; | |
| this.taskId = taskId; | |
| this.list = new Resource<Terminal[]>( | |
| () => rpc.terminals.getTerminalsForTask(projectId, taskId), | |
| [{ kind: 'demand' }] | |
| ); | |
| makeObservable(this, { | |
| terminals: observable, | |
| sessions: observable, | |
| isLoaded: computed, | |
| }); | |
| // Sync terminals and sessions maps whenever the resource data changes. | |
| // fireImmediately ensures the reaction runs once on construction to establish | |
| // the dependency on list.data, which triggers the demand-strategy load. | |
| this._disposeReaction = reaction( | |
| () => this.list.data, | |
| (data) => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/terminals/terminal-manager.ts
Line: 16-39
Comment:
Store the reaction disposer and call it in `dispose()` to prevent the reaction from firing after the store has been torn down.
```suggestion
/** Session layer keyed by terminal id — created alongside data, connected lazily. */
sessions = observable.map<string, PtySession>();
private readonly _disposeReaction: () => void;
constructor(projectId: string, taskId: string) {
this.projectId = projectId;
this.taskId = taskId;
this.list = new Resource<Terminal[]>(
() => rpc.terminals.getTerminalsForTask(projectId, taskId),
[{ kind: 'demand' }]
);
makeObservable(this, {
terminals: observable,
sessions: observable,
isLoaded: computed,
});
// Sync terminals and sessions maps whenever the resource data changes.
// fireImmediately ensures the reaction runs once on construction to establish
// the dependency on list.data, which triggers the demand-strategy load.
this._disposeReaction = reaction(
() => this.list.data,
(data) => {
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+142
to
+147
| dispose(): void { | ||
| for (const session of this.sessions.values()) { | ||
| session.dispose(); | ||
| } | ||
| this.list.dispose(); | ||
| } |
Contributor
There was a problem hiding this comment.
dispose() before tearing down sessions and the resource.
Suggested change
| dispose(): void { | |
| for (const session of this.sessions.values()) { | |
| session.dispose(); | |
| } | |
| this.list.dispose(); | |
| } | |
| dispose(): void { | |
| this._disposeReaction(); | |
| for (const session of this.sessions.values()) { | |
| session.dispose(); | |
| } | |
| this.list.dispose(); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/terminals/terminal-manager.ts
Line: 142-147
Comment:
Call the reaction disposer in `dispose()` before tearing down sessions and the resource.
```suggestion
dispose(): void {
this._disposeReaction();
for (const session of this.sessions.values()) {
session.dispose();
}
this.list.dispose();
}
```
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
Author
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.
Fixes: