diff --git a/apps/frontend/src/main/__tests__/pr-review-state-manager.test.ts b/apps/frontend/src/main/__tests__/pr-review-state-manager.test.ts new file mode 100644 index 0000000000..03d1264689 --- /dev/null +++ b/apps/frontend/src/main/__tests__/pr-review-state-manager.test.ts @@ -0,0 +1,333 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { PRReviewStateManager } from '../pr-review-state-manager'; +import type { PRReviewResult, PRReviewProgress } from '../../preload/api/modules/github-api'; + +// Mock dependencies +const mockSafeSendToRenderer = vi.fn(); +vi.mock('../ipc-handlers/utils', () => ({ + safeSendToRenderer: (...args: unknown[]) => mockSafeSendToRenderer(...args) +})); + +function createMockGetMainWindow() { + return vi.fn(() => ({ id: 1 }) as unknown as Electron.BrowserWindow); +} + +function createMockProgress(overrides: Partial = {}): PRReviewProgress { + return { + phase: 'analyzing', + progress: 50, + message: 'Analyzing files...', + ...overrides + } as PRReviewProgress; +} + +function createMockResult(overrides: Partial = {}): PRReviewResult { + return { + overallStatus: 'approved', + summary: 'Looks good', + ...overrides + } as PRReviewResult; +} + +describe('PRReviewStateManager', () => { + let manager: PRReviewStateManager; + const projectId = 'project-1'; + const prNumber = 42; + + beforeEach(() => { + manager = new PRReviewStateManager(createMockGetMainWindow()); + vi.clearAllMocks(); + }); + + afterEach(() => { + manager.clearAll(); + }); + + describe('actor lifecycle', () => { + it('should create actor on first handleStartReview call', () => { + manager.handleStartReview(projectId, prNumber); + const snapshot = manager.getState(projectId, prNumber); + expect(snapshot).not.toBeNull(); + }); + + it('should reuse existing actor for same PR key', () => { + manager.handleStartReview(projectId, prNumber); + const snapshot1 = manager.getState(projectId, prNumber); + // Calling again should not create a new actor + manager.handleStartReview(projectId, prNumber); + const snapshot2 = manager.getState(projectId, prNumber); + expect(snapshot1).not.toBeNull(); + expect(snapshot2).not.toBeNull(); + }); + + it('should create separate actors for different PRs', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + const snapshot1 = manager.getState(projectId, 1); + const snapshot2 = manager.getState(projectId, 2); + expect(snapshot1).not.toBeNull(); + expect(snapshot2).not.toBeNull(); + }); + + it('should start actor before events are sent', () => { + manager.handleStartReview(projectId, prNumber); + const snapshot = manager.getState(projectId, prNumber); + // If actor wasn't started, getSnapshot would fail or return unexpected state + expect(snapshot).not.toBeNull(); + expect(String(snapshot!.value)).toBe('reviewing'); + }); + }); + + describe('event routing', () => { + it('should transition to reviewing on handleStartReview', () => { + manager.handleStartReview(projectId, prNumber); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('reviewing'); + }); + + it('should send START_FOLLOWUP_REVIEW with previousResult', () => { + const previousResult = createMockResult(); + manager.handleStartFollowupReview(projectId, prNumber, previousResult); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('reviewing'); + expect(snapshot!.context.isFollowup).toBe(true); + expect(snapshot!.context.previousResult).toBe(previousResult); + }); + + it('should send START_REVIEW when handleStartFollowupReview has no previousResult', () => { + manager.handleStartFollowupReview(projectId, prNumber); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('reviewing'); + expect(snapshot!.context.isFollowup).toBe(false); + }); + + it('should update context on handleProgress', () => { + manager.handleStartReview(projectId, prNumber); + const progress = createMockProgress(); + manager.handleProgress(projectId, prNumber, progress); + const snapshot = manager.getState(projectId, prNumber); + expect(snapshot!.context.progress).toEqual(progress); + }); + + it('should ignore handleProgress for unknown PR', () => { + // Should not throw + manager.handleProgress(projectId, 999, createMockProgress()); + expect(manager.getState(projectId, 999)).toBeNull(); + }); + + it('should transition to completed on handleComplete', () => { + manager.handleStartReview(projectId, prNumber); + const result = createMockResult(); + manager.handleComplete(projectId, prNumber, result); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('completed'); + expect(snapshot!.context.result).toEqual(result); + }); + + it('should create actor for handleComplete on unknown PR (late-arriving result)', () => { + const result = createMockResult(); + // No handleStartReview called — handleComplete should create the actor + manager.handleComplete(projectId, prNumber, result); + const snapshot = manager.getState(projectId, prNumber); + expect(snapshot).not.toBeNull(); + expect(snapshot!.context.result).toEqual(result); + }); + + it('should send DETECT_EXTERNAL_REVIEW when overallStatus is in_progress', () => { + manager.handleStartReview(projectId, prNumber); + const result = createMockResult({ overallStatus: 'in_progress' }); + manager.handleComplete(projectId, prNumber, result); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('externalReview'); + }); + + it('should transition to error on handleError', () => { + manager.handleStartReview(projectId, prNumber); + manager.handleError(projectId, prNumber, 'Something went wrong'); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('error'); + expect(snapshot!.context.error).toBe('Something went wrong'); + }); + + it('should transition to error on handleCancel', () => { + manager.handleStartReview(projectId, prNumber); + manager.handleCancel(projectId, prNumber); + const snapshot = manager.getState(projectId, prNumber); + expect(String(snapshot!.value)).toBe('error'); + }); + }); + + describe('state emission', () => { + it('should emit state changes to renderer via safeSendToRenderer', () => { + manager.handleStartReview(projectId, prNumber); + expect(mockSafeSendToRenderer).toHaveBeenCalled(); + }); + + it('should use GITHUB_PR_REVIEW_STATE_CHANGE IPC channel', () => { + manager.handleStartReview(projectId, prNumber); + expect(mockSafeSendToRenderer).toHaveBeenCalledWith( + expect.any(Function), + 'github:pr:reviewStateChange', + expect.any(String), + expect.objectContaining({ state: expect.any(String) }) + ); + }); + + it('should emit PRReviewStatePayload with correct shape', () => { + manager.handleStartReview(projectId, prNumber); + // Find the call that emits 'reviewing' state + const reviewingCall = mockSafeSendToRenderer.mock.calls.find( + (call: unknown[]) => { + const payload = call[3] as Record | undefined; + return payload && typeof payload === 'object' && payload.state === 'reviewing'; + } + ); + expect(reviewingCall).toBeDefined(); + expect(reviewingCall![2]).toBe(`${projectId}:${prNumber}`); + const payload = reviewingCall![3] as Record; + expect(payload).toEqual(expect.objectContaining({ + state: 'reviewing', + prNumber, + projectId, + isReviewing: true, + startedAt: expect.any(String), + progress: null, + result: null, + previousResult: null, + error: null, + isExternalReview: false, + isFollowup: false, + })); + }); + + it('should use projectId:prNumber as key format', () => { + manager.handleStartReview(projectId, prNumber); + const calls = mockSafeSendToRenderer.mock.calls; + const prCall = calls.find((call: unknown[]) => call[2] === `${projectId}:${prNumber}`); + expect(prCall).toBeDefined(); + }); + }); + + describe('deduplication', () => { + it('should NOT emit duplicate IPC for same state + same context', () => { + manager.handleStartReview(projectId, prNumber); + const callCountAfterStart = mockSafeSendToRenderer.mock.calls.length; + + // Sending START_REVIEW again won't transition (guard prevents it), so no new emission + manager.handleStartReview(projectId, prNumber); + expect(mockSafeSendToRenderer.mock.calls.length).toBe(callCountAfterStart); + }); + + it('should emit for same state but different context (progress update)', () => { + manager.handleStartReview(projectId, prNumber); + const callCountAfterStart = mockSafeSendToRenderer.mock.calls.length; + + manager.handleProgress(projectId, prNumber, createMockProgress({ progress: 25, message: 'Step 1' })); + expect(mockSafeSendToRenderer.mock.calls.length).toBeGreaterThan(callCountAfterStart); + + const callCountAfterProgress1 = mockSafeSendToRenderer.mock.calls.length; + manager.handleProgress(projectId, prNumber, createMockProgress({ progress: 75, message: 'Step 2' })); + expect(mockSafeSendToRenderer.mock.calls.length).toBeGreaterThan(callCountAfterProgress1); + }); + + it('should always emit for different state transitions', () => { + manager.handleStartReview(projectId, prNumber); + const callCountAfterStart = mockSafeSendToRenderer.mock.calls.length; + + manager.handleComplete(projectId, prNumber, createMockResult()); + expect(mockSafeSendToRenderer.mock.calls.length).toBeGreaterThan(callCountAfterStart); + }); + }); + + describe('cleanup', () => { + it('should stop actor and remove from map on handleClearReview', () => { + manager.handleStartReview(projectId, prNumber); + expect(manager.getState(projectId, prNumber)).not.toBeNull(); + + manager.handleClearReview(projectId, prNumber); + expect(manager.getState(projectId, prNumber)).toBeNull(); + }); + + it('should emit exactly one cleared state IPC on handleClearReview (no double emission)', () => { + manager.handleStartReview(projectId, prNumber); + mockSafeSendToRenderer.mockClear(); + + manager.handleClearReview(projectId, prNumber); + + // Should emit exactly 1 cleared state, not 2 (no double emission from + // sending CLEAR_REVIEW to actor subscription + manual emitClearedState) + expect(mockSafeSendToRenderer).toHaveBeenCalledTimes(1); + const payload = mockSafeSendToRenderer.mock.calls[0][3] as Record; + expect(payload).toEqual(expect.objectContaining({ state: 'idle' })); + }); + + it('should stop ALL actors and clear maps on handleAuthChange', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + + manager.handleAuthChange(); + + expect(manager.getState(projectId, 1)).toBeNull(); + expect(manager.getState(projectId, 2)).toBeNull(); + }); + + it('should emit cleared state to renderer on handleAuthChange', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + mockSafeSendToRenderer.mockClear(); + + manager.handleAuthChange(); + + // Should emit idle/null state for each PR + expect(mockSafeSendToRenderer).toHaveBeenCalledTimes(2); + for (const call of mockSafeSendToRenderer.mock.calls) { + const payload = call[3] as Record; + expect(payload).toEqual(expect.objectContaining({ state: 'idle' })); + } + }); + + it('should stop all actors on clearAll', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + + manager.clearAll(); + + expect(manager.getState(projectId, 1)).toBeNull(); + expect(manager.getState(projectId, 2)).toBeNull(); + }); + }); + + describe('concurrent PRs', () => { + it('should support multiple PRs with independent actors', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + + manager.handleComplete(projectId, 1, createMockResult()); + + expect(String(manager.getState(projectId, 1)!.value)).toBe('completed'); + expect(String(manager.getState(projectId, 2)!.value)).toBe('reviewing'); + }); + + it('should route events to correct actor by key', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + + manager.handleError(projectId, 2, 'Error on PR 2'); + + expect(String(manager.getState(projectId, 1)!.value)).toBe('reviewing'); + expect(String(manager.getState(projectId, 2)!.value)).toBe('error'); + expect(manager.getState(projectId, 2)!.context.error).toBe('Error on PR 2'); + }); + + it('should not affect other PRs when clearing one', () => { + manager.handleStartReview(projectId, 1); + manager.handleStartReview(projectId, 2); + + manager.handleClearReview(projectId, 1); + + expect(manager.getState(projectId, 1)).toBeNull(); + expect(manager.getState(projectId, 2)).not.toBeNull(); + expect(String(manager.getState(projectId, 2)!.value)).toBe('reviewing'); + }); + }); +}); diff --git a/apps/frontend/src/main/ipc-handlers/github/oauth-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/oauth-handlers.ts index 773feb15e2..37adb6bb2d 100644 --- a/apps/frontend/src/main/ipc-handlers/github/oauth-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/oauth-handlers.ts @@ -51,6 +51,10 @@ function sendAuthChangedToRenderer(oldUsername: string | null, newUsername: stri for (const win of windows) { win.webContents.send(IPC_CHANNELS.GITHUB_AUTH_CHANGED, payload); } + // Uses EventEmitter.emit (not IPC send) so main-process listeners can react. + // The listener (PRReviewStateManager) intentionally ignores all args — it only + // needs the event signal, not the payload. + ipcMain.emit(IPC_CHANNELS.GITHUB_AUTH_CHANGED, payload); } /** diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index 1f63b6c4ab..3c000f4873 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -26,7 +26,7 @@ import { getMemoryService, getDefaultDbPath } from "../../memory-service"; import type { Project, AppSettings } from "../../../shared/types"; import { createContextLogger } from "./utils/logger"; import { withProjectOrNull } from "./utils/project-middleware"; -import { createIPCCommunicators } from "./utils/ipc-communicator"; +import { PRReviewStateManager } from "../../pr-review-state-manager"; import { getRunnerEnv } from "./utils/runner-env"; import { runPythonSubprocess, @@ -1384,7 +1384,7 @@ function sendReviewStateUpdate( project: Project, prNumber: number, projectId: string, - getMainWindow: () => BrowserWindow | null, + prReviewStateManager: PRReviewStateManager, context: string ): void { try { @@ -1393,18 +1393,8 @@ function sendReviewStateUpdate( debugLog("Could not retrieve updated review result for UI notification", { prNumber, context }); return; } - const mainWindow = getMainWindow(); - if (!mainWindow) return; - const { sendComplete } = createIPCCommunicators( - mainWindow, - { - progress: IPC_CHANNELS.GITHUB_PR_REVIEW_PROGRESS, - error: IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, - complete: IPC_CHANNELS.GITHUB_PR_REVIEW_COMPLETE, - }, - projectId - ); - sendComplete(updatedResult); + // Route through state manager so the XState actor emits the state change + prReviewStateManager.handleComplete(projectId, prNumber, updatedResult); debugLog(`Sent PR review state update ${context}`, { prNumber }); } catch (uiError) { debugLog("Failed to send UI update (non-critical)", { @@ -1445,7 +1435,8 @@ function getGitHubPRSettings(): { model: string; thinkingLevel: string } { async function runPRReview( project: Project, prNumber: number, - mainWindow: BrowserWindow + mainWindow: BrowserWindow, + prReviewStateManager: PRReviewStateManager ): Promise { // Comprehensive validation of GitHub module const validation = await validateGitHubModule(project); @@ -1456,15 +1447,9 @@ async function runPRReview( const backendPath = validation.backendPath!; - const { sendProgress } = createIPCCommunicators( - mainWindow, - { - progress: IPC_CHANNELS.GITHUB_PR_REVIEW_PROGRESS, - error: IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, - complete: IPC_CHANNELS.GITHUB_PR_REVIEW_COMPLETE, - }, - project.id - ); + const sendProgress = (progress: PRReviewProgress): void => { + prReviewStateManager.handleProgress(project.id, prNumber, progress); + }; const { model, thinkingLevel } = getGitHubPRSettings(); const args = buildRunnerArgs( @@ -1682,6 +1667,32 @@ async function fetchPRsFromGraphQL( export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): void { debugLog("Registering PR handlers"); + // Create the XState-based PR review state manager + const prReviewStateManager = new PRReviewStateManager(getMainWindow); + + // Clear all PR review actors when GitHub auth changes (account swap) + ipcMain.on(IPC_CHANNELS.GITHUB_AUTH_CHANGED, () => { + // Cancel all running review subprocesses and CI wait controllers + for (const [reviewKey, entry] of runningReviews) { + if (entry === CI_WAIT_PLACEHOLDER) { + const abortController = ciWaitAbortControllers.get(reviewKey); + if (abortController) { + abortController.abort(); + ciWaitAbortControllers.delete(reviewKey); + } + } else { + try { + entry.kill("SIGTERM"); + } catch { + // Process may have already exited + } + } + } + runningReviews.clear(); + ciWaitAbortControllers.clear(); + prReviewStateManager.handleAuthChange(); + }); + // List open PRs - fetches up to 100 open PRs at once, returns hasNextPage and endCursor from API ipcMain.handle( IPC_CHANNELS.GITHUB_PR_LIST, @@ -1889,31 +1900,27 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v try { await withProjectOrNull(projectId, async (project) => { - const { sendProgress, sendComplete } = createIPCCommunicators< - PRReviewProgress, - PRReviewResult - >( - mainWindow, - { - progress: IPC_CHANNELS.GITHUB_PR_REVIEW_PROGRESS, - error: IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, - complete: IPC_CHANNELS.GITHUB_PR_REVIEW_COMPLETE, - }, - projectId - ); + const sendProgress = (progress: PRReviewProgress): void => { + prReviewStateManager.handleProgress(projectId, prNumber, progress); + }; // Check if already running — notify renderer so it can display ongoing logs if (runningReviews.has(reviewKey)) { debugLog("Review already running, notifying renderer", { reviewKey }); + const currentSnapshot = prReviewStateManager.getState(projectId, prNumber); + const currentProgress = currentSnapshot?.context?.progress?.progress ?? 50; sendProgress({ phase: "analyzing", prNumber, - progress: 50, + progress: currentProgress, message: "Review is already in progress. Reconnecting to ongoing review...", }); return; } + // Notify state manager that review is starting (after duplicate check) + prReviewStateManager.handleStartReview(projectId, prNumber); + // Register as running BEFORE CI wait to prevent race conditions // Use CI_WAIT_PLACEHOLDER sentinel until real process is spawned runningReviews.set(reviewKey, CI_WAIT_PLACEHOLDER); @@ -1978,31 +1985,18 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v message: "Fetching PR data...", }); - const result = await runPRReview(project, prNumber, mainWindow); - - if (result.overallStatus === "in_progress") { - // Review is already running externally (detected by BotDetector). - // Send the result as-is so the renderer can activate external review polling. - debugLog("PR review already in progress externally", { prNumber }); - sendProgress({ - phase: "complete", - prNumber, - progress: 100, - message: "Review already in progress", - }); - sendComplete(result); - return; - } + const result = await runPRReview(project, prNumber, mainWindow, prReviewStateManager); debugLog("PR review completed", { prNumber, findingsCount: result.findings.length }); sendProgress({ phase: "complete", prNumber, progress: 100, - message: "Review complete!", + message: result.overallStatus === "in_progress" ? "Review already in progress" : "Review complete!", }); - sendComplete(result); + // Route through manager — handles external review detection internally + prReviewStateManager.handleComplete(projectId, prNumber, result); } finally { // Clean up in case we exit before runPRReview was called (e.g., cancelled during CI wait) // runPRReview also has its own cleanup, but delete is idempotent @@ -2019,16 +2013,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v prNumber, error: error instanceof Error ? error.message : error, }); - const { sendError } = createIPCCommunicators( - mainWindow, - { - progress: IPC_CHANNELS.GITHUB_PR_REVIEW_PROGRESS, - error: IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, - complete: IPC_CHANNELS.GITHUB_PR_REVIEW_COMPLETE, - }, - projectId - ); - sendError(error instanceof Error ? error.message : "Failed to run PR review"); + prReviewStateManager.handleError(projectId, prNumber, error instanceof Error ? error.message : "Failed to run PR review"); } }); @@ -2233,7 +2218,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v } // Send state update event to refresh UI immediately (non-blocking) - sendReviewStateUpdate(project, prNumber, projectId, getMainWindow, "after posting"); + sendReviewStateUpdate(project, prNumber, projectId, prReviewStateManager, "after posting"); return true; } catch (error) { @@ -2272,7 +2257,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v debugLog("Marked review as posted", { prNumber }); // Send state update event to refresh UI immediately (non-blocking) - sendReviewStateUpdate(project, prNumber, projectId, getMainWindow, "after marking posted"); + sendReviewStateUpdate(project, prNumber, projectId, prReviewStateManager, "after marking posted"); return true; } catch (error) { @@ -2390,7 +2375,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v } // Send state update event to refresh UI immediately (non-blocking) - sendReviewStateUpdate(project, prNumber, projectId, getMainWindow, "after deletion"); + sendReviewStateUpdate(project, prNumber, projectId, prReviewStateManager, "after deletion"); return true; } catch (error) { @@ -2502,6 +2487,8 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v ciWaitAbortControllers.delete(reviewKey); } runningReviews.delete(reviewKey); + // Notify state manager of cancellation + prReviewStateManager.handleCancel(projectId, prNumber); debugLog("CI wait cancelled", { reviewKey }); return true; } @@ -2522,6 +2509,8 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v // Clean up the registry runningReviews.delete(reviewKey); + // Notify state manager of cancellation + prReviewStateManager.handleCancel(projectId, prNumber); debugLog("Review process cancelled", { reviewKey }); return true; } catch (error) { @@ -2534,6 +2523,21 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v } ); + // Notify main process about external review completion or timeout + // Called by renderer when its polling detects an external review has finished on disk + ipcMain.handle( + IPC_CHANNELS.GITHUB_PR_NOTIFY_EXTERNAL_REVIEW_COMPLETE, + async (_, projectId: string, prNumber: number, result: PRReviewResult | null): Promise => { + debugLog("notifyExternalReviewComplete handler called", { projectId, prNumber, hasResult: !!result }); + if (result) { + prReviewStateManager.handleComplete(projectId, prNumber, result); + } else { + // Timeout — no result found within polling window + prReviewStateManager.handleError(projectId, prNumber, "External review timed out after 30 minutes"); + } + } + ); + // Check for new commits since last review ipcMain.handle( IPC_CHANNELS.GITHUB_PR_CHECK_NEW_COMMITS, @@ -2919,34 +2923,40 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v try { await withProjectOrNull(projectId, async (project) => { - const { sendProgress, sendError, sendComplete } = createIPCCommunicators< - PRReviewProgress, - PRReviewResult - >( - mainWindow, - { - progress: IPC_CHANNELS.GITHUB_PR_REVIEW_PROGRESS, - error: IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, - complete: IPC_CHANNELS.GITHUB_PR_REVIEW_COMPLETE, - }, - projectId - ); + const sendProgress = (progress: PRReviewProgress): void => { + prReviewStateManager.handleProgress(projectId, prNumber, progress); + }; + + const reviewKey = getReviewKey(projectId, prNumber); + + // Check if already running — notify renderer so it can display ongoing logs + if (runningReviews.has(reviewKey)) { + debugLog("Follow-up review already running, notifying renderer", { reviewKey }); + const currentSnapshot = prReviewStateManager.getState(projectId, prNumber); + const currentProgress = currentSnapshot?.context?.progress?.progress ?? 50; + sendProgress({ + phase: "analyzing", + prNumber, + progress: currentProgress, + message: "Follow-up review is already in progress. Reconnecting to ongoing review...", + }); + return; + } + + // Get previous result for followup context + const previousResult = getReviewResult(project, prNumber) ?? undefined; + + // Notify state manager that followup review is starting (after duplicate check) + prReviewStateManager.handleStartFollowupReview(projectId, prNumber, previousResult); // Comprehensive validation of GitHub module const validation = await validateGitHubModule(project); if (!validation.valid) { - sendError({ prNumber, error: validation.error || "GitHub module validation failed" }); + prReviewStateManager.handleError(projectId, prNumber, validation.error || "GitHub module validation failed"); return; } const backendPath = validation.backendPath!; - const reviewKey = getReviewKey(projectId, prNumber); - - // Check if already running - if (runningReviews.has(reviewKey)) { - debugLog("Follow-up review already running", { reviewKey }); - return; - } // Register as running BEFORE CI wait to prevent race conditions // Use CI_WAIT_PLACEHOLDER sentinel until real process is spawned @@ -3103,7 +3113,8 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v message: "Follow-up review complete!", }); - sendComplete(result.data!); + // Route through state manager + prReviewStateManager.handleComplete(projectId, prNumber, result.data!); } finally { // Always clean up registry, whether we exit normally or via error runningReviews.delete(reviewKey); @@ -3116,19 +3127,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v prNumber, error: error instanceof Error ? error.message : error, }); - const { sendError } = createIPCCommunicators( - mainWindow, - { - progress: IPC_CHANNELS.GITHUB_PR_REVIEW_PROGRESS, - error: IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, - complete: IPC_CHANNELS.GITHUB_PR_REVIEW_COMPLETE, - }, - projectId - ); - sendError({ - prNumber, - error: error instanceof Error ? error.message : "Failed to run follow-up review", - }); + prReviewStateManager.handleError(projectId, prNumber, error instanceof Error ? error.message : "Failed to run follow-up review"); } } ); diff --git a/apps/frontend/src/main/pr-review-state-manager.ts b/apps/frontend/src/main/pr-review-state-manager.ts new file mode 100644 index 0000000000..78a9c688b9 --- /dev/null +++ b/apps/frontend/src/main/pr-review-state-manager.ts @@ -0,0 +1,217 @@ +import { createActor } from 'xstate'; +import type { ActorRefFrom } from 'xstate'; +import type { BrowserWindow } from 'electron'; +import { prReviewMachine, type PRReviewEvent, type PRReviewContext } from '../shared/state-machines'; +import type { PRReviewProgress, PRReviewResult, PRReviewStatePayload } from '../preload/api/modules/github-api'; +import { IPC_CHANNELS } from '../shared/constants'; +import { safeSendToRenderer } from './ipc-handlers/utils'; + +type PRReviewActor = ActorRefFrom; + +/** + * Build a deduplication key from snapshot state + relevant context fields. + * PR reviews need to emit even when state stays the same but context changes + * (e.g., progress updates within 'reviewing' state). + */ +function buildContextKey(snapshot: { context: PRReviewContext }): string { + const ctx = snapshot.context; + const progressKey = ctx.progress + ? `${ctx.progress.phase}:${ctx.progress.progress}:${ctx.progress.message}` + : 'none'; + const resultKey = ctx.result ? ctx.result.overallStatus : 'none'; + const errorKey = ctx.error ?? 'none'; + return `${progressKey}|${resultKey}|${errorKey}`; +} + +export class PRReviewStateManager { + private actors = new Map(); + private lastStateByPR = new Map(); + private getMainWindow: () => BrowserWindow | null; + + constructor(getMainWindow: () => BrowserWindow | null) { + this.getMainWindow = getMainWindow; + } + + handleStartReview(projectId: string, prNumber: number): void { + const actor = this.getOrCreateActor(projectId, prNumber); + actor.send({ type: 'START_REVIEW', prNumber, projectId } satisfies PRReviewEvent); + } + + handleStartFollowupReview(projectId: string, prNumber: number, previousResult?: PRReviewResult): void { + const actor = this.getOrCreateActor(projectId, prNumber); + if (previousResult) { + actor.send({ type: 'START_FOLLOWUP_REVIEW', prNumber, projectId, previousResult } satisfies PRReviewEvent); + } else { + actor.send({ type: 'START_REVIEW', prNumber, projectId } satisfies PRReviewEvent); + } + } + + handleProgress(projectId: string, prNumber: number, progress: PRReviewProgress): void { + const actor = this.getActor(projectId, prNumber); + if (!actor) return; + actor.send({ type: 'SET_PROGRESS', progress } satisfies PRReviewEvent); + } + + handleComplete(projectId: string, prNumber: number, result: PRReviewResult): void { + // Use getOrCreateActor so late-arriving results (e.g. after auth change or + // app restart) still get processed instead of silently dropped. + const actor = this.getOrCreateActor(projectId, prNumber); + + // If the actor is in idle state (freshly created for a late-arriving result), + // transition to reviewing first so REVIEW_COMPLETE is accepted. + const snapshot = actor.getSnapshot(); + if (String(snapshot.value) === 'idle') { + actor.send({ type: 'START_REVIEW', prNumber, projectId } satisfies PRReviewEvent); + } + + // Detect external review (result arrives with 'in_progress' status from outside) + if (result.overallStatus === 'in_progress') { + actor.send({ type: 'DETECT_EXTERNAL_REVIEW' } satisfies PRReviewEvent); + } else { + actor.send({ type: 'REVIEW_COMPLETE', result } satisfies PRReviewEvent); + } + } + + handleError(projectId: string, prNumber: number, error: string): void { + const actor = this.getActor(projectId, prNumber); + if (!actor) return; + actor.send({ type: 'REVIEW_ERROR', error } satisfies PRReviewEvent); + } + + handleCancel(projectId: string, prNumber: number): void { + const actor = this.getActor(projectId, prNumber); + if (!actor) return; + actor.send({ type: 'CANCEL_REVIEW' } satisfies PRReviewEvent); + } + + handleClearReview(projectId: string, prNumber: number): void { + const key = this.getKey(projectId, prNumber); + const actor = this.actors.get(key); + if (actor) { + // Capture snapshot before stopping so the emitted payload has real context. + // Don't send CLEAR_REVIEW to the actor — that would trigger the subscription + // and cause a duplicate IPC emission alongside emitClearedState below. + const snapshot = actor.getSnapshot(); + actor.stop(); + this.actors.delete(key); + this.emitClearedState(key, snapshot?.context ?? null); + } + this.lastStateByPR.delete(key); + } + + handleAuthChange(): void { + for (const [key, actor] of this.actors) { + // Capture the last known snapshot before stopping so the emitted payload + // contains the real projectId/prNumber instead of zeros. + const snapshot = actor.getSnapshot(); + actor.stop(); + // Emit cleared (idle) state to renderer for each PR + this.emitClearedState(key, snapshot?.context ?? null); + } + this.actors.clear(); + this.lastStateByPR.clear(); + } + + getState(projectId: string, prNumber: number): ReturnType | null { + const actor = this.getActor(projectId, prNumber); + if (!actor) return null; + return actor.getSnapshot(); + } + + clearAll(): void { + for (const [, actor] of this.actors) { + actor.stop(); + } + this.actors.clear(); + this.lastStateByPR.clear(); + } + + // --------------------------------------------------------------------------- + // Private + // --------------------------------------------------------------------------- + + private getOrCreateActor(projectId: string, prNumber: number): PRReviewActor { + const key = this.getKey(projectId, prNumber); + const existing = this.actors.get(key); + if (existing) return existing; + + const actor = createActor(prReviewMachine); + + actor.subscribe((snapshot) => { + const stateValue = String(snapshot.value); + const contextKey = buildContextKey(snapshot); + const currentKey = `${stateValue}:${contextKey}`; + if (this.lastStateByPR.get(key) === currentKey) return; + this.lastStateByPR.set(key, currentKey); + this.emitStateToRenderer(key, snapshot); + }); + + actor.start(); + this.actors.set(key, actor); + return actor; + } + + private getActor(projectId: string, prNumber: number): PRReviewActor | null { + return this.actors.get(this.getKey(projectId, prNumber)) ?? null; + } + + private getKey(projectId: string, prNumber: number): string { + return `${projectId}:${prNumber}`; + } + + private emitStateToRenderer( + key: string, + snapshot: ReturnType | null + ): void { + const stateValue = snapshot ? String(snapshot.value) : 'idle'; + const ctx = snapshot?.context ?? null; + + const payload: PRReviewStatePayload = { + state: stateValue, + prNumber: ctx?.prNumber ?? 0, + projectId: ctx?.projectId ?? '', + isReviewing: stateValue === 'reviewing' || stateValue === 'externalReview', + startedAt: ctx?.startedAt ?? null, + progress: ctx?.progress ?? null, + result: ctx?.result ?? null, + previousResult: ctx?.previousResult ?? null, + error: ctx?.error ?? null, + isExternalReview: ctx?.isExternalReview ?? false, + isFollowup: ctx?.isFollowup ?? false, + }; + + safeSendToRenderer( + this.getMainWindow, + IPC_CHANNELS.GITHUB_PR_REVIEW_STATE_CHANGE, + key, + payload + ); + } + + /** + * Emit a cleared (idle) state using context from the last snapshot + * so the payload contains the real projectId/prNumber. + */ + private emitClearedState(key: string, ctx: PRReviewContext | null): void { + const payload: PRReviewStatePayload = { + state: 'idle', + prNumber: ctx?.prNumber ?? 0, + projectId: ctx?.projectId ?? '', + isReviewing: false, + startedAt: null, + progress: null, + result: null, + previousResult: null, + error: null, + isExternalReview: false, + isFollowup: false, + }; + + safeSendToRenderer( + this.getMainWindow, + IPC_CHANNELS.GITHUB_PR_REVIEW_STATE_CHANGE, + key, + payload + ); + } +} diff --git a/apps/frontend/src/preload/api/modules/github-api.ts b/apps/frontend/src/preload/api/modules/github-api.ts index bca2a8f129..c2115eb110 100644 --- a/apps/frontend/src/preload/api/modules/github-api.ts +++ b/apps/frontend/src/preload/api/modules/github-api.ts @@ -285,6 +285,9 @@ export interface GitHubAPI { getPRReview: (projectId: string, prNumber: number) => Promise; getPRReviewsBatch: (projectId: string, prNumbers: number[]) => Promise>; + // External review notification (renderer tells main process about external review completion/timeout) + notifyExternalReviewComplete: (projectId: string, prNumber: number, result: PRReviewResult | null) => Promise; + // Follow-up review operations checkNewCommits: (projectId: string, prNumber: number) => Promise; checkMergeReadiness: (projectId: string, prNumber: number) => Promise; @@ -308,6 +311,9 @@ export interface GitHubAPI { onPRReviewError: ( callback: (projectId: string, error: { prNumber: number; error: string }) => void ) => IpcListenerCleanup; + onPRReviewStateChange: ( + callback: (key: string, state: PRReviewStatePayload) => void + ) => IpcListenerCleanup; onPRLogsUpdated: ( callback: (projectId: string, data: { prNumber: number; entryCount: number }) => void ) => IpcListenerCleanup; @@ -456,6 +462,23 @@ export interface PRReviewProgress { message: string; } +/** + * PR review state payload (emitted on state machine transitions) + */ +export interface PRReviewStatePayload { + state: string; + prNumber: number; + projectId: string; + isReviewing: boolean; + startedAt: string | null; + progress: PRReviewProgress | null; + result: PRReviewResult | null; + previousResult: PRReviewResult | null; + error: string | null; + isExternalReview: boolean; + isFollowup: boolean; +} + /** * PR review log entry type */ @@ -740,6 +763,10 @@ export const createGitHubAPI = (): GitHubAPI => ({ getPRReviewsBatch: (projectId: string, prNumbers: number[]): Promise> => invokeIpc(IPC_CHANNELS.GITHUB_PR_GET_REVIEWS_BATCH, projectId, prNumbers), + // External review notification + notifyExternalReviewComplete: (projectId: string, prNumber: number, result: PRReviewResult | null): Promise => + invokeIpc(IPC_CHANNELS.GITHUB_PR_NOTIFY_EXTERNAL_REVIEW_COMPLETE, projectId, prNumber, result), + // Follow-up review operations checkNewCommits: (projectId: string, prNumber: number): Promise => invokeIpc(IPC_CHANNELS.GITHUB_PR_CHECK_NEW_COMMITS, projectId, prNumber), @@ -780,6 +807,11 @@ export const createGitHubAPI = (): GitHubAPI => ({ ): IpcListenerCleanup => createIpcListener(IPC_CHANNELS.GITHUB_PR_REVIEW_ERROR, callback), + onPRReviewStateChange: ( + callback: (key: string, state: PRReviewStatePayload) => void + ): IpcListenerCleanup => + createIpcListener(IPC_CHANNELS.GITHUB_PR_REVIEW_STATE_CHANGE, callback), + onPRLogsUpdated: ( callback: (projectId: string, data: { prNumber: number; entryCount: number }) => void ): IpcListenerCleanup => diff --git a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx index 3098ba6fa7..655c6e77ae 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx @@ -417,19 +417,22 @@ export function PRDetail({ const MAX_POLL_DURATION_MS = 30 * 60 * 1000; // 30 minutes const pollStart = Date.now(); + let notified = false; + const pollForCompletion = async () => { + // Skip if we already notified (prevents duplicate notifications before React cleanup) + if (notified) return; + // Timeout: stop polling after 30 minutes to avoid indefinite polling if (Date.now() - pollStart > MAX_POLL_DURATION_MS) { - usePRReviewStore.getState().setPRReviewResult(projectId, { - prNumber: pr.number, - repo: '', - success: false, - findings: [], - summary: '', - overallStatus: 'comment', - reviewedAt: new Date().toISOString(), - error: 'External review polling timed out after 30 minutes', - }); + console.warn('[PRDetail] External review polling timed out after 30 minutes'); + notified = true; + try { + // Notify main process so the XState actor transitions to error state + await window.electronAPI.github.notifyExternalReviewComplete(projectId, pr.number, null); + } catch { + // Non-critical — state manager timeout is a best-effort notification + } return; } @@ -440,7 +443,9 @@ export function PRDetail({ // Otherwise this is a stale result from a previous review still on disk // (in-progress results are intentionally NOT saved to disk). if (startedAt && result.reviewedAt && new Date(result.reviewedAt) > new Date(startedAt)) { - usePRReviewStore.getState().setPRReviewResult(projectId, result); + notified = true; + // Notify main process so the XState actor transitions to completed state + await window.electronAPI.github.notifyExternalReviewComplete(projectId, pr.number, result); } } } catch { diff --git a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts index bdaca8bd7c..9ed5764422 100644 --- a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts +++ b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts @@ -7,8 +7,6 @@ import type { } from "../../../../preload/api/modules/github-api"; import { usePRReviewStore, - startPRReview as storeStartPRReview, - startFollowupReview as storeStartFollowupReview, } from "../../../stores/github"; // Re-export types for consumers @@ -204,7 +202,7 @@ export function useGitHubPRs( // Update store with loaded results for (const reviewResult of Object.values(batchReviews)) { if (reviewResult) { - usePRReviewStore.getState().setPRReviewResult(projectId, reviewResult, { + usePRReviewStore.getState().setLoadedReviewResult(projectId, reviewResult, { preserveNewCommitsCheck: true, }); } @@ -422,7 +420,7 @@ export function useGitHubPRs( // Preserve newCommitsCheck when loading existing review from disk usePRReviewStore .getState() - .setPRReviewResult(projectId, result, { preserveNewCommitsCheck: true }); + .setLoadedReviewResult(projectId, result, { preserveNewCommitsCheck: true }); // Always check for new commits when selecting a reviewed PR // This ensures fresh data even if we have a cached check from earlier in the session @@ -510,7 +508,7 @@ export function useGitHubPRs( // Update store with loaded results for (const reviewResult of Object.values(batchReviews)) { if (reviewResult) { - usePRReviewStore.getState().setPRReviewResult(requestProjectId, reviewResult, { + usePRReviewStore.getState().setLoadedReviewResult(requestProjectId, reviewResult, { preserveNewCommitsCheck: true, }); } @@ -534,8 +532,8 @@ export function useGitHubPRs( (prNumber: number) => { if (!projectId) return; - // Use the store function which handles both state and IPC - storeStartPRReview(projectId, prNumber); + // Main process handles XState state transition and subprocess launch + window.electronAPI.github.runPRReview(projectId, prNumber); }, [projectId] ); @@ -544,8 +542,8 @@ export function useGitHubPRs( (prNumber: number) => { if (!projectId) return; - // Use the store function which handles both state and IPC - storeStartFollowupReview(projectId, prNumber); + // Main process handles XState state transition and subprocess launch + window.electronAPI.github.runFollowupReview(projectId, prNumber); }, [projectId] ); @@ -575,15 +573,9 @@ export function useGitHubPRs( if (!projectId) return false; try { + // Main process kills subprocess and sends CANCEL_REVIEW to XState + // State update flows back via IPC (onPRReviewStateChange) const success = await window.electronAPI.github.cancelPRReview(projectId, prNumber); - // Always update store state to exit the "reviewing" state - // Use different messages based on whether the process was found and killed - const message = success - ? "Review cancelled by user" - : "Review stopped - process not found"; - usePRReviewStore - .getState() - .setPRReviewError(projectId, prNumber, message); return success; } catch (err) { setError(err instanceof Error ? err.message : "Failed to cancel review"); @@ -615,7 +607,7 @@ export function useGitHubPRs( // Preserve newCommitsCheck - posting doesn't change whether there are new commits usePRReviewStore .getState() - .setPRReviewResult(projectId, result, { preserveNewCommitsCheck: true }); + .setLoadedReviewResult(projectId, result, { preserveNewCommitsCheck: true }); } } return success; @@ -697,7 +689,7 @@ export function useGitHubPRs( const existingState = getPRReviewState(projectId, prNumber); if (existingState?.result) { // If we have the result loaded, update it with hasPostedFindings and postedAt - usePRReviewStore.getState().setPRReviewResult( + usePRReviewStore.getState().setLoadedReviewResult( projectId, { ...existingState.result, hasPostedFindings: true, postedAt }, { preserveNewCommitsCheck: true } @@ -706,7 +698,7 @@ export function useGitHubPRs( // If result not loaded yet (race condition), reload from disk to get updated state const result = await window.electronAPI.github.getPRReview(projectId, prNumber); if (result) { - usePRReviewStore.getState().setPRReviewResult( + usePRReviewStore.getState().setLoadedReviewResult( projectId, result, { preserveNewCommitsCheck: true } diff --git a/apps/frontend/src/renderer/lib/browser-mock.ts b/apps/frontend/src/renderer/lib/browser-mock.ts index e933054e0c..d33756c9a4 100644 --- a/apps/frontend/src/renderer/lib/browser-mock.ts +++ b/apps/frontend/src/renderer/lib/browser-mock.ts @@ -214,6 +214,7 @@ const browserMockAPI: ElectronAPI = { markReviewPosted: async () => true, getPRReview: async () => null, getPRReviewsBatch: async () => ({}), + notifyExternalReviewComplete: async () => {}, deletePRReview: async () => true, checkNewCommits: async () => ({ hasNewCommits: false, newCommitCount: 0 }), checkMergeReadiness: async () => ({ isDraft: false, mergeable: 'UNKNOWN' as const, isBehind: false, ciStatus: 'none' as const, blockers: [] }), @@ -225,6 +226,7 @@ const browserMockAPI: ElectronAPI = { onPRReviewProgress: () => () => {}, onPRReviewComplete: () => () => {}, onPRReviewError: () => () => {}, + onPRReviewStateChange: () => () => {}, onPRLogsUpdated: () => () => {}, batchAutoFix: () => {}, getBatches: async () => [], diff --git a/apps/frontend/src/renderer/stores/github/index.ts b/apps/frontend/src/renderer/stores/github/index.ts index c20fa48b7f..ff241439d3 100644 --- a/apps/frontend/src/renderer/stores/github/index.ts +++ b/apps/frontend/src/renderer/stores/github/index.ts @@ -23,9 +23,7 @@ export { export { usePRReviewStore, initializePRReviewListeners, - cleanupPRReviewListeners, - startPRReview, - startFollowupReview + cleanupPRReviewListeners } from './pr-review-store'; import { initializePRReviewListeners as _initPRReviewListeners } from './pr-review-store'; import { cleanupPRReviewListeners as _cleanupPRReviewListeners } from './pr-review-store'; diff --git a/apps/frontend/src/renderer/stores/github/pr-review-store.ts b/apps/frontend/src/renderer/stores/github/pr-review-store.ts index b67ce18e7e..b790ee24f6 100644 --- a/apps/frontend/src/renderer/stores/github/pr-review-store.ts +++ b/apps/frontend/src/renderer/stores/github/pr-review-store.ts @@ -2,7 +2,8 @@ import { create } from 'zustand'; import type { PRReviewProgress, PRReviewResult, - NewCommitsCheck + NewCommitsCheck, + PRReviewStatePayload } from '../../../preload/api/modules/github-api'; import type { ChecksStatus, @@ -44,14 +45,13 @@ interface PRReviewStoreState { // Key: `${projectId}:${prNumber}` prReviews: Record; - // Actions - startPRReview: (projectId: string, prNumber: number) => void; - startFollowupReview: (projectId: string, prNumber: number) => void; - setPRReviewProgress: (projectId: string, progress: PRReviewProgress) => void; - setPRReviewResult: (projectId: string, result: PRReviewResult, options?: { preserveNewCommitsCheck?: boolean }) => void; - setPRReviewError: (projectId: string, prNumber: number, error: string) => void; + // XState state change handler + handlePRReviewStateChange: (key: string, payload: PRReviewStatePayload) => void; + + // Kept actions (not managed by XState) + /** Load a review result from disk into the store (not triggered by XState) */ + setLoadedReviewResult: (projectId: string, result: PRReviewResult, options?: { preserveNewCommitsCheck?: boolean }) => void; setNewCommitsCheck: (projectId: string, prNumber: number, check: NewCommitsCheck) => void; - clearPRReview: (projectId: string, prNumber: number) => void; /** Update PR status from polling (CI checks, reviews, mergeability) */ setPRStatus: (projectId: string, prNumber: number, status: { checksStatus: ChecksStatus; @@ -61,8 +61,6 @@ interface PRReviewStoreState { }) => void; /** Clear PR status fields for a specific PR */ clearPRStatus: (projectId: string, prNumber: number) => void; - /** Start an external review (from PR list) - sets isReviewing and isExternalReview */ - setExternalReviewInProgress: (projectId: string, prNumber: number, inProgressSince?: string) => void; // Selectors getPRReviewState: (projectId: string, prNumber: number) => PRReviewState | null; @@ -80,97 +78,57 @@ export const usePRReviewStore = create((set, get) => ({ // Initial state prReviews: {}, - // Actions - startPRReview: (projectId: string, prNumber: number) => set((state) => { - const key = `${projectId}:${prNumber}`; - const existing = state.prReviews[key]; - return { - prReviews: { - ...state.prReviews, - [key]: { - prNumber, - projectId, - isReviewing: true, - startedAt: new Date().toISOString(), - progress: null, - result: null, - previousResult: null, - error: null, - newCommitsCheck: existing?.newCommitsCheck ?? null, - checksStatus: existing?.checksStatus ?? null, - reviewsStatus: existing?.reviewsStatus ?? null, - mergeableState: existing?.mergeableState ?? null, - lastPolled: existing?.lastPolled ?? null, - isExternalReview: false - } - } - }; - }), + // XState state change handler — maps XState state/context back to PRReviewState shape + handlePRReviewStateChange: (key: string, payload: PRReviewStatePayload) => { + const isCompleted = payload.state === 'completed'; + + set((state) => { + const existing = state.prReviews[key]; + + const updated: PRReviewState = { + prNumber: payload.prNumber, + projectId: payload.projectId, + isReviewing: payload.state === 'reviewing' || payload.state === 'externalReview', + startedAt: payload.startedAt, + progress: payload.progress, + result: payload.result, + previousResult: payload.previousResult, + error: payload.error, + isExternalReview: payload.isExternalReview, + // Preserve polling data — not managed by XState + checksStatus: existing?.checksStatus ?? null, + reviewsStatus: existing?.reviewsStatus ?? null, + mergeableState: existing?.mergeableState ?? null, + lastPolled: existing?.lastPolled ?? null, + // Preserve newCommitsCheck unless review completed (it was just reviewed) + newCommitsCheck: isCompleted ? null : (existing?.newCommitsCheck ?? null), + }; - startFollowupReview: (projectId: string, prNumber: number) => set((state) => { - const key = `${projectId}:${prNumber}`; - const existing = state.prReviews[key]; + return { + prReviews: { + ...state.prReviews, + [key]: updated, + }, + }; + }); - // Log warning if starting follow-up without a previous result - if (!existing?.result) { - console.warn( - `[PRReviewStore] Starting follow-up review for PR #${prNumber} without a previous result. ` + - `This may indicate the follow-up was triggered incorrectly.` - ); + // Trigger registered refresh callbacks when review completes + if (isCompleted) { + refreshCallbacks.forEach(callback => { + Promise.resolve(callback()).catch(error => { + console.error('[PRReviewStore] Error in refresh callback:', error); + }); + }); } + }, - return { - prReviews: { - ...state.prReviews, - [key]: { - prNumber, - projectId, - isReviewing: true, - startedAt: new Date().toISOString(), - progress: null, - result: null, - previousResult: existing?.result ?? null, // Preserve for follow-up continuity - error: null, - newCommitsCheck: existing?.newCommitsCheck ?? null, - checksStatus: existing?.checksStatus ?? null, - reviewsStatus: existing?.reviewsStatus ?? null, - mergeableState: existing?.mergeableState ?? null, - lastPolled: existing?.lastPolled ?? null, - isExternalReview: false - } - } - }; - }), - - setPRReviewProgress: (projectId: string, progress: PRReviewProgress) => set((state) => { - const key = `${projectId}:${progress.prNumber}`; - const existing = state.prReviews[key]; - return { - prReviews: { - ...state.prReviews, - [key]: { - prNumber: progress.prNumber, - projectId, - isReviewing: true, - startedAt: existing?.startedAt ?? null, - progress, - result: existing?.result ?? null, - previousResult: existing?.previousResult ?? null, - error: null, - newCommitsCheck: existing?.newCommitsCheck ?? null, - checksStatus: existing?.checksStatus ?? null, - reviewsStatus: existing?.reviewsStatus ?? null, - mergeableState: existing?.mergeableState ?? null, - lastPolled: existing?.lastPolled ?? null, - isExternalReview: existing?.isExternalReview ?? false - } - } - }; - }), - - setPRReviewResult: (projectId: string, result: PRReviewResult, options?: { preserveNewCommitsCheck?: boolean }) => set((state) => { + setLoadedReviewResult: (projectId: string, result: PRReviewResult, options?: { preserveNewCommitsCheck?: boolean }) => set((state) => { const key = `${projectId}:${result.prNumber}`; const existing = state.prReviews[key]; + // Don't overwrite active review state from XState + if (existing?.isReviewing) { + return state; + } return { prReviews: { ...state.prReviews, @@ -178,47 +136,19 @@ export const usePRReviewStore = create((set, get) => ({ prNumber: result.prNumber, projectId, isReviewing: false, - startedAt: existing?.startedAt ?? null, + startedAt: null, progress: null, result, previousResult: existing?.previousResult ?? null, - error: result.error ?? null, - // Clear new commits check when review completes (it was just reviewed) - // BUT preserve it during preload/refresh to avoid race condition + error: null, newCommitsCheck: options?.preserveNewCommitsCheck ? (existing?.newCommitsCheck ?? null) : null, checksStatus: existing?.checksStatus ?? null, reviewsStatus: existing?.reviewsStatus ?? null, mergeableState: existing?.mergeableState ?? null, lastPolled: existing?.lastPolled ?? null, - isExternalReview: existing?.isExternalReview ?? false - } - } - }; - }), - - setPRReviewError: (projectId: string, prNumber: number, error: string) => set((state) => { - const key = `${projectId}:${prNumber}`; - const existing = state.prReviews[key]; - return { - prReviews: { - ...state.prReviews, - [key]: { - prNumber, - projectId, - isReviewing: false, - startedAt: existing?.startedAt ?? null, - progress: null, - result: existing?.result ?? null, - previousResult: existing?.previousResult ?? null, - error, - newCommitsCheck: existing?.newCommitsCheck ?? null, - checksStatus: existing?.checksStatus ?? null, - reviewsStatus: existing?.reviewsStatus ?? null, - mergeableState: existing?.mergeableState ?? null, - lastPolled: existing?.lastPolled ?? null, - isExternalReview: existing?.isExternalReview ?? false - } - } + isExternalReview: false, + }, + }, }; }), @@ -260,11 +190,6 @@ export const usePRReviewStore = create((set, get) => ({ }; }), - clearPRReview: (projectId: string, prNumber: number) => set((state) => { - const key = `${projectId}:${prNumber}`; - const { [key]: _, ...rest } = state.prReviews; - return { prReviews: rest }; - }), setPRStatus: (projectId: string, prNumber: number, status: { checksStatus: ChecksStatus; @@ -332,32 +257,6 @@ export const usePRReviewStore = create((set, get) => ({ }; }), - setExternalReviewInProgress: (projectId: string, prNumber: number, inProgressSince?: string) => set((state) => { - const key = `${projectId}:${prNumber}`; - const existing = state.prReviews[key]; - return { - prReviews: { - ...state.prReviews, - [key]: { - prNumber, - projectId, - isReviewing: true, - startedAt: inProgressSince || new Date().toISOString(), - progress: null, - result: existing?.result ?? null, - previousResult: existing?.previousResult ?? null, - error: null, - newCommitsCheck: existing?.newCommitsCheck ?? null, - checksStatus: existing?.checksStatus ?? null, - reviewsStatus: existing?.reviewsStatus ?? null, - mergeableState: existing?.mergeableState ?? null, - lastPolled: existing?.lastPolled ?? null, - isExternalReview: true - } - } - }; - }), - // Selectors getPRReviewState: (projectId: string, prNumber: number) => { const { prReviews } = get(); @@ -398,50 +297,18 @@ export function initializePRReviewListeners(): void { const store = usePRReviewStore.getState(); // Check if GitHub PR Review API is available - if (!window.electronAPI?.github?.onPRReviewProgress) { + if (!window.electronAPI?.github?.onPRReviewStateChange) { console.warn('[GitHub PR Store] GitHub PR Review API not available, skipping listener setup'); return; } - // Listen for PR review progress events - // Each on* method returns a cleanup function — capture them for proper teardown - const cleanupProgress = window.electronAPI.github.onPRReviewProgress( - (projectId: string, progress: PRReviewProgress) => { - store.setPRReviewProgress(projectId, progress); + // Listen for XState state changes — single handler replaces progress/complete/error listeners + const cleanupStateChange = window.electronAPI.github.onPRReviewStateChange( + (key: string, payload: PRReviewStatePayload) => { + store.handlePRReviewStateChange(key, payload); } ); - cleanupFunctions.push(cleanupProgress); - - // Listen for PR review completion events - const cleanupComplete = window.electronAPI.github.onPRReviewComplete( - (projectId: string, result: PRReviewResult) => { - // When the backend detects an already-running review (e.g., started from another - // client or the PR list), it returns overallStatus === 'in_progress' instead of - // a real result. Transition to external-review-in-progress so the log polling - // activates and the UI shows the ongoing review. - if (result.overallStatus === 'in_progress') { - store.setExternalReviewInProgress(projectId, result.prNumber, result.inProgressSince); - return; - } - - store.setPRReviewResult(projectId, result); - // Trigger all registered refresh callbacks when review completes - refreshCallbacks.forEach(callback => { - Promise.resolve(callback()).catch(error => { - console.error('[PRReviewStore] Error in refresh callback:', error); - }); - }); - } - ); - cleanupFunctions.push(cleanupComplete); - - // Listen for PR review error events - const cleanupError = window.electronAPI.github.onPRReviewError( - (projectId: string, data: { prNumber: number; error: string }) => { - store.setPRReviewError(projectId, data.prNumber, data.error); - } - ); - cleanupFunctions.push(cleanupError); + cleanupFunctions.push(cleanupStateChange); // Listen for GitHub auth changes - clear all PR review state when account changes const cleanupAuthChanged = window.electronAPI.github.onGitHubAuthChanged( @@ -457,7 +324,7 @@ export function initializePRReviewListeners(): void { cleanupFunctions.push(cleanupAuthChanged); // Listen for PR status polling updates (CI checks, reviews, mergeability) - window.electronAPI.github.onPRStatusUpdate( + const cleanupStatusUpdate = window.electronAPI.github.onPRStatusUpdate( (update: PRStatusUpdate) => { const { projectId, statuses } = update; for (const status of statuses) { @@ -470,6 +337,7 @@ export function initializePRReviewListeners(): void { } } ); + cleanupFunctions.push(cleanupStatusUpdate); prReviewListenersInitialized = true; } @@ -490,22 +358,3 @@ export function cleanupPRReviewListeners(): void { refreshCallbacks.clear(); prReviewListenersInitialized = false; } - -/** - * Start a PR review and track it in the store - */ -export function startPRReview(projectId: string, prNumber: number): void { - const store = usePRReviewStore.getState(); - store.startPRReview(projectId, prNumber); - window.electronAPI.github.runPRReview(projectId, prNumber); -} - -/** - * Start a follow-up PR review and track it in the store - * Uses startFollowupReview action to preserve previous result for continuity - */ -export function startFollowupReview(projectId: string, prNumber: number): void { - const store = usePRReviewStore.getState(); - store.startFollowupReview(projectId, prNumber); - window.electronAPI.github.runFollowupReview(projectId, prNumber); -} diff --git a/apps/frontend/src/shared/constants/ipc.ts b/apps/frontend/src/shared/constants/ipc.ts index 4c62860856..65bc51a479 100644 --- a/apps/frontend/src/shared/constants/ipc.ts +++ b/apps/frontend/src/shared/constants/ipc.ts @@ -408,11 +408,13 @@ export const IPC_CHANNELS = { GITHUB_PR_CHECK_MERGE_READINESS: 'github:pr:checkMergeReadiness', GITHUB_PR_MARK_REVIEW_POSTED: 'github:pr:markReviewPosted', GITHUB_PR_UPDATE_BRANCH: 'github:pr:updateBranch', + GITHUB_PR_NOTIFY_EXTERNAL_REVIEW_COMPLETE: 'github:pr:notifyExternalReviewComplete', // GitHub PR Review events (main -> renderer) GITHUB_PR_REVIEW_PROGRESS: 'github:pr:reviewProgress', GITHUB_PR_REVIEW_COMPLETE: 'github:pr:reviewComplete', GITHUB_PR_REVIEW_ERROR: 'github:pr:reviewError', + GITHUB_PR_REVIEW_STATE_CHANGE: 'github:pr:reviewStateChange', GITHUB_PR_LOGS_UPDATED: 'github:pr:logsUpdated', // GitHub PR Logs (for viewing AI review logs) diff --git a/apps/frontend/src/shared/state-machines/__tests__/pr-review-machine.test.ts b/apps/frontend/src/shared/state-machines/__tests__/pr-review-machine.test.ts new file mode 100644 index 0000000000..f5b5319d0e --- /dev/null +++ b/apps/frontend/src/shared/state-machines/__tests__/pr-review-machine.test.ts @@ -0,0 +1,352 @@ +import { describe, it, expect } from 'vitest'; +import { createActor } from 'xstate'; +import { prReviewMachine, type PRReviewEvent } from '../pr-review-machine'; + +/** + * Helper to run a sequence of events and get the final state + */ +function runEvents(events: PRReviewEvent[]) { + const actor = createActor(prReviewMachine); + actor.start(); + + for (const event of events) { + actor.send(event); + } + + const snapshot = actor.getSnapshot(); + actor.stop(); + return snapshot; +} + +const mockResult = { + prNumber: 42, + repo: 'test/repo', + success: true, + findings: [], + summary: 'Test review', + overallStatus: 'approve' as const, + reviewedAt: new Date().toISOString(), +}; + +const mockProgress = { + phase: 'analyzing' as const, + prNumber: 42, + progress: 50, + message: 'Analyzing files...', +}; + +describe('prReviewMachine', () => { + describe('initial state', () => { + it('should start in idle state', () => { + const actor = createActor(prReviewMachine); + actor.start(); + expect(actor.getSnapshot().value).toBe('idle'); + actor.stop(); + }); + + it('should have null context initially', () => { + const actor = createActor(prReviewMachine); + actor.start(); + const ctx = actor.getSnapshot().context; + expect(ctx.prNumber).toBeNull(); + expect(ctx.projectId).toBeNull(); + expect(ctx.progress).toBeNull(); + expect(ctx.result).toBeNull(); + expect(ctx.previousResult).toBeNull(); + expect(ctx.error).toBeNull(); + expect(ctx.isFollowup).toBe(false); + expect(ctx.isExternalReview).toBe(false); + actor.stop(); + }); + }); + + describe('happy path: idle -> reviewing -> completed', () => { + it('should transition through the standard review flow', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + ]); + + expect(snapshot.value).toBe('completed'); + expect(snapshot.context.result).toEqual(mockResult); + expect(snapshot.context.prNumber).toBe(42); + expect(snapshot.context.projectId).toBe('proj-1'); + }); + }); + + describe('follow-up review: completed -> reviewing (with previousResult)', () => { + it('should preserve previousResult when starting follow-up from completed', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'START_FOLLOWUP_REVIEW', prNumber: 42, projectId: 'proj-1', previousResult: mockResult }, + ]); + + expect(snapshot.value).toBe('reviewing'); + expect(snapshot.context.isFollowup).toBe(true); + expect(snapshot.context.previousResult).toEqual(mockResult); + expect(snapshot.context.result).toBeNull(); + }); + }); + + describe('error handling: reviewing -> error', () => { + it('should transition to error on REVIEW_ERROR', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_ERROR', error: 'API failure' }, + ]); + + expect(snapshot.value).toBe('error'); + expect(snapshot.context.error).toBe('API failure'); + expect(snapshot.context.progress).toBeNull(); + }); + }); + + describe('cancel flow: reviewing -> error', () => { + it('should set cancelled error message on CANCEL_REVIEW', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'CANCEL_REVIEW' }, + ]); + + expect(snapshot.value).toBe('error'); + expect(snapshot.context.error).toBe('Review cancelled by user'); + }); + }); + + describe('external review: reviewing -> externalReview -> completed', () => { + it('should transition through external review flow', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'DETECT_EXTERNAL_REVIEW' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + ]); + + expect(snapshot.value).toBe('completed'); + expect(snapshot.context.isExternalReview).toBe(true); + expect(snapshot.context.result).toEqual(mockResult); + }); + + it('should transition to error from externalReview', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'DETECT_EXTERNAL_REVIEW' }, + { type: 'REVIEW_ERROR', error: 'External review failed' }, + ]); + + expect(snapshot.value).toBe('error'); + expect(snapshot.context.error).toBe('External review failed'); + }); + }); + + describe('retry after error: error -> reviewing -> completed', () => { + it('should allow starting a new review from error state', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_ERROR', error: 'Failed' }, + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + ]); + + expect(snapshot.value).toBe('completed'); + expect(snapshot.context.error).toBeNull(); + expect(snapshot.context.result).toEqual(mockResult); + }); + + it('should allow starting a follow-up review from error state with previousResult preserved', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'START_FOLLOWUP_REVIEW', prNumber: 42, projectId: 'proj-1', previousResult: mockResult }, + { type: 'REVIEW_ERROR', error: 'Follow-up failed' }, + { type: 'START_FOLLOWUP_REVIEW', prNumber: 42, projectId: 'proj-1', previousResult: mockResult }, + ]); + + expect(snapshot.value).toBe('reviewing'); + expect(snapshot.context.previousResult).toEqual(mockResult); + expect(snapshot.context.isFollowup).toBe(true); + }); + }); + + describe('clear review', () => { + it('should clear from completed to idle', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'CLEAR_REVIEW' }, + ]); + + expect(snapshot.value).toBe('idle'); + expect(snapshot.context.prNumber).toBeNull(); + expect(snapshot.context.result).toBeNull(); + }); + + it('should clear from error to idle', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_ERROR', error: 'Failed' }, + { type: 'CLEAR_REVIEW' }, + ]); + + expect(snapshot.value).toBe('idle'); + expect(snapshot.context.error).toBeNull(); + }); + + it('should clear from reviewing to idle', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'CLEAR_REVIEW' }, + ]); + + expect(snapshot.value).toBe('idle'); + expect(snapshot.context.prNumber).toBeNull(); + expect(snapshot.context.projectId).toBeNull(); + }); + + it('should clear from externalReview to idle', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'DETECT_EXTERNAL_REVIEW' }, + { type: 'CLEAR_REVIEW' }, + ]); + + expect(snapshot.value).toBe('idle'); + expect(snapshot.context.prNumber).toBeNull(); + expect(snapshot.context.isExternalReview).toBe(false); + }); + }); + + describe('reject START_REVIEW when already reviewing', () => { + it('should stay in reviewing when START_REVIEW is sent again', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'START_REVIEW', prNumber: 99, projectId: 'proj-2' }, + ]); + + expect(snapshot.value).toBe('reviewing'); + expect(snapshot.context.prNumber).toBe(42); + }); + }); + + describe('guard: reject SET_PROGRESS when not in reviewing state', () => { + it('should ignore SET_PROGRESS in idle state', () => { + const snapshot = runEvents([ + { type: 'SET_PROGRESS', progress: mockProgress }, + ]); + + expect(snapshot.value).toBe('idle'); + expect(snapshot.context.progress).toBeNull(); + }); + + it('should ignore SET_PROGRESS in completed state', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'SET_PROGRESS', progress: mockProgress }, + ]); + + expect(snapshot.value).toBe('completed'); + expect(snapshot.context.progress).toBeNull(); + }); + + it('should ignore SET_PROGRESS in error state', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_ERROR', error: 'Failed' }, + { type: 'SET_PROGRESS', progress: mockProgress }, + ]); + + expect(snapshot.value).toBe('error'); + expect(snapshot.context.progress).toBeNull(); + }); + }); + + describe('context updates', () => { + it('should store progress during review', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'SET_PROGRESS', progress: mockProgress }, + ]); + + expect(snapshot.context.progress).toEqual(mockProgress); + }); + + it('should store result on completion', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + ]); + + expect(snapshot.context.result).toEqual(mockResult); + expect(snapshot.context.progress).toBeNull(); + }); + + it('should store error on failure', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_ERROR', error: 'Something broke' }, + ]); + + expect(snapshot.context.error).toBe('Something broke'); + expect(snapshot.context.progress).toBeNull(); + }); + + it('should set startedAt on review start', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + ]); + + expect(snapshot.context.startedAt).toBeTruthy(); + }); + }); + + describe('follow-up context', () => { + it('should preserve previousResult in follow-up review', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'START_FOLLOWUP_REVIEW', prNumber: 42, projectId: 'proj-1', previousResult: mockResult }, + { type: 'REVIEW_COMPLETE', result: { ...mockResult, summary: 'Follow-up review' } }, + ]); + + expect(snapshot.value).toBe('completed'); + expect(snapshot.context.previousResult).toEqual(mockResult); + expect(snapshot.context.result?.summary).toBe('Follow-up review'); + }); + + it('should clear previousResult on normal START_REVIEW from completed', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'START_REVIEW', prNumber: 43, projectId: 'proj-1' }, + ]); + + expect(snapshot.context.previousResult).toBeNull(); + expect(snapshot.context.isFollowup).toBe(false); + }); + }); + + describe('stale events', () => { + it('should reject SET_PROGRESS in completed state', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_COMPLETE', result: mockResult }, + { type: 'SET_PROGRESS', progress: mockProgress }, + ]); + + expect(snapshot.value).toBe('completed'); + expect(snapshot.context.progress).toBeNull(); + }); + + it('should reject SET_PROGRESS in error state', () => { + const snapshot = runEvents([ + { type: 'START_REVIEW', prNumber: 42, projectId: 'proj-1' }, + { type: 'REVIEW_ERROR', error: 'Failed' }, + { type: 'SET_PROGRESS', progress: mockProgress }, + ]); + + expect(snapshot.value).toBe('error'); + expect(snapshot.context.progress).toBeNull(); + }); + }); +}); diff --git a/apps/frontend/src/shared/state-machines/__tests__/pr-review-state-utils.test.ts b/apps/frontend/src/shared/state-machines/__tests__/pr-review-state-utils.test.ts new file mode 100644 index 0000000000..39c6f80f88 --- /dev/null +++ b/apps/frontend/src/shared/state-machines/__tests__/pr-review-state-utils.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect } from 'vitest'; +import { + PR_REVIEW_STATE_NAMES, + PR_REVIEW_SETTLED_STATES, + mapPRReviewStateToLegacy, +} from '../pr-review-state-utils'; + +describe('pr-review-state-utils', () => { + describe('PR_REVIEW_STATE_NAMES', () => { + it('should contain all expected state names', () => { + expect(PR_REVIEW_STATE_NAMES).toEqual([ + 'idle', 'reviewing', 'externalReview', 'completed', 'error', + ]); + }); + }); + + describe('PR_REVIEW_SETTLED_STATES', () => { + it('should contain completed and error', () => { + expect(PR_REVIEW_SETTLED_STATES.has('completed')).toBe(true); + expect(PR_REVIEW_SETTLED_STATES.has('error')).toBe(true); + }); + + it('should not contain active states', () => { + expect(PR_REVIEW_SETTLED_STATES.has('idle')).toBe(false); + expect(PR_REVIEW_SETTLED_STATES.has('reviewing')).toBe(false); + expect(PR_REVIEW_SETTLED_STATES.has('externalReview')).toBe(false); + }); + }); + + describe('mapPRReviewStateToLegacy', () => { + it('should map idle to idle', () => { + expect(mapPRReviewStateToLegacy('idle')).toBe('idle'); + }); + + it('should map reviewing to reviewing', () => { + expect(mapPRReviewStateToLegacy('reviewing')).toBe('reviewing'); + }); + + it('should map externalReview to reviewing', () => { + expect(mapPRReviewStateToLegacy('externalReview')).toBe('reviewing'); + }); + + it('should map completed to completed', () => { + expect(mapPRReviewStateToLegacy('completed')).toBe('completed'); + }); + + it('should map error to error', () => { + expect(mapPRReviewStateToLegacy('error')).toBe('error'); + }); + + it('should map unknown states to idle', () => { + expect(mapPRReviewStateToLegacy('unknown')).toBe('idle'); + }); + }); +}); diff --git a/apps/frontend/src/shared/state-machines/index.ts b/apps/frontend/src/shared/state-machines/index.ts index 68cd1f3abb..1c9a29fcbe 100644 --- a/apps/frontend/src/shared/state-machines/index.ts +++ b/apps/frontend/src/shared/state-machines/index.ts @@ -7,3 +7,12 @@ export { mapStateToLegacy, } from './task-state-utils'; export type { TaskStateName } from './task-state-utils'; + +export { prReviewMachine } from './pr-review-machine'; +export type { PRReviewContext, PRReviewEvent } from './pr-review-machine'; +export { + PR_REVIEW_STATE_NAMES, + PR_REVIEW_SETTLED_STATES, + mapPRReviewStateToLegacy, +} from './pr-review-state-utils'; +export type { PRReviewStateName } from './pr-review-state-utils'; diff --git a/apps/frontend/src/shared/state-machines/pr-review-machine.ts b/apps/frontend/src/shared/state-machines/pr-review-machine.ts new file mode 100644 index 0000000000..189002ebb2 --- /dev/null +++ b/apps/frontend/src/shared/state-machines/pr-review-machine.ts @@ -0,0 +1,190 @@ +import { assign, createMachine } from 'xstate'; +import type { PRReviewProgress, PRReviewResult } from '../../preload/api/modules/github-api'; + +export interface PRReviewContext { + prNumber: number | null; + projectId: string | null; + startedAt: string | null; + isFollowup: boolean; + progress: PRReviewProgress | null; + result: PRReviewResult | null; + previousResult: PRReviewResult | null; + error: string | null; + isExternalReview: boolean; +} + +export type PRReviewEvent = + | { type: 'START_REVIEW'; prNumber: number; projectId: string } + | { type: 'START_FOLLOWUP_REVIEW'; prNumber: number; projectId: string; previousResult: PRReviewResult } + | { type: 'SET_PROGRESS'; progress: PRReviewProgress } + | { type: 'REVIEW_COMPLETE'; result: PRReviewResult } + | { type: 'REVIEW_ERROR'; error: string } + | { type: 'CANCEL_REVIEW' } + | { type: 'DETECT_EXTERNAL_REVIEW' } + | { type: 'CLEAR_REVIEW' }; + +const initialContext: PRReviewContext = { + prNumber: null, + projectId: null, + startedAt: null, + isFollowup: false, + progress: null, + result: null, + previousResult: null, + error: null, + isExternalReview: false, +}; + +export const prReviewMachine = createMachine( + { + id: 'prReview', + initial: 'idle', + types: {} as { + context: PRReviewContext; + events: PRReviewEvent; + }, + context: { ...initialContext }, + states: { + idle: { + on: { + START_REVIEW: { + target: 'reviewing', + actions: 'setReviewStart', + }, + START_FOLLOWUP_REVIEW: { + target: 'reviewing', + actions: 'setFollowupReviewStart', + }, + }, + }, + reviewing: { + on: { + SET_PROGRESS: { + actions: 'setProgress', + }, + REVIEW_COMPLETE: { + target: 'completed', + actions: 'setResult', + }, + REVIEW_ERROR: { + target: 'error', + actions: 'setError', + }, + CANCEL_REVIEW: { + target: 'error', + actions: 'setCancelledError', + }, + CLEAR_REVIEW: { + target: 'idle', + actions: 'clearContext', + }, + DETECT_EXTERNAL_REVIEW: { + target: 'externalReview', + actions: 'setExternalReview', + }, + }, + }, + externalReview: { + on: { + REVIEW_COMPLETE: { + target: 'completed', + actions: 'setResult', + }, + REVIEW_ERROR: { + target: 'error', + actions: 'setError', + }, + CANCEL_REVIEW: { + target: 'error', + actions: 'setCancelledError', + }, + CLEAR_REVIEW: { + target: 'idle', + actions: 'clearContext', + }, + }, + }, + completed: { + on: { + START_REVIEW: { + target: 'reviewing', + actions: 'setReviewStart', + }, + START_FOLLOWUP_REVIEW: { + target: 'reviewing', + actions: 'setFollowupReviewStart', + }, + REVIEW_COMPLETE: { + actions: 'setResult', + }, + CLEAR_REVIEW: { + target: 'idle', + actions: 'clearContext', + }, + }, + }, + error: { + on: { + START_REVIEW: { + target: 'reviewing', + actions: 'setReviewStart', + }, + START_FOLLOWUP_REVIEW: { + target: 'reviewing', + actions: 'setFollowupReviewStart', + }, + CLEAR_REVIEW: { + target: 'idle', + actions: 'clearContext', + }, + }, + }, + }, + }, + { + actions: { + setReviewStart: assign({ + prNumber: ({ event }) => (event as { prNumber: number }).prNumber, + projectId: ({ event }) => (event as { projectId: string }).projectId, + startedAt: () => new Date().toISOString(), + isFollowup: () => false, + progress: () => null, + result: () => null, + previousResult: () => null, + error: () => null, + isExternalReview: () => false, + }), + setFollowupReviewStart: assign({ + prNumber: ({ event }) => (event as { prNumber: number }).prNumber, + projectId: ({ event }) => (event as { projectId: string }).projectId, + startedAt: () => new Date().toISOString(), + isFollowup: () => true, + progress: () => null, + result: () => null, + previousResult: ({ event }) => (event as { previousResult: PRReviewResult }).previousResult, + error: () => null, + isExternalReview: () => false, + }), + setProgress: assign({ + progress: ({ event }) => (event as { progress: PRReviewProgress }).progress, + }), + setResult: assign({ + result: ({ event }) => (event as { result: PRReviewResult }).result, + progress: () => null, + }), + setError: assign({ + error: ({ event }) => (event as { error: string }).error, + progress: () => null, + }), + setCancelledError: assign({ + error: () => 'Review cancelled by user', + progress: () => null, + }), + setExternalReview: assign({ + isExternalReview: () => true, + progress: () => null, + }), + clearContext: assign(() => ({ ...initialContext })), + }, + } +); diff --git a/apps/frontend/src/shared/state-machines/pr-review-state-utils.ts b/apps/frontend/src/shared/state-machines/pr-review-state-utils.ts new file mode 100644 index 0000000000..a45f41a49e --- /dev/null +++ b/apps/frontend/src/shared/state-machines/pr-review-state-utils.ts @@ -0,0 +1,52 @@ +/** + * Shared XState PR review state utilities. + * + * Provides type-safe state names, settled states, and legacy status conversion + * derived from the PR review machine definition. + */ + +/** + * All XState PR review state names. + * + * IMPORTANT: These must match the state keys in pr-review-machine.ts. + * If you add/remove a state in the machine, update this array. + */ +export const PR_REVIEW_STATE_NAMES = [ + 'idle', 'reviewing', 'externalReview', 'completed', 'error' +] as const; + +export type PRReviewStateName = typeof PR_REVIEW_STATE_NAMES[number]; + +/** + * XState states where the PR review has "settled" — the review lifecycle + * has reached a terminal or resting state. Progress events should not + * overwrite these states. + */ +export const PR_REVIEW_SETTLED_STATES: ReadonlySet = new Set([ + 'completed', 'error' +]); + +/** + * Legacy review status values used by the existing Zustand store. + */ +type LegacyPRReviewStatus = 'idle' | 'reviewing' | 'completed' | 'error'; + +/** + * Convert XState PR review state to legacy status for backward compatibility. + */ +export function mapPRReviewStateToLegacy(state: string): LegacyPRReviewStatus { + switch (state) { + case 'idle': + return 'idle'; + case 'reviewing': + return 'reviewing'; + case 'externalReview': + return 'reviewing'; + case 'completed': + return 'completed'; + case 'error': + return 'error'; + default: + return 'idle'; + } +}