From 32811142a26113305e9c50c3b2bb56ee0243946d Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Thu, 15 Jan 2026 22:40:18 +0100 Subject: [PATCH] feat(pr-review): add fast-path detection for merge commits without finding overlap (#1145) * feat(pr-review): add fast-path detection for merge commits without finding overlap When merging develop/main into a PR branch, the system now checks if the merged files overlap with files that had findings from the review: - If overlap: Shows warning "X new commits (Y files with findings modified)" with prominent "Verify Changes" button - If no overlap: Shows success "Branch synced (X commits from base)" with optional "Verify" button for manual follow-up This reduces unnecessary "Ready for Follow-up" prompts when syncing branches with the base branch, improving the review workflow rhythm. Changes: - Extended NewCommitsCheck interface with overlap detection fields - Added merge commit detection and file overlap logic in checkNewCommits - Updated ReviewStatusTree UI to show appropriate status based on overlap - Added i18n translations for new UI states (en/fr) Co-Authored-By: Claude Opus 4.5 * fix(frontend): address code review findings for merge commit detection - Add missing fields to NewCommitsCheck interface (hasOverlapWithFindings, overlappingFiles, isMergeFromBase) to match github-api.ts definition - Broaden merge detection regex to /^merge\s+/i to catch more patterns like "Merge develop into feature-branch" and GitHub's Update branch button - Fix i18n pluralization issue by using "file(s)" format to avoid commit/file count mismatch in both EN and FR locales - Add clarifying comment for intentional omission of overlap fields in force push error path Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- .../main/ipc-handlers/github/pr-handlers.ts | 51 ++++++++++++++- .../src/preload/api/modules/github-api.ts | 6 ++ .../components/ReviewStatusTree.tsx | 62 ++++++++++++++----- .../src/shared/i18n/locales/en/common.json | 9 +++ .../src/shared/i18n/locales/fr/common.json | 9 +++ 5 files changed, 119 insertions(+), 18 deletions(-) diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index 6fa3c96a..cbd45da5 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -172,6 +172,12 @@ export interface NewCommitsCheck { currentHeadCommit?: string; /** Whether new commits happened AFTER findings were posted (for "Ready for Follow-up" status) */ hasCommitsAfterPosting?: boolean; + /** Whether new commits touch files that had findings (requires verification) */ + hasOverlapWithFindings?: boolean; + /** Files from new commits that overlap with finding files */ + overlappingFiles?: string[]; + /** Whether this appears to be a merge from base branch (develop/main) */ + isMergeFromBase?: boolean; } /** @@ -2073,14 +2079,18 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v // Try to get detailed comparison try { - // Get comparison to count new commits + // Get comparison to count new commits and see what files changed const comparison = (await githubFetch( config.token, `/repos/${config.repo}/compare/${reviewedCommitSha}...${currentHeadSha}` )) as { ahead_by?: number; total_commits?: number; - commits?: Array<{ commit: { committer: { date: string } } }>; + commits?: Array<{ + commit: { committer: { date: string }; message: string }; + parents?: Array<{ sha: string }>; + }>; + files?: Array<{ filename: string }>; }; // Check if findings have been posted and if new commits are after the posting date @@ -2107,12 +2117,46 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v hasCommitsAfterPosting = false; } + // Check if this looks like a merge from base branch (develop/main) + // Merge commits always have 2+ parents, so we check for that AND a merge-like message + // Pattern matches: "Merge branch", "Merge pull request", "Merge remote-tracking", + // "Merge 'develop' into", "Merge develop into", GitHub's "Update branch" button, etc. + const isMergeFromBase = comparison.commits?.some((c) => { + const hasTwoParents = (c.parents?.length ?? 0) >= 2; + const isMergeMessage = /^merge\s+/i.test(c.commit.message); + return hasTwoParents && isMergeMessage; + }) ?? false; + + // Get files that had findings from the review + const findingFiles = new Set( + (review.findings || []).map((f) => f.file).filter(Boolean) + ); + + // Get files changed in the new commits + const newCommitFiles = (comparison.files || []).map((f) => f.filename); + + // Check for overlap between new commit files and finding files + const overlappingFiles = newCommitFiles.filter((f) => findingFiles.has(f)); + const hasOverlapWithFindings = overlappingFiles.length > 0; + + debugLog("File overlap check", { + prNumber, + findingFilesCount: findingFiles.size, + newCommitFilesCount: newCommitFiles.length, + overlappingFiles, + hasOverlapWithFindings, + isMergeFromBase, + }); + return { hasNewCommits: true, newCommitCount: comparison.ahead_by || comparison.total_commits || 1, lastReviewedCommit: reviewedCommitSha, currentHeadCommit: currentHeadSha, hasCommitsAfterPosting, + hasOverlapWithFindings, + overlappingFiles: overlappingFiles.length > 0 ? overlappingFiles : undefined, + isMergeFromBase, }; } catch (error) { // Comparison failed (e.g., force push made old commit unreachable) @@ -2126,6 +2170,9 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v error: error instanceof Error ? error.message : error, } ); + // Note: hasOverlapWithFindings, overlappingFiles, isMergeFromBase intentionally omitted + // since we can't determine them without the comparison API. UI defaults to safe behavior + // (hasOverlapWithFindings ?? true) which prompts user to verify. return { hasNewCommits: true, newCommitCount: 1, // Unknown count due to force push diff --git a/apps/frontend/src/preload/api/modules/github-api.ts b/apps/frontend/src/preload/api/modules/github-api.ts index 6481f820..6408479f 100644 --- a/apps/frontend/src/preload/api/modules/github-api.ts +++ b/apps/frontend/src/preload/api/modules/github-api.ts @@ -378,6 +378,12 @@ export interface NewCommitsCheck { currentHeadCommit?: string; /** Whether new commits happened AFTER findings were posted (for "Ready for Follow-up" status) */ hasCommitsAfterPosting?: boolean; + /** Whether new commits touch files that had findings (requires verification) */ + hasOverlapWithFindings?: boolean; + /** Files from new commits that overlap with finding files */ + overlappingFiles?: string[]; + /** Whether this appears to be a merge from base branch (develop/main) */ + isMergeFromBase?: boolean; } /** diff --git a/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx b/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx index 7c3d5562..986f6187 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx @@ -182,22 +182,52 @@ export function ReviewStatusTree({ // This prevents showing follow-up prompts when initial review was never posted to GitHub const hasPostedFindings = postedCount > 0 || reviewResult?.hasPostedFindings; if (!isReviewing && hasPostedFindings && newCommitsCheck?.hasNewCommits && newCommitsCheck?.hasCommitsAfterPosting) { - steps.push({ - id: 'new_commits', - label: t('prReview.newCommits', { count: newCommitsCheck.newCommitCount }), - status: 'alert', - date: null - }); - steps.push({ - id: 'followup', - label: t('prReview.readyForFollowup'), - status: 'pending', - action: ( - - ) - }); + // Check if new commits overlap with files that had findings + const hasOverlap = newCommitsCheck.hasOverlapWithFindings ?? true; // Default to true for safety + + if (hasOverlap) { + // Files with findings were modified - need verification + steps.push({ + id: 'new_commits', + label: t('prReview.newCommitsOverlap', { + count: newCommitsCheck.newCommitCount, + files: newCommitsCheck.overlappingFiles?.length ?? 0 + }), + status: 'alert', + date: null + }); + steps.push({ + id: 'followup', + label: t('prReview.verifyChanges'), + status: 'pending', + action: ( + + ) + }); + } else { + // No overlap - branch synced, previous review still valid + steps.push({ + id: 'branch_synced', + label: newCommitsCheck.isMergeFromBase + ? t('prReview.branchSynced', { count: newCommitsCheck.newCommitCount }) + : t('prReview.newCommitsNoOverlap', { count: newCommitsCheck.newCommitCount }), + status: 'completed', + date: null, + action: ( + + ) + }); + } } } diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index c2608a43..5debc3c1 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -339,6 +339,15 @@ "failedPostCleanReview": "Failed to post clean review", "viewErrorDetails": "View details", "hideErrorDetails": "Hide details", + "branchSynced": "Branch synced ({{count}} commit from base)", + "branchSynced_plural": "Branch synced ({{count}} commits from base)", + "newCommitsOverlap": "{{count}} new commit ({{files}} finding file(s) modified)", + "newCommitsOverlap_plural": "{{count}} new commits ({{files}} finding file(s) modified)", + "newCommitsNoOverlap": "{{count}} new commit (no overlap with findings)", + "newCommitsNoOverlap_plural": "{{count}} new commits (no overlap with findings)", + "verifyChanges": "Verify Changes", + "verifyAnyway": "Verify", + "runFollowupAnyway": "Run follow-up verification even though no files overlap", "logs": { "agentActivity": "Agent Activity", "showMore": "Show {{count}} more", diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index 78ab5609..8ce1ee93 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -305,6 +305,15 @@ "blockingIssuesDesc_plural": "{{resolved}} résolus, {{unresolved}} problèmes bloquants encore ouverts.", "newCommitsSinceReview": "{{count}} nouveau commit depuis la révision. Lancez un suivi pour vérifier.", "newCommitsSinceReview_plural": "{{count}} nouveaux commits depuis la révision. Lancez un suivi pour vérifier.", + "branchSynced": "Branche synchronisée ({{count}} commit de la base)", + "branchSynced_plural": "Branche synchronisée ({{count}} commits de la base)", + "newCommitsOverlap": "{{count}} nouveau commit ({{files}} fichier(s) avec résultats modifié(s))", + "newCommitsOverlap_plural": "{{count}} nouveaux commits ({{files}} fichier(s) avec résultats modifié(s))", + "newCommitsNoOverlap": "{{count}} nouveau commit (aucun chevauchement avec les résultats)", + "newCommitsNoOverlap_plural": "{{count}} nouveaux commits (aucun chevauchement avec les résultats)", + "verifyChanges": "Vérifier les modifications", + "verifyAnyway": "Vérifier", + "runFollowupAnyway": "Lancer la vérification de suivi même si aucun fichier ne chevauche", "noBlockingIssues": "Aucun problème bloquant trouvé. Cette PR peut être fusionnée.", "findingsPostedWaiting": "{{count}} résultat publié. En attente des modifications du contributeur.", "findingsPostedWaiting_plural": "{{count}} résultats publiés. En attente des modifications du contributeur.",