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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2026-01-15 22:40:18 +01:00
parent 33014682f8
commit 32811142a2
5 changed files with 119 additions and 18 deletions
@@ -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<string>(
+            (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
@@ -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;
 }

 /**
@@ -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: (
-          <Button size="sm" variant="outline" onClick={onRunFollowupReview} className="ml-2 h-6 text-xs px-2">
-            {t('prReview.runFollowup')}
-          </Button>
-        )
-      });
+      // 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: (
+            <Button size="sm" variant="outline" onClick={onRunFollowupReview} className="ml-2 h-6 text-xs px-2">
+              {t('prReview.runFollowup')}
+            </Button>
+          )
+        });
+      } 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: (
+            <Button
+              size="sm"
+              variant="ghost"
+              onClick={onRunFollowupReview}
+              className="ml-2 h-6 text-xs px-2 text-muted-foreground hover:text-foreground"
+              title={t('prReview.runFollowupAnyway')}
+            >
+              {t('prReview.verifyAnyway')}
+            </Button>
+          )
+        });
+      }
    }
  }

@@ -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",
@@ -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.",