Skip to content

Commit 398518e

Browse files
AndyMik90claude
andcommitted
fix: address PR review findings for XState PR review refactor
- handleComplete uses getOrCreateActor so late-arriving results after auth change or restart are processed instead of silently dropped - GITHUB_AUTH_CHANGED handler now kills running review subprocesses and aborts CI wait controllers before clearing XState actors - Duplicate review detection reads actual progress from actor snapshot instead of hardcoding progress=50 - handleClearReview stops actor directly without sending CLEAR_REVIEW event, preventing double IPC emission to renderer Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f20a43c commit 398518e

3 files changed

Lines changed: 59 additions & 6 deletions

File tree

apps/frontend/src/main/__tests__/pr-review-state-manager.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,15 @@ describe('PRReviewStateManager', () => {
124124
expect(snapshot!.context.result).toEqual(result);
125125
});
126126

127+
it('should create actor for handleComplete on unknown PR (late-arriving result)', () => {
128+
const result = createMockResult();
129+
// No handleStartReview called — handleComplete should create the actor
130+
manager.handleComplete(projectId, prNumber, result);
131+
const snapshot = manager.getState(projectId, prNumber);
132+
expect(snapshot).not.toBeNull();
133+
expect(snapshot!.context.result).toEqual(result);
134+
});
135+
127136
it('should send DETECT_EXTERNAL_REVIEW when overallStatus is in_progress', () => {
128137
manager.handleStartReview(projectId, prNumber);
129138
const result = createMockResult({ overallStatus: 'in_progress' });
@@ -239,6 +248,19 @@ describe('PRReviewStateManager', () => {
239248
expect(manager.getState(projectId, prNumber)).toBeNull();
240249
});
241250

251+
it('should emit exactly one cleared state IPC on handleClearReview (no double emission)', () => {
252+
manager.handleStartReview(projectId, prNumber);
253+
mockSafeSendToRenderer.mockClear();
254+
255+
manager.handleClearReview(projectId, prNumber);
256+
257+
// Should emit exactly 1 cleared state, not 2 (no double emission from
258+
// sending CLEAR_REVIEW to actor subscription + manual emitClearedState)
259+
expect(mockSafeSendToRenderer).toHaveBeenCalledTimes(1);
260+
const payload = mockSafeSendToRenderer.mock.calls[0][3] as Record<string, unknown>;
261+
expect(payload).toEqual(expect.objectContaining({ state: 'idle' }));
262+
});
263+
242264
it('should stop ALL actors and clear maps on handleAuthChange', () => {
243265
manager.handleStartReview(projectId, 1);
244266
manager.handleStartReview(projectId, 2);

apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1664,6 +1664,24 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v
16641664

16651665
// Clear all PR review actors when GitHub auth changes (account swap)
16661666
ipcMain.on(IPC_CHANNELS.GITHUB_AUTH_CHANGED, () => {
1667+
// Cancel all running review subprocesses and CI wait controllers
1668+
for (const [reviewKey, entry] of runningReviews) {
1669+
if (entry === CI_WAIT_PLACEHOLDER) {
1670+
const abortController = ciWaitAbortControllers.get(reviewKey);
1671+
if (abortController) {
1672+
abortController.abort();
1673+
ciWaitAbortControllers.delete(reviewKey);
1674+
}
1675+
} else {
1676+
try {
1677+
entry.kill("SIGTERM");
1678+
} catch {
1679+
// Process may have already exited
1680+
}
1681+
}
1682+
}
1683+
runningReviews.clear();
1684+
ciWaitAbortControllers.clear();
16671685
prReviewStateManager.handleAuthChange();
16681686
});
16691687

@@ -1881,10 +1899,12 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v
18811899
// Check if already running — notify renderer so it can display ongoing logs
18821900
if (runningReviews.has(reviewKey)) {
18831901
debugLog("Review already running, notifying renderer", { reviewKey });
1902+
const currentSnapshot = prReviewStateManager.getState(projectId, prNumber);
1903+
const currentProgress = currentSnapshot?.context?.progress?.progress ?? 50;
18841904
sendProgress({
18851905
phase: "analyzing",
18861906
prNumber,
1887-
progress: 50,
1907+
progress: currentProgress,
18881908
message: "Review is already in progress. Reconnecting to ongoing review...",
18891909
});
18901910
return;
@@ -2904,10 +2924,12 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v
29042924
// Check if already running — notify renderer so it can display ongoing logs
29052925
if (runningReviews.has(reviewKey)) {
29062926
debugLog("Follow-up review already running, notifying renderer", { reviewKey });
2927+
const currentSnapshot = prReviewStateManager.getState(projectId, prNumber);
2928+
const currentProgress = currentSnapshot?.context?.progress?.progress ?? 50;
29072929
sendProgress({
29082930
phase: "analyzing",
29092931
prNumber,
2910-
progress: 50,
2932+
progress: currentProgress,
29112933
message: "Follow-up review is already in progress. Reconnecting to ongoing review...",
29122934
});
29132935
return;

apps/frontend/src/main/pr-review-state-manager.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,16 @@ export class PRReviewStateManager {
5353
}
5454

5555
handleComplete(projectId: string, prNumber: number, result: PRReviewResult): void {
56-
const actor = this.getActor(projectId, prNumber);
57-
if (!actor) return;
56+
// Use getOrCreateActor so late-arriving results (e.g. after auth change or
57+
// app restart) still get processed instead of silently dropped.
58+
const actor = this.getOrCreateActor(projectId, prNumber);
59+
60+
// If the actor is in idle state (freshly created for a late-arriving result),
61+
// transition to reviewing first so REVIEW_COMPLETE is accepted.
62+
const snapshot = actor.getSnapshot();
63+
if (String(snapshot.value) === 'idle') {
64+
actor.send({ type: 'START_REVIEW', prNumber, projectId } satisfies PRReviewEvent);
65+
}
5866

5967
// Detect external review (result arrives with 'in_progress' status from outside)
6068
if (result.overallStatus === 'in_progress') {
@@ -80,9 +88,10 @@ export class PRReviewStateManager {
8088
const key = this.getKey(projectId, prNumber);
8189
const actor = this.actors.get(key);
8290
if (actor) {
83-
// Capture snapshot before clearing so the emitted payload has real context
91+
// Capture snapshot before stopping so the emitted payload has real context.
92+
// Don't send CLEAR_REVIEW to the actor — that would trigger the subscription
93+
// and cause a duplicate IPC emission alongside emitClearedState below.
8494
const snapshot = actor.getSnapshot();
85-
actor.send({ type: 'CLEAR_REVIEW' } satisfies PRReviewEvent);
8695
actor.stop();
8796
this.actors.delete(key);
8897
this.emitClearedState(key, snapshot?.context ?? null);

0 commit comments

Comments
 (0)