From b3d6bbdeda1ed3fd765bc800abadf6916e95fd3a Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sat, 14 Feb 2026 08:49:57 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20resolve=20IPC=20serialization,=20auth=20?= =?UTF-8?q?change=20wiring,=20and=20error=E2=86=92followup=20transition=20?= =?UTF-8?q?(qa-requested)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: - Build PRReviewStatePayload object in emitStateToRenderer instead of sending raw args - Wire up handleAuthChange via ipcMain listener for GITHUB_AUTH_CHANGED - Add START_FOLLOWUP_REVIEW transition to error state in pr-review-machine - Update state manager tests to verify PRReviewStatePayload shape Verified: - All 3279 tests pass - TypeScript compilation clean QA Fix Session: 1 Co-Authored-By: Claude Opus 4.6 --- .../__tests__/pr-review-state-manager.test.ts | 29 ++++++++++++++----- .../ipc-handlers/github/oauth-handlers.ts | 2 ++ .../main/ipc-handlers/github/pr-handlers.ts | 5 ++++ .../src/main/pr-review-state-manager.ts | 21 +++++++++++--- .../__tests__/pr-review-machine.test.ts | 14 +++++++++ .../state-machines/pr-review-machine.ts | 4 +++ 6 files changed, 64 insertions(+), 11 deletions(-) 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 index 3661fe4d..02f7135b 100644 --- a/apps/frontend/src/main/__tests__/pr-review-state-manager.test.ts +++ b/apps/frontend/src/main/__tests__/pr-review-state-manager.test.ts @@ -160,20 +160,35 @@ describe('PRReviewStateManager', () => { expect.any(Function), 'github:pr:reviewStateChange', expect.any(String), - expect.any(String), - expect.anything() + expect.objectContaining({ state: expect.any(String) }) ); }); - it('should include state value and context in payload', () => { + 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[]) => call[3] === 'reviewing' + (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}`); - expect(reviewingCall![4]).toEqual(expect.objectContaining({ prNumber, projectId })); + 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', () => { @@ -244,8 +259,8 @@ describe('PRReviewStateManager', () => { // Should emit idle/null state for each PR expect(mockSafeSendToRenderer).toHaveBeenCalledTimes(2); for (const call of mockSafeSendToRenderer.mock.calls) { - expect(call[3]).toBe('idle'); // stateValue - expect(call[4]).toBeNull(); // context + const payload = call[3] as Record; + expect(payload).toEqual(expect.objectContaining({ state: 'idle' })); } }); 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 773feb15..e1b40b1d 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,8 @@ function sendAuthChangedToRenderer(oldUsername: string | null, newUsername: stri for (const win of windows) { win.webContents.send(IPC_CHANNELS.GITHUB_AUTH_CHANGED, payload); } + // Emit on ipcMain so main-process listeners (e.g., PRReviewStateManager) can react + 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 e56810f9..35ef0bc7 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -1670,6 +1670,11 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v // 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, () => { + 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, diff --git a/apps/frontend/src/main/pr-review-state-manager.ts b/apps/frontend/src/main/pr-review-state-manager.ts index 01ba68fb..301a3c4a 100644 --- a/apps/frontend/src/main/pr-review-state-manager.ts +++ b/apps/frontend/src/main/pr-review-state-manager.ts @@ -2,7 +2,7 @@ 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 } from '../preload/api/modules/github-api'; +import type { PRReviewProgress, PRReviewResult, PRReviewStatePayload } from '../preload/api/modules/github-api'; import { IPC_CHANNELS } from '../shared/constants'; import { safeSendToRenderer } from './ipc-handlers/utils'; @@ -149,14 +149,27 @@ export class PRReviewStateManager { snapshot: ReturnType | null ): void { const stateValue = snapshot ? String(snapshot.value) : 'idle'; - const context = snapshot ? snapshot.context : null; + 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, - stateValue, - context + payload ); } } 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 index 4df160de..f2e5d548 100644 --- 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 @@ -152,6 +152,20 @@ describe('prReviewMachine', () => { 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', () => { diff --git a/apps/frontend/src/shared/state-machines/pr-review-machine.ts b/apps/frontend/src/shared/state-machines/pr-review-machine.ts index fd14698a..8ae075a8 100644 --- a/apps/frontend/src/shared/state-machines/pr-review-machine.ts +++ b/apps/frontend/src/shared/state-machines/pr-review-machine.ts @@ -118,6 +118,10 @@ export const prReviewMachine = createMachine( target: 'reviewing', actions: 'setReviewStart', }, + START_FOLLOWUP_REVIEW: { + target: 'reviewing', + actions: 'setFollowupReviewStart', + }, CLEAR_REVIEW: { target: 'idle', actions: 'clearContext',