From a7e504e205c1d7b7cd8abc2465c34bef60061429 Mon Sep 17 00:00:00 2001 From: Pedram Amini Date: Tue, 3 Feb 2026 23:21:06 -0600 Subject: [PATCH] =?UTF-8?q?##=20CHANGES=20-=20Symphony=20IPC=20now=20valid?= =?UTF-8?q?ates=20active=20contributions=20against=20stored=20sessions=20?= =?UTF-8?q?=F0=9F=A7=A9=20-=20Orphaned=20contributions=20auto-filtered=20w?= =?UTF-8?q?hen=20sessions=20disappear,=20keeping=20UI=20clean=20?= =?UTF-8?q?=F0=9F=A7=B9=20-=20`symphony:getState`=20returns=20only=20sessi?= =?UTF-8?q?on-backed=20active=20items=20for=20accuracy=20=F0=9F=8E=AF=20-?= =?UTF-8?q?=20`symphony:getActive`=20now=20excludes=20contributions=20tied?= =?UTF-8?q?=20to=20missing=20sessions=20=F0=9F=9A=AB=20-=20Added=20reusabl?= =?UTF-8?q?e=20`filterOrphanedContributions`=20helper=20with=20detailed=20?= =?UTF-8?q?logging=20=F0=9F=AA=B5=20-=20Wired=20`sessionsStore`=20dependen?= =?UTF-8?q?cy=20through=20main=20+=20handler=20registration=20flow=20?= =?UTF-8?q?=F0=9F=94=8C=20-=20Integration=20tests=20now=20mock=20sessions?= =?UTF-8?q?=20store=20for=20realistic=20state=20scenarios=20=F0=9F=A7=AA?= =?UTF-8?q?=20-=20Expanded=20handler=20tests=20to=20cover=20missing-sessio?= =?UTF-8?q?n=20filtering=20behavior=20=F0=9F=9B=A1=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../integration/symphony.integration.test.ts | 7 ++ .../main/ipc/handlers/symphony.test.ts | 86 ++++++++++++++++++- src/main/index.ts | 1 + src/main/ipc/handlers/index.ts | 1 + src/main/ipc/handlers/symphony.ts | 44 +++++++++- 5 files changed, 135 insertions(+), 4 deletions(-) diff --git a/src/__tests__/integration/symphony.integration.test.ts b/src/__tests__/integration/symphony.integration.test.ts index b45c157b..5c63cfec 100644 --- a/src/__tests__/integration/symphony.integration.test.ts +++ b/src/__tests__/integration/symphony.integration.test.ts @@ -222,10 +222,17 @@ describe('Symphony Integration Tests', () => { }, } as unknown as BrowserWindow; + // Setup mock sessions store (returns empty by default - no sessions) + const mockSessionsStore = { + get: vi.fn().mockReturnValue([]), + set: vi.fn(), + }; + // Setup dependencies mockDeps = { app: mockApp, getMainWindow: () => mockMainWindow, + sessionsStore: mockSessionsStore as any, }; // Default fetch mock (successful responses) diff --git a/src/__tests__/main/ipc/handlers/symphony.test.ts b/src/__tests__/main/ipc/handlers/symphony.test.ts index df706b60..07df3890 100644 --- a/src/__tests__/main/ipc/handlers/symphony.test.ts +++ b/src/__tests__/main/ipc/handlers/symphony.test.ts @@ -64,6 +64,7 @@ describe('Symphony IPC handlers', () => { let mockApp: App; let mockMainWindow: BrowserWindow; let mockDeps: SymphonyHandlerDependencies; + let mockSessionsStore: { get: ReturnType; set: ReturnType }; beforeEach(() => { vi.clearAllMocks(); @@ -88,10 +89,17 @@ describe('Symphony IPC handlers', () => { }, } as unknown as BrowserWindow; + // Setup mock sessions store (exposed for individual tests to modify) + mockSessionsStore = { + get: vi.fn().mockReturnValue([]), + set: vi.fn(), + }; + // Setup dependencies mockDeps = { app: mockApp, getMainWindow: () => mockMainWindow, + sessionsStore: mockSessionsStore as any, }; // Default mock for fs operations @@ -1209,7 +1217,7 @@ describe('Symphony IPC handlers', () => { expect(result.state.stats.repositoriesContributed).toEqual([]); }); - it('should return persisted state from disk', async () => { + it('should return persisted state from disk (with valid sessions)', async () => { const persistedState = { active: [ { @@ -1267,17 +1275,49 @@ describe('Symphony IPC handlers', () => { }, }; vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(persistedState)); + // Mock the session to exist so the active contribution is included + mockSessionsStore.get.mockReturnValue([{ id: 'session-123', name: 'Test Session' }]); const handler = handlers.get('symphony:getState'); const result = await handler!({} as any); - expect(result.state).toEqual(persistedState); expect(result.state.active).toHaveLength(1); expect(result.state.active[0].id).toBe('contrib_123'); expect(result.state.history).toHaveLength(1); expect(result.state.stats.totalContributions).toBe(1); }); + it('should filter out active contributions with missing sessions', async () => { + const persistedState = { + active: [ + { + id: 'contrib_with_session', + repoSlug: 'owner/repo', + sessionId: 'session-exists', + status: 'running', + }, + { + id: 'contrib_orphaned', + repoSlug: 'owner/repo2', + sessionId: 'session-gone', + status: 'running', + }, + ], + history: [], + stats: { totalContributions: 0 }, + }; + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(persistedState)); + // Only one session exists + mockSessionsStore.get.mockReturnValue([{ id: 'session-exists', name: 'Existing' }]); + + const handler = handlers.get('symphony:getState'); + const result = await handler!({} as any); + + // Only the contribution with an existing session should be returned + expect(result.state.active).toHaveLength(1); + expect(result.state.active[0].id).toBe('contrib_with_session'); + }); + it('should handle file read errors gracefully', async () => { vi.mocked(fs.readFile).mockRejectedValue(new Error('Permission denied')); @@ -1302,7 +1342,7 @@ describe('Symphony IPC handlers', () => { expect(result.contributions).toEqual([]); }); - it('should return all active contributions from state', async () => { + it('should return active contributions that have matching sessions', async () => { const stateWithActive = { active: [ { @@ -1310,18 +1350,25 @@ describe('Symphony IPC handlers', () => { repoSlug: 'owner/repo1', issueNumber: 1, status: 'running', + sessionId: 'session_1', }, { id: 'contrib_2', repoSlug: 'owner/repo2', issueNumber: 2, status: 'paused', + sessionId: 'session_2', }, ], history: [], stats: {}, }; vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(stateWithActive)); + // Mock sessions store to return matching sessions + mockSessionsStore.get.mockReturnValue([ + { id: 'session_1', name: 'Session 1' }, + { id: 'session_2', name: 'Session 2' }, + ]); const handler = handlers.get('symphony:getActive'); const result = await handler!({} as any); @@ -1330,6 +1377,39 @@ describe('Symphony IPC handlers', () => { expect(result.contributions[0].id).toBe('contrib_1'); expect(result.contributions[1].id).toBe('contrib_2'); }); + + it('should filter out contributions whose sessions no longer exist', async () => { + const stateWithActive = { + active: [ + { + id: 'contrib_1', + repoSlug: 'owner/repo1', + issueNumber: 1, + status: 'running', + sessionId: 'session_exists', + }, + { + id: 'contrib_2', + repoSlug: 'owner/repo2', + issueNumber: 2, + status: 'paused', + sessionId: 'session_gone', + }, + ], + history: [], + stats: {}, + }; + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(stateWithActive)); + // Only return session_exists, session_gone is missing + mockSessionsStore.get.mockReturnValue([{ id: 'session_exists', name: 'Existing Session' }]); + + const handler = handlers.get('symphony:getActive'); + const result = await handler!({} as any); + + // Only contrib_1 should be returned since contrib_2's session is gone + expect(result.contributions).toHaveLength(1); + expect(result.contributions[0].id).toBe('contrib_1'); + }); }); describe('symphony:getCompleted', () => { diff --git a/src/main/index.ts b/src/main/index.ts index 6ea32e45..3208659b 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -608,6 +608,7 @@ function setupIpcHandlers() { registerSymphonyHandlers({ app, getMainWindow: () => mainWindow, + sessionsStore, }); // Register tab naming handlers for automatic tab naming diff --git a/src/main/ipc/handlers/index.ts b/src/main/ipc/handlers/index.ts index d26c5310..e5e89e00 100644 --- a/src/main/ipc/handlers/index.ts +++ b/src/main/ipc/handlers/index.ts @@ -259,6 +259,7 @@ export function registerAllHandlers(deps: HandlerDependencies): void { registerSymphonyHandlers({ app: deps.app, getMainWindow: deps.getMainWindow, + sessionsStore: deps.sessionsStore, }); // Register agent error handlers (error state management) registerAgentErrorHandlers(); diff --git a/src/main/ipc/handlers/symphony.ts b/src/main/ipc/handlers/symphony.ts index e72cb4cc..8004510b 100644 --- a/src/main/ipc/handlers/symphony.ts +++ b/src/main/ipc/handlers/symphony.ts @@ -11,10 +11,12 @@ */ import { ipcMain, App, BrowserWindow } from 'electron'; +import type Store from 'electron-store'; import fs from 'fs/promises'; import path from 'path'; import { logger } from '../../utils/logger'; import { isWebContentsAvailable } from '../../utils/safe-send'; +import type { SessionsData, StoredSession } from '../../stores/types'; import { createIpcHandler, CreateHandlerOptions } from '../../utils/ipcHandler'; import { execFileNoThrow } from '../../utils/execFile'; import { getExpandedEnv } from '../../agents/path-prober'; @@ -194,6 +196,7 @@ function validateContributionParams(params: { export interface SymphonyHandlerDependencies { app: App; getMainWindow: () => BrowserWindow | null; + sessionsStore: Store; } // ============================================================================ @@ -865,6 +868,39 @@ function broadcastSymphonyUpdate(getMainWindow: () => BrowserWindow | null): voi } } +/** + * Filter out orphaned contributions whose sessions no longer exist. + * Returns only contributions that have a corresponding session in the sessions store. + */ +function filterOrphanedContributions( + contributions: ActiveContribution[], + sessionsStore: Store +): ActiveContribution[] { + const sessions = sessionsStore.get('sessions', []) as StoredSession[]; + const sessionIds = new Set(sessions.map((s) => s.id)); + + const validContributions: ActiveContribution[] = []; + const orphanedIds: string[] = []; + + for (const contribution of contributions) { + if (sessionIds.has(contribution.sessionId)) { + validContributions.push(contribution); + } else { + orphanedIds.push(contribution.id); + } + } + + if (orphanedIds.length > 0) { + logger.info( + `Filtering ${orphanedIds.length} orphaned contribution(s) with missing sessions`, + LOG_CONTEXT, + { orphanedIds } + ); + } + + return validContributions; +} + // ============================================================================ // Handler Options Helper // ============================================================================ @@ -882,6 +918,7 @@ const handlerOpts = (operation: string, logSuccess = true): CreateHandlerOptions export function registerSymphonyHandlers({ app, getMainWindow, + sessionsStore, }: SymphonyHandlerDependencies): void { // ───────────────────────────────────────────────────────────────────────── // Registry Operations @@ -1031,6 +1068,7 @@ export function registerSymphonyHandlers({ /** * Get current symphony state. + * Filters out contributions whose sessions no longer exist. */ ipcMain.handle( 'symphony:getState', @@ -1038,6 +1076,8 @@ export function registerSymphonyHandlers({ handlerOpts('getState', false), async (): Promise<{ state: SymphonyState }> => { const state = await readState(app); + // Filter out orphaned contributions whose sessions are gone + state.active = filterOrphanedContributions(state.active, sessionsStore); return { state }; } ) @@ -1045,6 +1085,7 @@ export function registerSymphonyHandlers({ /** * Get active contributions. + * Filters out contributions whose sessions no longer exist. */ ipcMain.handle( 'symphony:getActive', @@ -1052,7 +1093,8 @@ export function registerSymphonyHandlers({ handlerOpts('getActive', false), async (): Promise<{ contributions: ActiveContribution[] }> => { const state = await readState(app); - return { contributions: state.active }; + const validContributions = filterOrphanedContributions(state.active, sessionsStore); + return { contributions: validContributions }; } ) );