fix(main): stop hidden conversation sessions#2198
Conversation
Greptile SummaryThis PR introduces a
Confidence Score: 5/5Safe to merge; the grace-period logic is sound and the regression risks are well-covered by the new test suite. All five scenarios flagged in the prior review thread are addressed: src/main/core/conversations/conversation-session-visibility.ts — the
|
| Filename | Overview |
|---|---|
| src/main/core/conversations/conversation-session-visibility.ts | New service tracking visible conversations and scheduling kill timers for hidden PTY sessions; introduces trackedTasks set that is never cleaned up |
| src/main/core/conversations/conversation-session-visibility.test.ts | New test file covering the five main scenarios including grace period, cancellation, deferred starts on empty/non-empty visibility sets, and task removal before timer fire |
| src/renderer/features/tasks/stores/workspace-view-model.tsx | Adds MobX reaction to track visible conversation IDs and report them via IPC; uses comparer.structural to avoid spurious calls; adds explicit empty-list call on dispose |
| src/main/core/conversations/controller.ts | Adds updateVisibleConversations RPC endpoint that delegates to the new visibility service |
| src/main/core/conversations/impl/local-conversation.ts | Calls onConversationSessionStarted immediately after PTY session is registered, correctly ordering the registry write before the visibility check |
| src/main/core/conversations/impl/ssh-conversation.ts | Mirror of local-conversation.ts change: hooks SSH sessions into the visibility service after PTY registration |
| src/renderer/features/tasks/stores/workspace-view-model.test.ts | Adds updateVisibleConversations stub to the IPC mock so existing tests continue to compile and pass |
Sequence Diagram
sequenceDiagram
participant R as Renderer (WorkspaceViewModel)
participant IPC as IPC / Controller
participant VS as ConversationSessionVisibilityService
participant PR as PtySessionRegistry
participant Task as Task.conversations
Note over R: Constructor — fireImmediately reaction
R->>IPC: updateVisibleConversations(projectId, taskId, [conv-A])
IPC->>VS: updateVisibleConversations(...)
VS->>VS: trackedTasks.add(key)
VS->>VS: "visibleConversationsByTask.set(key, {conv-A})"
VS->>VS: cancel timer for conv-A
Note over R: User navigates away — isActive = false
R->>IPC: updateVisibleConversations(projectId, taskId, [])
IPC->>VS: updateVisibleConversations(..., [])
VS->>VS: trackedTasks.add(key)
VS->>VS: visibleConversationsByTask.delete(key)
VS->>PR: listActiveSessions()
PR-->>VS: [conv-A session]
VS->>Task: resolveTask — capture stopSession ref
VS->>VS: "schedule(conv-A, timer=30s)"
Note over VS: 29999 ms pass
Note over R: User navigates back — isActive = true
R->>IPC: updateVisibleConversations(..., [conv-A])
IPC->>VS: updateVisibleConversations(..., [conv-A])
VS->>VS: cancel timer for conv-A
Note over VS: alternatively, timer fires at 30s
VS->>PR: get(sessionId) — still active?
PR-->>VS: PTY instance
VS->>Task: stopSession(conv-A)
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/main/core/conversations/conversation-session-visibility.ts:9-13
`trackedTasks` is never pruned. `updateVisibleConversations` always adds the task key but nothing ever removes it, so the set grows by one entry per task ever opened for the lifetime of the process. The prior review thread called out the same pattern for `visibleConversationsByTask` (now fixed). A more consequential effect: if a workspace is disposed and immediately recreated for the same task ID — which can happen during task re-provisioning — `onConversationSessionStarted` will see `trackedTasks.has(key) === true` but `visibleConversationsByTask.get(key) === undefined`, so it will schedule a 30 s kill timer for every new session before the new workspace's `fireImmediately` reaction can cancel it. The timer races the IPC round-trip; in practice the reaction wins, but the intermediate state is incorrect. Adding a `removeTask` method (called from `dispose()` alongside the explicit `[]` IPC) would clean up both sets.
```suggestion
private readonly killTimers = new Map<string, ReturnType<typeof setTimeout>>();
private readonly visibleConversationsByTask = new Map<string, Set<string>>();
private readonly trackedTasks = new Set<string>();
removeTask(projectId: string, taskId: string) {
const taskKey = this.taskKey(projectId, taskId);
this.trackedTasks.delete(taskKey);
this.visibleConversationsByTask.delete(taskKey);
}
updateVisibleConversations(projectId: string, taskId: string, visibleConversationIds: string[]) {
```
Reviews (3): Last reviewed commit: "fix: track empty conversation visibility" | Re-trigger Greptile
summary