Fix GitHub PR State Management - Follow-up Review Trigger Bug (#1238)
* auto-claude: subtask-1-1 - Add debug logging to trace PR selection → review load Add comprehensive debug logging to useGitHubPRs hook to trace the flow: 1) selectPR function entry with timestamp 2) Existing state check results (hasResult, isReviewing, reviewedCommitSha) 3) getPRReview IPC call results (reviewedCommitSha, postedAt, findingsCount) 4) checkNewCommits IPC calls and results (hasNewCommits, newCommitCount, hasCommitsAfterPosting) 5) setNewCommitsCheckAction store action calls This debug logging helps identify where the state sync failure occurs when a PR with new commits is selected but not detected properly. Note: This logging will be removed in phase-7 (subtask-7-1) cleanup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * auto-claude: subtask-2-1 - Refactor selectPR to ensure checkNewCommits is always called - Extract checkNewCommitsForPR helper function for reuse in both branches - Add race condition protection with currentFetchPRNumberRef checks - Check for new commits AFTER review state is loaded from disk - Check for new commits immediately when review is already in store - Add debug logging to track the flow (to be removed in phase-7) This ensures that when a PR is selected: 1. If review is loaded from disk → checkNewCommits runs after store is updated 2. If review is already in store → checkNewCommits runs immediately 3. Race conditions are handled when user switches PRs rapidly Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * auto-claude: subtask-2-2 - Handle race condition when user switches PRs rapid Add AbortController pattern to prevent stale newCommitsCheck data from appearing when users rapidly switch between PRs in the GitHub PR list. Changes: - Add checkNewCommitsAbortRef to track pending checkNewCommits requests - Abort pending requests when user selects a different PR - Check abort signal before updating store to prevent stale data - Add cleanup on unmount and project change to prevent memory leaks - Suppress error logging for intentionally aborted requests This follows the same AbortController pattern used in PRDetail.tsx for the checkForNewCommits function. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * auto-claude: subtask-3-1 - Verify initialNewCommitsCheck prop sync and checkF - Added optimization to prevent redundant checkNewCommits API calls - Skip API call if local newCommitsCheck already matches the review's commit SHA - This prevents duplicate calls when useGitHubPRs hook has already checked - Added newCommitsCheck to useCallback dependencies for proper re-evaluation The sync mechanism works as follows: 1. useGitHubPRs hook calls checkNewCommits on PR selection 2. Result is stored in Zustand store via setNewCommitsCheckAction 3. PRDetail receives initialNewCommitsCheck prop from store 4. Sync useEffect updates local newCommitsCheck state 5. checkForNewCommits now skips if we have a fresh check for same commit This ensures the UI correctly shows 'ready_for_followup' status when new commits are detected after posting findings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * auto-claude: subtask-6-1 - Add unit tests for selectPR triggering checkNewCom * auto-claude: subtask-6-2 - Extend PRDetail integration tests for follow-up review Added comprehensive integration tests to verify follow-up review trigger behavior: - Test "Ready for Follow-up" status displays when new commits exist after posting - Test "Run Follow-up" button appears when commits overlap with findings - Test onRunFollowupReview callback is triggered on button click - Test follow-up NOT shown when hasCommitsAfterPosting is false - Test follow-up NOT shown when findings have not been posted - Test status updates when newCommitsCheck prop changes - Test "Verify" option appears when commits have no overlap with findings - Test follow-up prompt NOT shown during active review - Test follow-up state resets when PR changes All 15 integration tests pass, full test suite (1786 tests) passes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * auto-claude: subtask-7-1 - Remove debug console.log statements added in phase Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(pr-review): address PR review findings for checkForNewCommits - Remove node_modules symlink accidentally committed to git - Prevent infinite loop when API returns result without lastReviewedCommit - Always reset isCheckingNewCommitsRef in finally block to allow future checks - Fix import path in useGitHubPRs.test.ts (wrong depth) - Use NewCommitsCheck type from github-api module in test helper - Rename misleading test case to match actual behavior - Fix TypeScript mock typing errors with explicit any annotations --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -172,30 +172,48 @@ export function PRDetail({
|
||||
return;
|
||||
}
|
||||
|
||||
// Check for new commits if we have ANY successful review with a commit SHA
|
||||
// This includes follow-up reviews that resolved all issues (no new findings)
|
||||
// New commits = new code that needs to be reviewed, regardless of posting status
|
||||
if (!reviewResult?.success || !reviewResult.reviewedCommitSha) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Skip if we already have a fresh newCommitsCheck from initialNewCommitsCheck (store)
|
||||
// that matches the current review's commit SHA. This prevents redundant API calls
|
||||
// when the useGitHubPRs hook has already checked for new commits on PR selection.
|
||||
// The `lastReviewedCommit` field indicates which commit SHA the check was performed against.
|
||||
if (newCommitsCheck?.lastReviewedCommit === reviewResult.reviewedCommitSha) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Additional guard: if we have any newCommitsCheck result but it lacks lastReviewedCommit,
|
||||
// skip to prevent infinite loops. This handles edge cases where the API returns
|
||||
// a result without the tracking field.
|
||||
if (newCommitsCheck && !newCommitsCheck.lastReviewedCommit) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Cancel any pending check
|
||||
if (checkNewCommitsAbortRef.current) {
|
||||
checkNewCommitsAbortRef.current.abort();
|
||||
}
|
||||
checkNewCommitsAbortRef.current = new AbortController();
|
||||
|
||||
// Check for new commits if we have ANY successful review with a commit SHA
|
||||
// This includes follow-up reviews that resolved all issues (no new findings)
|
||||
// New commits = new code that needs to be reviewed, regardless of posting status
|
||||
if (reviewResult?.success && reviewResult.reviewedCommitSha) {
|
||||
isCheckingNewCommitsRef.current = true;
|
||||
try {
|
||||
const result = await onCheckNewCommits();
|
||||
// Only update state if not aborted
|
||||
if (!checkNewCommitsAbortRef.current?.signal.aborted) {
|
||||
setNewCommitsCheck(result);
|
||||
}
|
||||
} finally {
|
||||
if (!checkNewCommitsAbortRef.current?.signal.aborted) {
|
||||
isCheckingNewCommitsRef.current = false;
|
||||
}
|
||||
isCheckingNewCommitsRef.current = true;
|
||||
try {
|
||||
const result = await onCheckNewCommits();
|
||||
// Only update state if not aborted
|
||||
if (!checkNewCommitsAbortRef.current?.signal.aborted) {
|
||||
setNewCommitsCheck(result);
|
||||
}
|
||||
} finally {
|
||||
// Always reset the checking ref to allow future checks.
|
||||
// The abort only determines whether to update STATE, not whether
|
||||
// the operation tracking should be reset.
|
||||
isCheckingNewCommitsRef.current = false;
|
||||
}
|
||||
}, [reviewResult, onCheckNewCommits]);
|
||||
}, [reviewResult, onCheckNewCommits, newCommitsCheck]);
|
||||
|
||||
useEffect(() => {
|
||||
checkForNewCommits();
|
||||
|
||||
+479
@@ -11,6 +11,7 @@ import { I18nextProvider } from 'react-i18next';
|
||||
import i18n from '../../../../../shared/i18n';
|
||||
import { PRDetail } from '../PRDetail';
|
||||
import type { PRData, PRReviewResult } from '../../hooks/useGitHubPRs';
|
||||
import type { NewCommitsCheck } from '../../../../../preload/api/modules/github-api';
|
||||
|
||||
// Mock window.electronAPI
|
||||
type PostCommentFn = (body: string) => void | Promise<void>;
|
||||
@@ -345,3 +346,481 @@ describe('PRDetail - Clean Review State Reset Integration', () => {
|
||||
unmount();
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Integration tests for PRDetail follow-up review trigger
|
||||
* Tests that follow-up review is correctly triggered when new commits are detected
|
||||
* after findings have been posted to GitHub
|
||||
*/
|
||||
describe('PRDetail - Follow-up Review Trigger Integration', () => {
|
||||
const mockProjectId = 'test-project-id';
|
||||
|
||||
// Helper function to create a mock review result with posted findings
|
||||
function createMockPostedReviewResult(overrides: Partial<PRReviewResult> = {}): PRReviewResult {
|
||||
return {
|
||||
prNumber: 123,
|
||||
repo: 'test/repo',
|
||||
success: true,
|
||||
overallStatus: 'request_changes',
|
||||
summary: 'Found issues that need attention.',
|
||||
findings: [
|
||||
{
|
||||
id: 'finding-1',
|
||||
severity: 'high',
|
||||
category: 'security',
|
||||
title: 'Security Issue',
|
||||
file: 'src/test.ts',
|
||||
line: 10,
|
||||
description: 'High severity security issue',
|
||||
fixable: true
|
||||
}
|
||||
],
|
||||
reviewedAt: '2024-01-01T00:00:00Z',
|
||||
reviewedCommitSha: 'abc123',
|
||||
postedFindingIds: ['finding-1'],
|
||||
hasPostedFindings: true,
|
||||
postedAt: '2024-01-01T01:00:00Z',
|
||||
...overrides
|
||||
};
|
||||
}
|
||||
|
||||
// Helper function to render PRDetail with all props for follow-up review tests
|
||||
function renderPRDetailForFollowup(overrides: {
|
||||
pr?: PRData;
|
||||
reviewResult?: PRReviewResult;
|
||||
initialNewCommitsCheck?: NewCommitsCheck | null;
|
||||
isReviewing?: boolean;
|
||||
onRunFollowupReview?: () => void;
|
||||
} = {}) {
|
||||
const defaultPR = createMockPR({ number: 123 });
|
||||
const defaultReviewResult = createMockPostedReviewResult();
|
||||
const defaultNewCommitsCheck = overrides.initialNewCommitsCheck ?? null;
|
||||
const onRunFollowupReviewMock = overrides.onRunFollowupReview ?? mockOnRunFollowupReview;
|
||||
|
||||
return render(
|
||||
<I18nWrapper>
|
||||
<PRDetail
|
||||
pr={overrides.pr ?? defaultPR}
|
||||
projectId={mockProjectId}
|
||||
reviewResult={overrides.reviewResult ?? defaultReviewResult}
|
||||
previousReviewResult={null}
|
||||
reviewProgress={null}
|
||||
startedAt={null}
|
||||
isReviewing={overrides.isReviewing ?? false}
|
||||
initialNewCommitsCheck={defaultNewCommitsCheck}
|
||||
onRunReview={mockOnRunReview}
|
||||
onRunFollowupReview={onRunFollowupReviewMock}
|
||||
onCheckNewCommits={mockOnCheckNewCommits}
|
||||
onCancelReview={mockOnCancelReview}
|
||||
onPostReview={mockOnPostReview}
|
||||
onPostComment={mockOnPostComment}
|
||||
onMergePR={mockOnMergePR}
|
||||
onAssignPR={mockOnAssignPR}
|
||||
onGetLogs={mockOnGetLogs}
|
||||
/>
|
||||
</I18nWrapper>
|
||||
);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
|
||||
// Setup default mock return values
|
||||
mockOnGetLogs.mockResolvedValue(null);
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: false,
|
||||
hasCommitsAfterPosting: false,
|
||||
newCommitCount: 0
|
||||
});
|
||||
mockOnPostComment.mockResolvedValue(undefined);
|
||||
});
|
||||
|
||||
it('should display "Ready for Follow-up" status when new commits exist after posting', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true
|
||||
}
|
||||
});
|
||||
|
||||
// Wait for the status tree to render with "Ready for Follow-up" status
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/ready for follow-up/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should show "Run Follow-up" button when new commits overlap with findings', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 3,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true
|
||||
}
|
||||
});
|
||||
|
||||
// Wait for the "Run Follow-up" button to appear
|
||||
await waitFor(() => {
|
||||
expect(screen.getByRole('button', { name: /run follow-up/i })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should call onRunFollowupReview when "Run Follow-up" button is clicked', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
// Mock checkNewCommits to return consistent result
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true,
|
||||
lastReviewedCommit: 'abc123' // Match reviewedCommitSha to prevent additional API call
|
||||
});
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true,
|
||||
lastReviewedCommit: 'abc123' // Prevents redundant checkNewCommits call
|
||||
}
|
||||
});
|
||||
|
||||
// Wait for the "Run Follow-up" button to appear
|
||||
const followupButton = await screen.findByRole('button', { name: /run follow-up/i });
|
||||
expect(followupButton).toBeInTheDocument();
|
||||
|
||||
// Click the button
|
||||
fireEvent.click(followupButton);
|
||||
|
||||
// Verify the callback was called
|
||||
expect(mockOnRunFollowupReview).toHaveBeenCalledTimes(1);
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should NOT show follow-up prompt when hasCommitsAfterPosting is false', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: false // New commits exist but before posting
|
||||
}
|
||||
});
|
||||
|
||||
// The "Run Follow-up" button should NOT be visible
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Should show "Waiting for Changes" instead since blockers are posted but no new commits after posting
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/waiting for changes/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should NOT show follow-up prompt when findings have not been posted', async () => {
|
||||
// Review with findings but NOT posted
|
||||
const reviewResult: PRReviewResult = {
|
||||
prNumber: 123,
|
||||
repo: 'test/repo',
|
||||
success: true,
|
||||
overallStatus: 'request_changes',
|
||||
summary: 'Found issues.',
|
||||
findings: [
|
||||
{
|
||||
id: 'finding-1',
|
||||
severity: 'high',
|
||||
category: 'security',
|
||||
title: 'Security Issue',
|
||||
file: 'src/test.ts',
|
||||
line: 10,
|
||||
description: 'High severity issue',
|
||||
fixable: true
|
||||
}
|
||||
],
|
||||
reviewedAt: '2024-01-01T00:00:00Z',
|
||||
reviewedCommitSha: 'abc123',
|
||||
hasPostedFindings: false, // NOT posted
|
||||
postedFindingIds: []
|
||||
};
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true
|
||||
}
|
||||
});
|
||||
|
||||
// The "Run Follow-up" button should NOT be visible since findings weren't posted
|
||||
await waitFor(() => {
|
||||
expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Should show "Needs Attention" since there are unposted blockers
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/needs attention/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should update follow-up status when newCommitsCheck changes via props', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
// Mock checkNewCommits to return no new commits initially
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: false,
|
||||
newCommitCount: 0,
|
||||
hasCommitsAfterPosting: false,
|
||||
lastReviewedCommit: 'abc123'
|
||||
});
|
||||
|
||||
// Start without new commits
|
||||
const { rerender, unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: false,
|
||||
newCommitCount: 0,
|
||||
hasCommitsAfterPosting: false,
|
||||
lastReviewedCommit: 'abc123'
|
||||
}
|
||||
});
|
||||
|
||||
// Should show "Waiting for Changes" initially
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/waiting for changes/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// No follow-up button initially
|
||||
expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument();
|
||||
|
||||
// Update mock before rerender
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 3,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true,
|
||||
lastReviewedCommit: 'abc123'
|
||||
});
|
||||
|
||||
// Rerender with new commits detected
|
||||
rerender(
|
||||
<I18nWrapper>
|
||||
<PRDetail
|
||||
pr={createMockPR({ number: 123 })}
|
||||
projectId={mockProjectId}
|
||||
reviewResult={reviewResult}
|
||||
previousReviewResult={null}
|
||||
reviewProgress={null}
|
||||
startedAt={null}
|
||||
isReviewing={false}
|
||||
initialNewCommitsCheck={{
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 3,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true,
|
||||
lastReviewedCommit: 'abc123'
|
||||
}}
|
||||
onRunReview={mockOnRunReview}
|
||||
onRunFollowupReview={mockOnRunFollowupReview}
|
||||
onCheckNewCommits={mockOnCheckNewCommits}
|
||||
onCancelReview={mockOnCancelReview}
|
||||
onPostReview={mockOnPostReview}
|
||||
onPostComment={mockOnPostComment}
|
||||
onMergePR={mockOnMergePR}
|
||||
onAssignPR={mockOnAssignPR}
|
||||
onGetLogs={mockOnGetLogs}
|
||||
/>
|
||||
</I18nWrapper>
|
||||
);
|
||||
|
||||
// Now should show "Ready for Follow-up" status
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/ready for follow-up/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Follow-up button should now be visible
|
||||
const followupButton = await screen.findByRole('button', { name: /run follow-up/i });
|
||||
expect(followupButton).toBeInTheDocument();
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should show "Verify" option when new commits have no overlap with findings', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
// Mock checkNewCommits to return result with no overlap
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: false,
|
||||
lastReviewedCommit: 'abc123'
|
||||
});
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: false, // No overlap - safe commits
|
||||
lastReviewedCommit: 'abc123'
|
||||
}
|
||||
});
|
||||
|
||||
// Should show "Verify" button for optional follow-up (translation key: verifyAnyway)
|
||||
const verifyButton = await screen.findByRole('button', { name: /^verify$/i });
|
||||
expect(verifyButton).toBeInTheDocument();
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should call onRunFollowupReview when "Verify" button is clicked', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
// Mock checkNewCommits to return result with no overlap
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: false,
|
||||
lastReviewedCommit: 'abc123'
|
||||
});
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: false,
|
||||
lastReviewedCommit: 'abc123'
|
||||
}
|
||||
});
|
||||
|
||||
// Wait for the "Verify" button and click it
|
||||
const verifyButton = await screen.findByRole('button', { name: /^verify$/i });
|
||||
fireEvent.click(verifyButton);
|
||||
|
||||
// Verify the callback was called
|
||||
expect(mockOnRunFollowupReview).toHaveBeenCalledTimes(1);
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should NOT show follow-up prompt during active review', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
// Mock checkNewCommits - won't be called during active review anyway
|
||||
mockOnCheckNewCommits.mockResolvedValue({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true,
|
||||
lastReviewedCommit: 'abc123'
|
||||
});
|
||||
|
||||
const { unmount } = renderPRDetailForFollowup({
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true,
|
||||
lastReviewedCommit: 'abc123'
|
||||
},
|
||||
isReviewing: true // Review in progress
|
||||
});
|
||||
|
||||
// Should show "AI Review in Progress" status - may appear multiple times (title + badge)
|
||||
// Use getAllByText to handle multiple occurrences
|
||||
await waitFor(() => {
|
||||
const reviewingElements = screen.getAllByText(/ai review in progress/i);
|
||||
expect(reviewingElements.length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
// Follow-up button should NOT be visible during active review
|
||||
expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument();
|
||||
|
||||
unmount();
|
||||
});
|
||||
|
||||
it('should reset follow-up state when PR changes', async () => {
|
||||
const reviewResult = createMockPostedReviewResult();
|
||||
|
||||
const { rerender, unmount } = renderPRDetailForFollowup({
|
||||
pr: createMockPR({ number: 123 }),
|
||||
reviewResult,
|
||||
initialNewCommitsCheck: {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 2,
|
||||
hasCommitsAfterPosting: true,
|
||||
hasOverlapWithFindings: true
|
||||
}
|
||||
});
|
||||
|
||||
// Should show follow-up status for PR 123
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/ready for follow-up/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Switch to a different PR that hasn't been reviewed
|
||||
const newPR = createMockPR({ number: 456 });
|
||||
rerender(
|
||||
<I18nWrapper>
|
||||
<PRDetail
|
||||
pr={newPR}
|
||||
projectId={mockProjectId}
|
||||
reviewResult={null} // No review for new PR
|
||||
previousReviewResult={null}
|
||||
reviewProgress={null}
|
||||
startedAt={null}
|
||||
isReviewing={false}
|
||||
initialNewCommitsCheck={null}
|
||||
onRunReview={mockOnRunReview}
|
||||
onRunFollowupReview={mockOnRunFollowupReview}
|
||||
onCheckNewCommits={mockOnCheckNewCommits}
|
||||
onCancelReview={mockOnCancelReview}
|
||||
onPostReview={mockOnPostReview}
|
||||
onPostComment={mockOnPostComment}
|
||||
onMergePR={mockOnMergePR}
|
||||
onAssignPR={mockOnAssignPR}
|
||||
onGetLogs={mockOnGetLogs}
|
||||
/>
|
||||
</I18nWrapper>
|
||||
);
|
||||
|
||||
// Should now show "Not Reviewed" for the new PR
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/not reviewed/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Follow-up button should not be visible for unreviewed PR
|
||||
expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument();
|
||||
|
||||
unmount();
|
||||
});
|
||||
});
|
||||
|
||||
+586
@@ -0,0 +1,586 @@
|
||||
/**
|
||||
* @vitest-environment jsdom
|
||||
*/
|
||||
/**
|
||||
* Unit tests for useGitHubPRs hook - selectPR triggering checkNewCommits
|
||||
*
|
||||
* Key behavior tested:
|
||||
* - selectPR calls checkNewCommits when review exists in store
|
||||
* - selectPR calls checkNewCommits after loading review from disk
|
||||
* - checkNewCommits is NOT called when review is in progress
|
||||
* - checkNewCommits is NOT called when no reviewedCommitSha exists
|
||||
* - Race condition prevention with AbortController
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
import type { PRReviewResult, NewCommitsCheck } from '../../../../../preload/api/modules/github-api';
|
||||
|
||||
// Mock factory functions
|
||||
function createMockReviewResult(overrides: Partial<PRReviewResult> = {}): PRReviewResult {
|
||||
return {
|
||||
prNumber: 123,
|
||||
repo: 'test/repo',
|
||||
success: true,
|
||||
findings: [],
|
||||
summary: 'Test summary',
|
||||
overallStatus: 'approve',
|
||||
reviewedAt: '2024-01-01T00:00:00Z',
|
||||
reviewedCommitSha: 'abc123def456',
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
function createMockNewCommitsCheck(overrides: Partial<NewCommitsCheck> = {}): NewCommitsCheck {
|
||||
return {
|
||||
hasNewCommits: false,
|
||||
newCommitCount: 0,
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Simulate the selectPR logic flow for testing checkNewCommits behavior.
|
||||
* This is extracted from useGitHubPRs.ts for unit testing without needing
|
||||
* to render the full hook in a React environment.
|
||||
*/
|
||||
interface SelectPRTestParams {
|
||||
prNumber: number | null;
|
||||
projectId: string | null;
|
||||
existingState: {
|
||||
result: PRReviewResult | null;
|
||||
isReviewing: boolean;
|
||||
newCommitsCheck: NewCommitsCheck | null;
|
||||
} | null;
|
||||
diskReviewResult: PRReviewResult | null;
|
||||
mockCheckNewCommits: (projectId: string, prNumber: number) => Promise<NewCommitsCheck>;
|
||||
mockGetPRReview: (projectId: string, prNumber: number) => Promise<PRReviewResult | null>;
|
||||
mockSetNewCommitsCheck: (projectId: string, prNumber: number, check: NewCommitsCheck) => void;
|
||||
mockSetPRReviewResult: (projectId: string, result: PRReviewResult) => void;
|
||||
abortSignal?: AbortSignal;
|
||||
}
|
||||
|
||||
interface SelectPRTestResult {
|
||||
checkNewCommitsCalled: boolean;
|
||||
checkNewCommitsCallArgs: { projectId: string; prNumber: number } | null;
|
||||
getPRReviewCalled: boolean;
|
||||
setNewCommitsCheckCalled: boolean;
|
||||
setPRReviewResultCalled: boolean;
|
||||
}
|
||||
|
||||
async function simulateSelectPR(params: SelectPRTestParams): Promise<SelectPRTestResult> {
|
||||
const {
|
||||
prNumber,
|
||||
projectId,
|
||||
existingState,
|
||||
diskReviewResult,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
abortSignal,
|
||||
} = params;
|
||||
|
||||
const result: SelectPRTestResult = {
|
||||
checkNewCommitsCalled: false,
|
||||
checkNewCommitsCallArgs: null,
|
||||
getPRReviewCalled: false,
|
||||
setNewCommitsCheckCalled: false,
|
||||
setPRReviewResultCalled: false,
|
||||
};
|
||||
|
||||
// Early return if no prNumber or deselecting
|
||||
if (prNumber === null || !projectId) {
|
||||
return result;
|
||||
}
|
||||
|
||||
// Helper function to check for new commits (matches useGitHubPRs logic)
|
||||
const checkNewCommitsForPR = async (reviewedCommitSha: string | undefined) => {
|
||||
// Skip if no commit SHA to compare against
|
||||
if (!reviewedCommitSha) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Skip if aborted
|
||||
if (abortSignal?.aborted) {
|
||||
return;
|
||||
}
|
||||
|
||||
result.checkNewCommitsCalled = true;
|
||||
result.checkNewCommitsCallArgs = { projectId, prNumber };
|
||||
|
||||
try {
|
||||
const newCommitsResult = await mockCheckNewCommits(projectId, prNumber);
|
||||
|
||||
// Check abort signal after async call
|
||||
if (abortSignal?.aborted) {
|
||||
return;
|
||||
}
|
||||
|
||||
mockSetNewCommitsCheck(projectId, prNumber, newCommitsResult);
|
||||
result.setNewCommitsCheckCalled = true;
|
||||
} catch {
|
||||
// Ignore errors in tests
|
||||
}
|
||||
};
|
||||
|
||||
// Case 1: No existing state or no result, and not reviewing - load from disk
|
||||
if (!existingState?.result && !existingState?.isReviewing) {
|
||||
result.getPRReviewCalled = true;
|
||||
const reviewFromDisk = await mockGetPRReview(projectId, prNumber);
|
||||
|
||||
if (reviewFromDisk) {
|
||||
mockSetPRReviewResult(projectId, reviewFromDisk);
|
||||
result.setPRReviewResultCalled = true;
|
||||
|
||||
// CRITICAL: Check for new commits AFTER loading review
|
||||
await checkNewCommitsForPR(reviewFromDisk.reviewedCommitSha);
|
||||
}
|
||||
}
|
||||
// Case 2: Review already in store - check for new commits immediately
|
||||
else if (existingState?.result) {
|
||||
await checkNewCommitsForPR(existingState.result.reviewedCommitSha);
|
||||
}
|
||||
// Case 3: Review in progress - do NOT check for new commits
|
||||
// (no action needed, we just don't call checkNewCommits)
|
||||
|
||||
return result;
|
||||
}
|
||||
|
||||
describe('useGitHubPRs - selectPR triggering checkNewCommits', () => {
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
let mockCheckNewCommits: any;
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
let mockGetPRReview: any;
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
let mockSetNewCommitsCheck: any;
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
let mockSetPRReviewResult: any;
|
||||
|
||||
beforeEach(() => {
|
||||
mockCheckNewCommits = vi.fn().mockResolvedValue(createMockNewCommitsCheck());
|
||||
mockGetPRReview = vi.fn().mockResolvedValue(null);
|
||||
mockSetNewCommitsCheck = vi.fn();
|
||||
mockSetPRReviewResult = vi.fn();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
describe('checkNewCommits triggered when review exists in store', () => {
|
||||
it('should call checkNewCommits when selecting PR with existing review in store', async () => {
|
||||
const existingReview = createMockReviewResult({
|
||||
prNumber: 123,
|
||||
reviewedCommitSha: 'abc123',
|
||||
});
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
expect(result.checkNewCommitsCalled).toBe(true);
|
||||
expect(result.checkNewCommitsCallArgs).toEqual({
|
||||
projectId: 'test-project',
|
||||
prNumber: 123,
|
||||
});
|
||||
expect(mockCheckNewCommits).toHaveBeenCalledWith('test-project', 123);
|
||||
});
|
||||
|
||||
it('should update store with new commits check result', async () => {
|
||||
const newCommitsResult = createMockNewCommitsCheck({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 3,
|
||||
hasCommitsAfterPosting: true,
|
||||
});
|
||||
mockCheckNewCommits.mockResolvedValue(newCommitsResult);
|
||||
|
||||
const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' });
|
||||
|
||||
await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
expect(mockSetNewCommitsCheck).toHaveBeenCalledWith(
|
||||
'test-project',
|
||||
123,
|
||||
newCommitsResult
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkNewCommits triggered after loading review from disk', () => {
|
||||
it('should call checkNewCommits after loading review from disk', async () => {
|
||||
const diskReview = createMockReviewResult({
|
||||
prNumber: 456,
|
||||
reviewedCommitSha: 'def789',
|
||||
});
|
||||
mockGetPRReview.mockResolvedValue(diskReview);
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 456,
|
||||
projectId: 'test-project',
|
||||
existingState: null, // No existing state
|
||||
diskReviewResult: diskReview,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
// Should load from disk first
|
||||
expect(result.getPRReviewCalled).toBe(true);
|
||||
expect(result.setPRReviewResultCalled).toBe(true);
|
||||
expect(mockSetPRReviewResult).toHaveBeenCalledWith('test-project', diskReview);
|
||||
|
||||
// Then check for new commits
|
||||
expect(result.checkNewCommitsCalled).toBe(true);
|
||||
expect(mockCheckNewCommits).toHaveBeenCalledWith('test-project', 456);
|
||||
});
|
||||
|
||||
it('should NOT call checkNewCommits if disk review has no reviewedCommitSha', async () => {
|
||||
const diskReviewWithoutSha = createMockReviewResult({
|
||||
prNumber: 789,
|
||||
reviewedCommitSha: undefined, // No SHA
|
||||
});
|
||||
mockGetPRReview.mockResolvedValue(diskReviewWithoutSha);
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 789,
|
||||
projectId: 'test-project',
|
||||
existingState: null,
|
||||
diskReviewResult: diskReviewWithoutSha,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
// Should still load from disk
|
||||
expect(result.getPRReviewCalled).toBe(true);
|
||||
expect(result.setPRReviewResultCalled).toBe(true);
|
||||
|
||||
// But NOT check for new commits
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
expect(mockCheckNewCommits).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkNewCommits NOT triggered during active review', () => {
|
||||
it('should NOT call checkNewCommits when review is in progress', async () => {
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: null, // No result yet
|
||||
isReviewing: true, // Review in progress
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
// Should NOT check for new commits during active review
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
expect(mockCheckNewCommits).not.toHaveBeenCalled();
|
||||
|
||||
// Should also NOT load from disk (review is managed by IPC)
|
||||
expect(result.getPRReviewCalled).toBe(false);
|
||||
});
|
||||
|
||||
it('should still call checkNewCommits when previous result exists during active review', async () => {
|
||||
const previousReview = createMockReviewResult({ reviewedCommitSha: 'old123' });
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: previousReview,
|
||||
isReviewing: true,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
expect(result.checkNewCommitsCalled).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkNewCommits NOT triggered without reviewedCommitSha', () => {
|
||||
it('should NOT call checkNewCommits if store review has no reviewedCommitSha', async () => {
|
||||
const reviewWithoutSha = createMockReviewResult({
|
||||
reviewedCommitSha: undefined,
|
||||
});
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: reviewWithoutSha,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
expect(mockCheckNewCommits).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('Race condition prevention', () => {
|
||||
it('should abort checkNewCommits when signal is aborted', async () => {
|
||||
const abortController = new AbortController();
|
||||
const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' });
|
||||
|
||||
// Abort before the call
|
||||
abortController.abort();
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
abortSignal: abortController.signal,
|
||||
});
|
||||
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
expect(mockSetNewCommitsCheck).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should NOT update store if aborted during async operation', async () => {
|
||||
const abortController = new AbortController();
|
||||
const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' });
|
||||
|
||||
// Make checkNewCommits delay and abort during the delay
|
||||
mockCheckNewCommits.mockImplementation(async () => {
|
||||
// Simulate abort happening during the async call
|
||||
abortController.abort();
|
||||
return createMockNewCommitsCheck({ hasNewCommits: true });
|
||||
});
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
abortSignal: abortController.signal,
|
||||
});
|
||||
|
||||
// checkNewCommits was called
|
||||
expect(result.checkNewCommitsCalled).toBe(true);
|
||||
// But store was NOT updated because abort happened
|
||||
expect(result.setNewCommitsCheckCalled).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Edge cases', () => {
|
||||
it('should NOT trigger anything when prNumber is null (deselecting)', async () => {
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: null,
|
||||
projectId: 'test-project',
|
||||
existingState: null,
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
expect(result.getPRReviewCalled).toBe(false);
|
||||
expect(mockCheckNewCommits).not.toHaveBeenCalled();
|
||||
expect(mockGetPRReview).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should NOT trigger anything when projectId is null', async () => {
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: null,
|
||||
existingState: null,
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
expect(result.getPRReviewCalled).toBe(false);
|
||||
});
|
||||
|
||||
it('should NOT call getPRReview if review already exists in store (not reviewing)', async () => {
|
||||
const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' });
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
// Should NOT load from disk (already in store)
|
||||
expect(result.getPRReviewCalled).toBe(false);
|
||||
expect(mockGetPRReview).not.toHaveBeenCalled();
|
||||
|
||||
// But SHOULD check for new commits
|
||||
expect(result.checkNewCommitsCalled).toBe(true);
|
||||
});
|
||||
|
||||
it('should NOT load from disk if no review exists on disk', async () => {
|
||||
mockGetPRReview.mockResolvedValue(null); // No review on disk
|
||||
|
||||
const result = await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: null, // No existing state
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview,
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult,
|
||||
});
|
||||
|
||||
// Should try to load from disk
|
||||
expect(result.getPRReviewCalled).toBe(true);
|
||||
// But no result to set
|
||||
expect(result.setPRReviewResultCalled).toBe(false);
|
||||
// And no new commits check (no reviewedCommitSha)
|
||||
expect(result.checkNewCommitsCalled).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('useGitHubPRs - checkNewCommits result handling', () => {
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
let mockSetNewCommitsCheck: any;
|
||||
|
||||
beforeEach(() => {
|
||||
mockSetNewCommitsCheck = vi.fn();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it('should handle hasNewCommits: true correctly', async () => {
|
||||
const mockCheckNewCommits = vi.fn().mockResolvedValue(
|
||||
createMockNewCommitsCheck({
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 5,
|
||||
hasCommitsAfterPosting: true,
|
||||
})
|
||||
);
|
||||
|
||||
const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' });
|
||||
|
||||
await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview: vi.fn(),
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult: vi.fn(),
|
||||
});
|
||||
|
||||
expect(mockSetNewCommitsCheck).toHaveBeenCalledWith('test-project', 123, {
|
||||
hasNewCommits: true,
|
||||
newCommitCount: 5,
|
||||
hasCommitsAfterPosting: true,
|
||||
});
|
||||
});
|
||||
|
||||
it('should handle hasNewCommits: false correctly', async () => {
|
||||
const mockCheckNewCommits = vi.fn().mockResolvedValue(
|
||||
createMockNewCommitsCheck({
|
||||
hasNewCommits: false,
|
||||
newCommitCount: 0,
|
||||
hasCommitsAfterPosting: false,
|
||||
})
|
||||
);
|
||||
|
||||
const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' });
|
||||
|
||||
await simulateSelectPR({
|
||||
prNumber: 123,
|
||||
projectId: 'test-project',
|
||||
existingState: {
|
||||
result: existingReview,
|
||||
isReviewing: false,
|
||||
newCommitsCheck: null,
|
||||
},
|
||||
diskReviewResult: null,
|
||||
mockCheckNewCommits,
|
||||
mockGetPRReview: vi.fn(),
|
||||
mockSetNewCommitsCheck,
|
||||
mockSetPRReviewResult: vi.fn(),
|
||||
});
|
||||
|
||||
expect(mockSetNewCommitsCheck).toHaveBeenCalledWith('test-project', 123, {
|
||||
hasNewCommits: false,
|
||||
newCommitCount: 0,
|
||||
hasCommitsAfterPosting: false,
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -86,6 +86,8 @@ export function useGitHubPRs(
|
||||
const hasLoadedRef = useRef(false);
|
||||
// Track the current PR being fetched (for race condition prevention)
|
||||
const currentFetchPRNumberRef = useRef<number | null>(null);
|
||||
// AbortController for cancelling pending checkNewCommits calls on rapid PR switching
|
||||
const checkNewCommitsAbortRef = useRef<AbortController | null>(null);
|
||||
|
||||
// Get PR review state from the global store
|
||||
const prReviews = usePRReviewStore((state) => state.prReviews);
|
||||
@@ -249,12 +251,35 @@ export function useGitHubPRs(
|
||||
setPrs([]);
|
||||
setSelectedPRNumber(null);
|
||||
setSelectedPRDetails(null);
|
||||
currentFetchPRNumberRef.current = null;
|
||||
// Cancel any pending checkNewCommits request
|
||||
if (checkNewCommitsAbortRef.current) {
|
||||
checkNewCommitsAbortRef.current.abort();
|
||||
checkNewCommitsAbortRef.current = null;
|
||||
}
|
||||
}, [projectId]);
|
||||
|
||||
// Cleanup abort controller on unmount to prevent memory leaks
|
||||
// and avoid state updates on unmounted components
|
||||
useEffect(() => {
|
||||
return () => {
|
||||
if (checkNewCommitsAbortRef.current) {
|
||||
checkNewCommitsAbortRef.current.abort();
|
||||
}
|
||||
};
|
||||
}, []);
|
||||
|
||||
// No need for local IPC listeners - they're handled globally in github-store
|
||||
|
||||
const selectPR = useCallback(
|
||||
(prNumber: number | null) => {
|
||||
// Abort any pending checkNewCommits request from previous PR selection
|
||||
// This prevents stale data from appearing when user switches PRs rapidly
|
||||
if (checkNewCommitsAbortRef.current) {
|
||||
checkNewCommitsAbortRef.current.abort();
|
||||
checkNewCommitsAbortRef.current = null;
|
||||
}
|
||||
|
||||
setSelectedPRNumber(prNumber);
|
||||
// Note: Don't reset review result - it comes from the store now
|
||||
// and persists across navigation
|
||||
@@ -290,12 +315,63 @@ export function useGitHubPRs(
|
||||
}
|
||||
});
|
||||
|
||||
// Helper function to check for new commits with race condition protection
|
||||
// This is called after review state is available (from store or disk)
|
||||
// Uses AbortController pattern to cancel pending checks when user switches PRs rapidly
|
||||
const checkNewCommitsForPR = (reviewedCommitSha: string | undefined) => {
|
||||
// Skip if no commit SHA to compare against
|
||||
if (!reviewedCommitSha) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Skip if user has already switched to a different PR (race condition prevention)
|
||||
if (prNumber !== currentFetchPRNumberRef.current) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Cancel any pending checkNewCommits request before starting a new one
|
||||
if (checkNewCommitsAbortRef.current) {
|
||||
checkNewCommitsAbortRef.current.abort();
|
||||
}
|
||||
checkNewCommitsAbortRef.current = new AbortController();
|
||||
const currentAbortController = checkNewCommitsAbortRef.current;
|
||||
|
||||
window.electronAPI.github
|
||||
.checkNewCommits(projectId, prNumber)
|
||||
.then((newCommitsResult) => {
|
||||
// Check if request was aborted (user switched PRs)
|
||||
if (currentAbortController.signal.aborted) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Final race condition check before updating store
|
||||
if (prNumber !== currentFetchPRNumberRef.current) {
|
||||
return;
|
||||
}
|
||||
|
||||
setNewCommitsCheckAction(projectId, prNumber, newCommitsResult);
|
||||
})
|
||||
.catch((err) => {
|
||||
// Don't log errors for aborted requests
|
||||
if (currentAbortController.signal.aborted) {
|
||||
return;
|
||||
}
|
||||
console.warn(`Failed to check new commits for PR #${prNumber}:`, err);
|
||||
});
|
||||
};
|
||||
|
||||
// Load existing review from disk if not already in store
|
||||
const existingState = getPRReviewState(projectId, prNumber);
|
||||
|
||||
// Only fetch from disk if we don't have a result in the store AND no review is running
|
||||
// If a review is in progress, the state is managed by IPC listeners - don't overwrite it
|
||||
if (!existingState?.result && !existingState?.isReviewing) {
|
||||
window.electronAPI.github.getPRReview(projectId, prNumber).then((result) => {
|
||||
// Race condition check: skip if user switched PRs
|
||||
if (prNumber !== currentFetchPRNumberRef.current) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (result) {
|
||||
// Update store with the loaded result
|
||||
// Preserve newCommitsCheck when loading existing review from disk
|
||||
@@ -305,33 +381,16 @@ export function useGitHubPRs(
|
||||
|
||||
// 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
|
||||
const reviewedCommitSha = result.reviewedCommitSha;
|
||||
if (reviewedCommitSha) {
|
||||
window.electronAPI.github
|
||||
.checkNewCommits(projectId, prNumber)
|
||||
.then((newCommitsResult) => {
|
||||
setNewCommitsCheckAction(projectId, prNumber, newCommitsResult);
|
||||
})
|
||||
.catch((err) => {
|
||||
console.warn(`Failed to check new commits for PR #${prNumber}:`, err);
|
||||
});
|
||||
}
|
||||
// CRITICAL: This runs AFTER store is updated with review result
|
||||
checkNewCommitsForPR(result.reviewedCommitSha);
|
||||
}
|
||||
});
|
||||
} else if (existingState?.result) {
|
||||
// Review already in store - always check for new commits to get fresh status
|
||||
const reviewedCommitSha = existingState.result.reviewedCommitSha;
|
||||
if (reviewedCommitSha) {
|
||||
window.electronAPI.github
|
||||
.checkNewCommits(projectId, prNumber)
|
||||
.then((newCommitsResult) => {
|
||||
setNewCommitsCheckAction(projectId, prNumber, newCommitsResult);
|
||||
})
|
||||
.catch((err) => {
|
||||
console.warn(`Failed to check new commits for PR #${prNumber}:`, err);
|
||||
});
|
||||
}
|
||||
// CRITICAL: Review state is already available, check for new commits immediately
|
||||
checkNewCommitsForPR(existingState.result.reviewedCommitSha);
|
||||
}
|
||||
// If existingState?.isReviewing, state is managed by IPC listeners - do nothing
|
||||
}
|
||||
},
|
||||
[projectId, getPRReviewState, setNewCommitsCheckAction]
|
||||
|
||||
Reference in New Issue
Block a user