Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions apps/frontend/src/preload/api/modules/github-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>>;

Expand Down Expand Up @@ -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),

Expand Down
11 changes: 9 additions & 2 deletions apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
postComment,
mergePR,
assignPR,
markReviewPosted,
refresh,
loadMore,
isConnected,
Expand Down Expand Up @@ -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]
);
Expand Down Expand Up @@ -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} />;
Expand Down Expand Up @@ -259,6 +265,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
onMergePR={handleMergePR}
onAssignPR={handleAssignPR}
onGetLogs={handleGetLogs}
onMarkReviewPosted={handleMarkReviewPosted}
/>
) : (
<EmptyState message={t("prReview.selectPRToView")} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -89,6 +90,7 @@ export function PRDetail({
onMergePR,
onAssignPR: _onAssignPR,
onGetLogs,
onMarkReviewPosted,
}: PRDetailProps) {
const { t } = useTranslation('common');
// Selection state for findings
Expand Down Expand Up @@ -780,12 +782,17 @@ ${reviewResult.summary}

${t('prReview.blockedStatusMessageFooter')}`;

await Promise.resolve(onPostComment(blockedStatusMessage));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve handler silently fails on post error

Low Severity

The handleApprove function also doesn't check the success return value from onPostComment after the type change from void to Promise<boolean>. When posting the approval comment fails and returns false, the function silently completes with no error feedback to the user. The button loading state ends but the user has no indication that the approval wasn't actually posted to GitHub. This is the same incomplete pattern as handlePostCleanReview, though with less severe user impact since there's no false success message displayed.

Fix in Cursor Fix in Web

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);

This comment was marked as outdated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store update skipped when user switches PRs during posting

Medium Severity

The onMarkReviewPosted call is placed inside the pr.number === currentPr check, which means if the user switches to a different PR while the comment is being posted, the store and disk are never updated with has_posted_findings = true - even though the comment was successfully posted to GitHub. The code comment explicitly mentions "Pass prNumber explicitly to avoid race conditions with PR selection changes," but the guard prevents this from working. The result is stale data: the PR list shows "not posted" even after a successful post, and this persists through app restarts since the disk file isn't updated.

Fix in Cursor Fix in Web

} else if (!success && pr.number === currentPr) {
setBlockedStatusError('Failed to post comment');
}
} catch (err) {
console.error('Failed to post blocked status comment:', err);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The markReviewPosted function performs a synchronous state update. To make the code clearer and avoid unnecessary asynchronicity, its type should be (prNumber: number) => void; instead of returning a Promise.

Suggested change
markReviewPosted: (prNumber: number) => Promise<void>;
markReviewPosted: (prNumber: number) => void;

getReviewStateForPR: (prNumber: number) => {
isReviewing: boolean;
startedAt: string | null;
Expand Down Expand Up @@ -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 }
);
}
}
Comment on lines +589 to +591

This comment was marked as outdated.

},
[projectId, getPRReviewState]
);

return {
prs,
isLoading,
Expand Down Expand Up @@ -585,6 +621,7 @@ export function useGitHubPRs(
postComment,
mergePR,
assignPR,
markReviewPosted,
getReviewStateForPR,
};
}
1 change: 1 addition & 0 deletions apps/frontend/src/renderer/lib/browser-mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions apps/frontend/src/shared/constants/ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading