Fix PR List Update on Post Status Click (#1207)
* auto-claude: subtask-1-1 - Add markReviewPosted function to useGitHubPRs hook Fix PR list not updating when "Post Status" button is clicked for blocked PRs. Changes: - Add markReviewPosted function to useGitHubPRs hook that updates the store with hasPostedFindings: true - Pass markReviewPosted through GitHubPRs.tsx to PRDetail component - Call onMarkReviewPosted in handlePostBlockedStatus after successful post This ensures the PR list status display updates immediately when posting blocked status (BLOCKED/NEEDS_REVISION verdicts with no findings). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: persist hasPostedFindings flag to disk in markReviewPosted The markReviewPosted function was only updating the in-memory Zustand store without persisting the has_posted_findings flag to the review JSON file on disk. After an app restart, the flag would be lost. This commit adds: - New IPC handler GITHUB_PR_MARK_REVIEW_POSTED that updates the review JSON file on disk with has_posted_findings=true and posted_at timestamp - New API method markReviewPosted in github-api.ts - Updated markReviewPosted in useGitHubPRs.ts to call the IPC handler first, then update the in-memory store Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address follow-up review findings for markReviewPosted Fixes several issues identified in the follow-up PR review: 1. Race condition with prNumber: onMarkReviewPosted callback now accepts prNumber as a parameter instead of relying on closure state, preventing wrong PR updates when user switches PRs during async operations. 2. Silent failure handling: handlePostBlockedStatus now checks the return value of onPostComment and only marks review as posted on success. 3. Missing postedAt timestamp: markReviewPosted now includes postedAt timestamp in the store update for consistency with disk state. 4. Store not updated when result not loaded: If the review result hasn't been loaded yet (race condition), markReviewPosted now reloads it from disk after persistence to ensure the UI reflects the correct state. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: update PostCommentFn type in PRDetail tests to match new signature Update the mock type to return Promise<boolean> instead of void | Promise<void> to match the updated onPostComment interface that now returns success status. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: correct mock return value type in PRDetail integration test The mockOnPostComment.mockResolvedValue() was passing undefined instead of boolean, causing TypeScript type check failures in CI. PostCommentFn returns Promise<boolean>, so the mock must also return a boolean value. * fix(security): eliminate TOCTOU race condition in markReviewPosted handler Remove separate fs.existsSync() check before fs.readFileSync() to prevent time-of-check to time-of-use (TOCTOU) race condition flagged by CodeQL. Instead, let readFileSync throw ENOENT if file doesn't exist and handle it in the catch block with specific error code checking. * chore: merge develop and fix additional test type error Resolve merge conflict in ipc.ts by keeping both new IPC channels: - GITHUB_PR_MARK_REVIEW_POSTED (from this branch) - GITHUB_PR_UPDATE_BRANCH (from develop) Fix second occurrence of mockOnPostComment.mockResolvedValue(undefined) type error in PRDetail integration tests. --------- Co-authored-by: Test User <test@example.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -1777,6 +1777,47 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v
|
||||
}
|
||||
);
|
||||
|
||||
// Mark review as posted (persists has_posted_findings to disk)
|
||||
ipcMain.handle(
|
||||
IPC_CHANNELS.GITHUB_PR_MARK_REVIEW_POSTED,
|
||||
async (_, projectId: string, prNumber: number): Promise<boolean> => {
|
||||
debugLog("markReviewPosted handler called", { projectId, prNumber });
|
||||
const result = await withProjectOrNull(projectId, async (project) => {
|
||||
try {
|
||||
const reviewPath = path.join(getGitHubDir(project), "pr", `review_${prNumber}.json`);
|
||||
|
||||
// Read file directly without separate existence check to avoid TOCTOU race condition
|
||||
// If file doesn't exist, readFileSync will throw ENOENT which we handle below
|
||||
const rawData = fs.readFileSync(reviewPath, "utf-8");
|
||||
// Sanitize data before parsing (review may contain data from GitHub API)
|
||||
const sanitizedData = sanitizeNetworkData(rawData);
|
||||
const data = JSON.parse(sanitizedData);
|
||||
|
||||
// Mark as posted
|
||||
data.has_posted_findings = true;
|
||||
data.posted_at = new Date().toISOString();
|
||||
|
||||
fs.writeFileSync(reviewPath, JSON.stringify(data, null, 2), "utf-8");
|
||||
debugLog("Marked review as posted", { prNumber });
|
||||
|
||||
return true;
|
||||
} catch (error) {
|
||||
// Handle file not found (ENOENT) separately for clearer logging
|
||||
if (error instanceof Error && "code" in error && error.code === "ENOENT") {
|
||||
debugLog("Review file not found", { prNumber });
|
||||
return false;
|
||||
}
|
||||
debugLog("Failed to mark review as posted", {
|
||||
prNumber,
|
||||
error: error instanceof Error ? error.message : error,
|
||||
});
|
||||
return false;
|
||||
}
|
||||
});
|
||||
return result ?? false;
|
||||
}
|
||||
);
|
||||
|
||||
// Post comment to PR
|
||||
ipcMain.handle(
|
||||
IPC_CHANNELS.GITHUB_PR_POST_COMMENT,
|
||||
|
||||
@@ -272,6 +272,7 @@ export interface GitHubAPI {
|
||||
postPRComment: (projectId: string, prNumber: number, body: string) => Promise<boolean>;
|
||||
mergePR: (projectId: string, prNumber: number, mergeMethod?: 'merge' | 'squash' | 'rebase') => Promise<boolean>;
|
||||
assignPR: (projectId: string, prNumber: number, username: string) => Promise<boolean>;
|
||||
markReviewPosted: (projectId: string, prNumber: number) => Promise<boolean>;
|
||||
getPRReview: (projectId: string, prNumber: number) => Promise<PRReviewResult | null>;
|
||||
getPRReviewsBatch: (projectId: string, prNumbers: number[]) => Promise<Record<number, PRReviewResult | null>>;
|
||||
|
||||
@@ -678,6 +679,9 @@ export const createGitHubAPI = (): GitHubAPI => ({
|
||||
assignPR: (projectId: string, prNumber: number, username: string): Promise<boolean> =>
|
||||
invokeIpc(IPC_CHANNELS.GITHUB_PR_ASSIGN, projectId, prNumber, username),
|
||||
|
||||
markReviewPosted: (projectId: string, prNumber: number): Promise<boolean> =>
|
||||
invokeIpc(IPC_CHANNELS.GITHUB_PR_MARK_REVIEW_POSTED, projectId, prNumber),
|
||||
|
||||
getPRReview: (projectId: string, prNumber: number): Promise<PRReviewResult | null> =>
|
||||
invokeIpc(IPC_CHANNELS.GITHUB_PR_GET_REVIEW, projectId, prNumber),
|
||||
|
||||
|
||||
@@ -77,6 +77,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
|
||||
postComment,
|
||||
mergePR,
|
||||
assignPR,
|
||||
markReviewPosted,
|
||||
refresh,
|
||||
loadMore,
|
||||
isConnected,
|
||||
@@ -140,10 +141,11 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
|
||||
);
|
||||
|
||||
const handlePostComment = useCallback(
|
||||
async (body: string) => {
|
||||
async (body: string): Promise<boolean> => {
|
||||
if (selectedPRNumber) {
|
||||
await postComment(selectedPRNumber, body);
|
||||
return await postComment(selectedPRNumber, body);
|
||||
}
|
||||
return false;
|
||||
},
|
||||
[selectedPRNumber, postComment]
|
||||
);
|
||||
@@ -173,6 +175,10 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
|
||||
return null;
|
||||
}, [selectedProjectId, selectedPRNumber]);
|
||||
|
||||
const handleMarkReviewPosted = useCallback(async (prNumber: number) => {
|
||||
await markReviewPosted(prNumber);
|
||||
}, [markReviewPosted]);
|
||||
|
||||
// Not connected state
|
||||
if (!isConnected) {
|
||||
return <NotConnectedState error={error} onOpenSettings={onOpenSettings} t={t} />;
|
||||
@@ -259,6 +265,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
|
||||
onMergePR={handleMergePR}
|
||||
onAssignPR={handleAssignPR}
|
||||
onGetLogs={handleGetLogs}
|
||||
onMarkReviewPosted={handleMarkReviewPosted}
|
||||
/>
|
||||
) : (
|
||||
<EmptyState message={t("prReview.selectPRToView")} />
|
||||
|
||||
@@ -52,10 +52,11 @@ interface PRDetailProps {
|
||||
onCheckNewCommits: () => Promise<NewCommitsCheck>;
|
||||
onCancelReview: () => void;
|
||||
onPostReview: (selectedFindingIds?: string[], options?: { forceApprove?: boolean }) => Promise<boolean>;
|
||||
onPostComment: (body: string) => void;
|
||||
onPostComment: (body: string) => Promise<boolean>;
|
||||
onMergePR: (mergeMethod?: 'merge' | 'squash' | 'rebase') => void;
|
||||
onAssignPR: (username: string) => void;
|
||||
onGetLogs: () => Promise<PRLogsType | null>;
|
||||
onMarkReviewPosted?: (prNumber: number) => Promise<void>;
|
||||
}
|
||||
|
||||
function getStatusColor(status: PRReviewResult['overallStatus']): string {
|
||||
@@ -89,6 +90,7 @@ export function PRDetail({
|
||||
onMergePR,
|
||||
onAssignPR: _onAssignPR,
|
||||
onGetLogs,
|
||||
onMarkReviewPosted,
|
||||
}: PRDetailProps) {
|
||||
const { t } = useTranslation('common');
|
||||
// Selection state for findings
|
||||
@@ -780,12 +782,17 @@ ${reviewResult.summary}
|
||||
|
||||
${t('prReview.blockedStatusMessageFooter')}`;
|
||||
|
||||
await Promise.resolve(onPostComment(blockedStatusMessage));
|
||||
const success = await onPostComment(blockedStatusMessage);
|
||||
|
||||
// Only mark as posted on success if PR hasn't changed
|
||||
if (pr.number === currentPr) {
|
||||
// Only mark as posted on success if PR hasn't changed AND comment was posted successfully
|
||||
if (success && pr.number === currentPr) {
|
||||
setBlockedStatusPosted(true);
|
||||
setBlockedStatusError(null);
|
||||
// Update the store to mark review as posted so PR list reflects the change
|
||||
// Pass prNumber explicitly to avoid race conditions with PR selection changes
|
||||
await onMarkReviewPosted?.(currentPr);
|
||||
} else if (!success && pr.number === currentPr) {
|
||||
setBlockedStatusError('Failed to post comment');
|
||||
}
|
||||
} catch (err) {
|
||||
console.error('Failed to post blocked status comment:', err);
|
||||
|
||||
+4
-4
@@ -14,8 +14,8 @@ 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>;
|
||||
const mockOnPostComment = vi.fn<PostCommentFn>();
|
||||
type PostCommentFn = (body: string) => Promise<boolean>;
|
||||
const mockOnPostComment = vi.fn<PostCommentFn>().mockResolvedValue(true);
|
||||
const mockOnPostReview = vi.fn();
|
||||
const mockOnRunReview = vi.fn();
|
||||
const mockOnRunFollowupReview = vi.fn();
|
||||
@@ -128,7 +128,7 @@ describe('PRDetail - Clean Review State Reset Integration', () => {
|
||||
newCommitCount: 0
|
||||
});
|
||||
// Resolve successfully by default
|
||||
mockOnPostComment.mockResolvedValue(undefined);
|
||||
mockOnPostComment.mockResolvedValue(true);
|
||||
});
|
||||
|
||||
it('should reset cleanReviewPosted state when pr.number changes', async () => {
|
||||
@@ -432,7 +432,7 @@ describe('PRDetail - Follow-up Review Trigger Integration', () => {
|
||||
hasCommitsAfterPosting: false,
|
||||
newCommitCount: 0
|
||||
});
|
||||
mockOnPostComment.mockResolvedValue(undefined);
|
||||
mockOnPostComment.mockResolvedValue(true);
|
||||
});
|
||||
|
||||
it('should display "Ready for Follow-up" status when new commits exist after posting', async () => {
|
||||
|
||||
@@ -52,6 +52,7 @@ interface UseGitHubPRsResult {
|
||||
postComment: (prNumber: number, body: string) => Promise<boolean>;
|
||||
mergePR: (prNumber: number, mergeMethod?: "merge" | "squash" | "rebase") => Promise<boolean>;
|
||||
assignPR: (prNumber: number, username: string) => Promise<boolean>;
|
||||
markReviewPosted: (prNumber: number) => Promise<void>;
|
||||
getReviewStateForPR: (prNumber: number) => {
|
||||
isReviewing: boolean;
|
||||
startedAt: string | null;
|
||||
@@ -557,6 +558,41 @@ export function useGitHubPRs(
|
||||
[projectId, fetchPRs]
|
||||
);
|
||||
|
||||
const markReviewPosted = useCallback(
|
||||
async (prNumber: number): Promise<void> => {
|
||||
if (!projectId) return;
|
||||
|
||||
// Persist to disk first
|
||||
const success = await window.electronAPI.github.markReviewPosted(projectId, prNumber);
|
||||
if (!success) return;
|
||||
|
||||
// Get the current timestamp for consistent update
|
||||
const postedAt = new Date().toISOString();
|
||||
|
||||
// Update the in-memory store
|
||||
const existingState = getPRReviewState(projectId, prNumber);
|
||||
if (existingState?.result) {
|
||||
// If we have the result loaded, update it with hasPostedFindings and postedAt
|
||||
usePRReviewStore.getState().setPRReviewResult(
|
||||
projectId,
|
||||
{ ...existingState.result, hasPostedFindings: true, postedAt },
|
||||
{ preserveNewCommitsCheck: true }
|
||||
);
|
||||
} else {
|
||||
// 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(
|
||||
projectId,
|
||||
result,
|
||||
{ preserveNewCommitsCheck: true }
|
||||
);
|
||||
}
|
||||
}
|
||||
},
|
||||
[projectId, getPRReviewState]
|
||||
);
|
||||
|
||||
return {
|
||||
prs,
|
||||
isLoading,
|
||||
@@ -585,6 +621,7 @@ export function useGitHubPRs(
|
||||
postComment,
|
||||
mergePR,
|
||||
assignPR,
|
||||
markReviewPosted,
|
||||
getReviewStateForPR,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -204,6 +204,7 @@ const browserMockAPI: ElectronAPI = {
|
||||
postPRComment: async () => true,
|
||||
mergePR: async () => true,
|
||||
assignPR: async () => true,
|
||||
markReviewPosted: async () => true,
|
||||
getPRReview: async () => null,
|
||||
getPRReviewsBatch: async () => ({}),
|
||||
deletePRReview: async () => true,
|
||||
|
||||
@@ -374,6 +374,7 @@ export const IPC_CHANNELS = {
|
||||
GITHUB_PR_FOLLOWUP_REVIEW: 'github:pr:followupReview',
|
||||
GITHUB_PR_CHECK_NEW_COMMITS: 'github:pr:checkNewCommits',
|
||||
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 Review events (main -> renderer)
|
||||
|
||||
Reference in New Issue
Block a user