fix: resolve IPC serialization, auth change wiring, and error→followup transition (qa-requested)
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 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
16f3194ca0
commit
b3d6bbdeda
@@ -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<string, unknown> | 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<string, unknown>;
|
||||
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<string, unknown>;
|
||||
expect(payload).toEqual(expect.objectContaining({ state: 'idle' }));
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<PRReviewActor['getSnapshot']> | 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
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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',
|
||||
|
||||
Reference in New Issue
Block a user