feat(web): showcase merge conflicts in the PR panel (#1151)
This commit is contained in:
@@ -6,7 +6,6 @@ import {
|
||||
IconRefresh,
|
||||
IconPlus,
|
||||
IconMinus,
|
||||
IconAlertTriangle,
|
||||
IconGitMerge,
|
||||
IconCheck,
|
||||
IconLoader2,
|
||||
@@ -21,7 +20,7 @@ import { useActiveTaskPR, useTaskPR } from "@/hooks/domains/github/use-task-pr";
|
||||
import { prPanelLabel } from "@/components/github/pr-utils";
|
||||
import { usePRFeedback } from "@/hooks/domains/github/use-pr-feedback";
|
||||
import { useGitHubStatus } from "@/hooks/domains/github/use-github-status";
|
||||
import { useCommentsStore } from "@/lib/state/slices/comments";
|
||||
import { useCommentsStore, isPRFeedbackComment } from "@/lib/state/slices/comments";
|
||||
import type { PRFeedbackComment } from "@/lib/state/slices/comments";
|
||||
import { useToast } from "@/components/toast-provider";
|
||||
import { submitPRReview } from "@/lib/api/domains/github-api";
|
||||
@@ -34,6 +33,7 @@ import {
|
||||
PRMarkdownBody,
|
||||
} from "./pr-shared";
|
||||
import { PRMergeButton } from "./pr-merge-button";
|
||||
import { PRMergeabilityNotice, buildConflictResolutionMessage } from "./pr-mergeability-notice";
|
||||
import { ReviewStateBadge } from "./pr-reviews-section";
|
||||
import { ChecksSection } from "./pr-checks-section";
|
||||
import { ReviewsSection } from "./pr-reviews-section";
|
||||
@@ -111,6 +111,65 @@ function useAddPRFeedbackAsContext(sessionId: string, prNumber: number) {
|
||||
return { addAsContext };
|
||||
}
|
||||
|
||||
// Sync live feedback data back to the store so topbar/other consumers stay up to date.
|
||||
// Use primitive deps to avoid re-render loops from object reference changes.
|
||||
// Guard: never regress the store to a less-terminal state (e.g. merged → open)
|
||||
// because the feedback fetch may return stale data from before a backend poll update.
|
||||
function useSyncLivePRState(taskPR: TaskPR, feedback: PRFeedback | null) {
|
||||
const setTaskPR = useAppStore((s) => s.setTaskPR);
|
||||
const prState = taskPR.state;
|
||||
const prMergedAt = taskPR.merged_at ?? null;
|
||||
const prClosedAt = taskPR.closed_at ?? null;
|
||||
const prAdditions = taskPR.additions;
|
||||
const prDeletions = taskPR.deletions;
|
||||
const prMergeableState = taskPR.mergeable_state;
|
||||
const prTaskId = taskPR.task_id;
|
||||
useEffect(() => {
|
||||
if (!feedback) return;
|
||||
const livePR = feedback.pr;
|
||||
// State priority: merged > closed > open. Never regress to a less-terminal state.
|
||||
const stateRank = (s: string) => {
|
||||
if (s === "merged") return 2;
|
||||
if (s === "closed") return 1;
|
||||
return 0;
|
||||
};
|
||||
const effectiveState = stateRank(livePR.state) >= stateRank(prState) ? livePR.state : prState;
|
||||
const effectiveMergedAt = effectiveState === prState ? prMergedAt : (livePR.merged_at ?? null);
|
||||
const effectiveClosedAt = effectiveState === prState ? prClosedAt : (livePR.closed_at ?? null);
|
||||
// Live mergeable_state is authoritative when present; otherwise keep the stored value.
|
||||
const effectiveMergeableState = livePR.mergeable_state ?? prMergeableState;
|
||||
if (
|
||||
effectiveState !== prState ||
|
||||
effectiveMergedAt !== prMergedAt ||
|
||||
effectiveClosedAt !== prClosedAt ||
|
||||
livePR.additions !== prAdditions ||
|
||||
livePR.deletions !== prDeletions ||
|
||||
effectiveMergeableState !== prMergeableState
|
||||
) {
|
||||
setTaskPR(prTaskId, {
|
||||
...taskPR,
|
||||
state: effectiveState as TaskPR["state"],
|
||||
additions: livePR.additions,
|
||||
deletions: livePR.deletions,
|
||||
merged_at: effectiveMergedAt,
|
||||
closed_at: effectiveClosedAt,
|
||||
mergeable_state: effectiveMergeableState,
|
||||
});
|
||||
}
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [
|
||||
feedback,
|
||||
prState,
|
||||
prMergedAt,
|
||||
prClosedAt,
|
||||
prAdditions,
|
||||
prDeletions,
|
||||
prMergeableState,
|
||||
prTaskId,
|
||||
setTaskPR,
|
||||
]);
|
||||
}
|
||||
|
||||
type PRPanelMetrics = {
|
||||
reviewCount: number;
|
||||
pendingReviewCount: number;
|
||||
@@ -251,51 +310,38 @@ function ApproveButton({
|
||||
export function PRDetailContent({ taskPR, sessionId }: { taskPR: TaskPR; sessionId: string }) {
|
||||
const { feedback, loading, refresh } = usePRFeedback(taskPR.owner, taskPR.repo, taskPR.pr_number);
|
||||
const { addAsContext } = useAddPRFeedbackAsContext(sessionId, taskPR.pr_number);
|
||||
const setTaskPR = useAppStore((s) => s.setTaskPR);
|
||||
|
||||
// Sync live feedback data back to the store so topbar/other consumers stay up to date.
|
||||
// Use primitive deps to avoid re-render loops from object reference changes.
|
||||
// Guard: never regress the store to a less-terminal state (e.g. merged → open)
|
||||
// because the feedback fetch may return stale data from before a backend poll update.
|
||||
const prState = taskPR.state;
|
||||
const prMergedAt = taskPR.merged_at ?? null;
|
||||
const prClosedAt = taskPR.closed_at ?? null;
|
||||
const prAdditions = taskPR.additions;
|
||||
const prDeletions = taskPR.deletions;
|
||||
const prTaskId = taskPR.task_id;
|
||||
useEffect(() => {
|
||||
if (!feedback) return;
|
||||
const livePR = feedback.pr;
|
||||
// State priority: merged > closed > open. Never regress to a less-terminal state.
|
||||
const stateRank = (s: string) => {
|
||||
if (s === "merged") return 2;
|
||||
if (s === "closed") return 1;
|
||||
return 0;
|
||||
};
|
||||
const effectiveState = stateRank(livePR.state) >= stateRank(prState) ? livePR.state : prState;
|
||||
const effectiveMergedAt = effectiveState === prState ? prMergedAt : (livePR.merged_at ?? null);
|
||||
const effectiveClosedAt = effectiveState === prState ? prClosedAt : (livePR.closed_at ?? null);
|
||||
if (
|
||||
effectiveState !== prState ||
|
||||
effectiveMergedAt !== prMergedAt ||
|
||||
effectiveClosedAt !== prClosedAt ||
|
||||
livePR.additions !== prAdditions ||
|
||||
livePR.deletions !== prDeletions
|
||||
) {
|
||||
setTaskPR(prTaskId, {
|
||||
...taskPR,
|
||||
state: effectiveState as TaskPR["state"],
|
||||
additions: livePR.additions,
|
||||
deletions: livePR.deletions,
|
||||
merged_at: effectiveMergedAt,
|
||||
closed_at: effectiveClosedAt,
|
||||
});
|
||||
}
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [feedback, prState, prMergedAt, prClosedAt, prAdditions, prDeletions, prTaskId, setTaskPR]);
|
||||
useSyncLivePRState(taskPR, feedback);
|
||||
|
||||
const metrics = derivePanelMetrics(taskPR, feedback);
|
||||
|
||||
// True once a conflict prompt for this PR is already queued — avoids piling
|
||||
// up identical instructions if the user clicks "Resolve conflicts" again.
|
||||
const conflictQueued = useCommentsStore((s) =>
|
||||
s.pendingForChat.some((id) => {
|
||||
const c = s.byId[id];
|
||||
return (
|
||||
!!c &&
|
||||
isPRFeedbackComment(c) &&
|
||||
c.feedbackType === "conflict" &&
|
||||
c.sessionId === sessionId &&
|
||||
c.prNumber === taskPR.pr_number
|
||||
);
|
||||
}),
|
||||
);
|
||||
|
||||
const onResolveConflicts = useCallback(() => {
|
||||
if (conflictQueued) return;
|
||||
addAsContext(
|
||||
"conflict",
|
||||
buildConflictResolutionMessage({
|
||||
prNumber: taskPR.pr_number,
|
||||
headBranch: taskPR.head_branch,
|
||||
baseBranch: taskPR.base_branch,
|
||||
}),
|
||||
);
|
||||
}, [addAsContext, conflictQueued, taskPR.pr_number, taskPR.head_branch, taskPR.base_branch]);
|
||||
|
||||
return (
|
||||
<div className="flex flex-col h-full">
|
||||
<PRHeader
|
||||
@@ -304,6 +350,8 @@ export function PRDetailContent({ taskPR, sessionId }: { taskPR: TaskPR; session
|
||||
metrics={metrics}
|
||||
loading={loading}
|
||||
onRefresh={refresh}
|
||||
onResolveConflicts={onResolveConflicts}
|
||||
conflictQueued={conflictQueued}
|
||||
/>
|
||||
<Separator />
|
||||
<ScrollArea className="flex-1 overflow-hidden">
|
||||
@@ -467,17 +515,23 @@ function PRHeader({
|
||||
metrics,
|
||||
loading,
|
||||
onRefresh,
|
||||
onResolveConflicts,
|
||||
conflictQueued,
|
||||
}: {
|
||||
taskPR: TaskPR;
|
||||
feedback: PRFeedback | null;
|
||||
metrics: PRPanelMetrics;
|
||||
loading: boolean;
|
||||
onRefresh: () => void;
|
||||
onResolveConflicts: () => void;
|
||||
conflictQueued: boolean;
|
||||
}) {
|
||||
const liveState = feedback?.pr.state ?? taskPR.state;
|
||||
const isDraft = feedback?.pr.draft ?? false;
|
||||
const isMergeable = feedback?.pr.mergeable ?? true;
|
||||
const showWarnings = !isDraft && !isMergeable && liveState === "open";
|
||||
// Prefer the live feedback state (refreshed by the panel's Refresh button);
|
||||
// fall back to the polled store value before feedback loads.
|
||||
const mergeableState = feedback?.pr.mergeable_state ?? taskPR.mergeable_state;
|
||||
|
||||
return (
|
||||
<div className="p-3 space-y-2">
|
||||
@@ -499,14 +553,15 @@ function PRHeader({
|
||||
{taskPR.base_branch}
|
||||
</code>
|
||||
</div>
|
||||
{showWarnings && (
|
||||
<div className="flex items-center gap-1.5 flex-wrap">
|
||||
<span className="flex items-center gap-1 text-[10px] text-yellow-600 dark:text-yellow-400">
|
||||
<IconAlertTriangle className="h-3 w-3" />
|
||||
Not mergeable
|
||||
</span>
|
||||
</div>
|
||||
)}
|
||||
<PRMergeabilityNotice
|
||||
state={mergeableState}
|
||||
mergeable={isMergeable}
|
||||
isDraft={isDraft}
|
||||
prState={liveState}
|
||||
baseBranch={taskPR.base_branch}
|
||||
onResolveConflicts={onResolveConflicts}
|
||||
resolveDisabled={conflictQueued}
|
||||
/>
|
||||
<HeaderDateLine taskPR={taskPR} />
|
||||
<HeaderStatsLine taskPR={taskPR} metrics={metrics} />
|
||||
</div>
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { describeMergeability, buildConflictResolutionMessage } from "./pr-mergeability-notice";
|
||||
import type { MergeableState } from "@/lib/types/github";
|
||||
|
||||
function describe_({
|
||||
state,
|
||||
mergeable = false,
|
||||
isDraft = false,
|
||||
prState = "open",
|
||||
}: {
|
||||
state: MergeableState | undefined;
|
||||
mergeable?: boolean;
|
||||
isDraft?: boolean;
|
||||
prState?: string;
|
||||
}) {
|
||||
return describeMergeability({ state, mergeable, isDraft, prState });
|
||||
}
|
||||
|
||||
describe("describeMergeability", () => {
|
||||
it("shows the conflict banner for a dirty PR", () => {
|
||||
expect(describe_({ state: "dirty" })).toEqual({ kind: "banner" });
|
||||
});
|
||||
|
||||
it("shows a 'Blocked' chip for a blocked PR (even when GitHub reports mergeable)", () => {
|
||||
expect(describe_({ state: "blocked", mergeable: true })).toEqual({
|
||||
kind: "chip",
|
||||
label: "Blocked",
|
||||
});
|
||||
});
|
||||
|
||||
it("shows a 'Behind base' chip for a behind PR", () => {
|
||||
expect(describe_({ state: "behind", mergeable: true })).toEqual({
|
||||
kind: "chip",
|
||||
label: "Behind base",
|
||||
});
|
||||
});
|
||||
|
||||
it("shows nothing for clean / unstable / has_hooks / draft", () => {
|
||||
expect(describe_({ state: "clean", mergeable: true })).toEqual({ kind: "none" });
|
||||
expect(describe_({ state: "unstable", mergeable: true })).toEqual({ kind: "none" });
|
||||
expect(describe_({ state: "has_hooks", mergeable: true })).toEqual({ kind: "none" });
|
||||
// "draft" enum on a non-draft PR object must not fall through to "Not mergeable".
|
||||
expect(describe_({ state: "draft", mergeable: false })).toEqual({ kind: "none" });
|
||||
});
|
||||
|
||||
it("falls back to generic text for unknown states that GitHub deems non-mergeable", () => {
|
||||
expect(describe_({ state: "unknown", mergeable: false })).toEqual({ kind: "text" });
|
||||
expect(describe_({ state: "", mergeable: false })).toEqual({ kind: "text" });
|
||||
expect(describe_({ state: undefined, mergeable: false })).toEqual({ kind: "text" });
|
||||
});
|
||||
|
||||
it("shows nothing for unknown states GitHub still considers mergeable", () => {
|
||||
expect(describe_({ state: "unknown", mergeable: true })).toEqual({ kind: "none" });
|
||||
});
|
||||
|
||||
it("suppresses every notice for draft PRs", () => {
|
||||
expect(describe_({ state: "dirty", isDraft: true })).toEqual({ kind: "none" });
|
||||
});
|
||||
|
||||
it("suppresses every notice for non-open PRs", () => {
|
||||
expect(describe_({ state: "dirty", prState: "merged" })).toEqual({ kind: "none" });
|
||||
expect(describe_({ state: "dirty", prState: "closed" })).toEqual({ kind: "none" });
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildConflictResolutionMessage", () => {
|
||||
it("names the PR and both branches", () => {
|
||||
const msg = buildConflictResolutionMessage({
|
||||
prNumber: 1132,
|
||||
headBranch: "feature/x",
|
||||
baseBranch: "main",
|
||||
});
|
||||
expect(msg).toContain("#1132");
|
||||
expect(msg).toContain("`feature/x`");
|
||||
expect(msg).toContain("`main`");
|
||||
expect(msg.toLowerCase()).toContain("conflict");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,155 @@
|
||||
"use client";
|
||||
|
||||
import { IconAlertTriangle } from "@tabler/icons-react";
|
||||
import { Button } from "@kandev/ui/button";
|
||||
import type { MergeableState } from "@/lib/types/github";
|
||||
|
||||
// --- Pure descriptor (unit-tested) ---
|
||||
|
||||
export type MergeabilityNotice =
|
||||
| { kind: "none" }
|
||||
| { kind: "banner" } // dirty / merge conflicts
|
||||
| { kind: "chip"; label: string } // blocked / behind base
|
||||
| { kind: "text" }; // generic non-mergeable fallback
|
||||
|
||||
/** Maps a PR's merge state to how prominently the panel surfaces it. */
|
||||
export function describeMergeability({
|
||||
state,
|
||||
mergeable,
|
||||
isDraft,
|
||||
prState,
|
||||
}: {
|
||||
state: MergeableState | undefined;
|
||||
mergeable: boolean;
|
||||
isDraft: boolean;
|
||||
prState: string;
|
||||
}): MergeabilityNotice {
|
||||
if (prState !== "open" || isDraft) return { kind: "none" };
|
||||
switch (state) {
|
||||
case "dirty":
|
||||
return { kind: "banner" };
|
||||
case "blocked":
|
||||
return { kind: "chip", label: "Blocked" };
|
||||
case "behind":
|
||||
return { kind: "chip", label: "Behind base" };
|
||||
case "clean":
|
||||
case "unstable":
|
||||
case "has_hooks":
|
||||
case "draft":
|
||||
// "draft" is gated above via isDraft, but handle the enum value too.
|
||||
return { kind: "none" };
|
||||
default:
|
||||
// unknown / "" / future states: defer to the legacy boolean signal.
|
||||
return mergeable === false ? { kind: "text" } : { kind: "none" };
|
||||
}
|
||||
}
|
||||
|
||||
/** Chat-context message sent to the agent when the user clicks "Resolve conflicts". */
|
||||
export function buildConflictResolutionMessage({
|
||||
prNumber,
|
||||
headBranch,
|
||||
baseBranch,
|
||||
}: {
|
||||
prNumber: number;
|
||||
headBranch: string;
|
||||
baseBranch: string;
|
||||
}): string {
|
||||
return (
|
||||
`PR #${prNumber} (\`${headBranch}\` → \`${baseBranch}\`) has merge conflicts and ` +
|
||||
`can't be merged automatically. Please merge \`${baseBranch}\` into \`${headBranch}\` ` +
|
||||
`(or rebase onto it) and resolve the conflicts.`
|
||||
);
|
||||
}
|
||||
|
||||
// --- Presentation ---
|
||||
|
||||
function ConflictBanner({
|
||||
baseBranch,
|
||||
onResolveConflicts,
|
||||
resolveDisabled,
|
||||
}: {
|
||||
baseBranch: string;
|
||||
onResolveConflicts?: () => void;
|
||||
resolveDisabled?: boolean;
|
||||
}) {
|
||||
return (
|
||||
<div
|
||||
data-testid="pr-conflict-banner"
|
||||
className="flex items-start gap-2.5 rounded-md border border-red-500/35 bg-red-500/10 px-3 py-2.5"
|
||||
>
|
||||
<IconAlertTriangle className="h-4 w-4 shrink-0 text-red-600 dark:text-red-400 mt-0.5" />
|
||||
<div className="min-w-0 flex-1">
|
||||
<div className="text-xs font-semibold text-red-600 dark:text-red-400">
|
||||
Merge conflicts with <code className="font-mono">{baseBranch}</code>
|
||||
</div>
|
||||
<p className="mt-0.5 text-[11px] leading-snug text-muted-foreground">
|
||||
This branch can't be merged automatically. Resolve the conflicts before merging.
|
||||
</p>
|
||||
{onResolveConflicts && (
|
||||
<Button
|
||||
size="sm"
|
||||
variant="outline"
|
||||
data-testid="pr-resolve-conflicts-button"
|
||||
className="mt-2 h-6 cursor-pointer px-2 text-[11px]"
|
||||
onClick={onResolveConflicts}
|
||||
disabled={resolveDisabled}
|
||||
>
|
||||
{resolveDisabled ? "Added to chat context" : "Resolve conflicts"}
|
||||
</Button>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function MergeabilityChip({ label }: { label: string }) {
|
||||
return (
|
||||
<span className="flex items-center gap-1 text-[10px] text-amber-600 dark:text-amber-400">
|
||||
<IconAlertTriangle className="h-3 w-3" />
|
||||
{label}
|
||||
</span>
|
||||
);
|
||||
}
|
||||
|
||||
function NotMergeableText() {
|
||||
return (
|
||||
<span className="flex items-center gap-1 text-[10px] text-yellow-600 dark:text-yellow-400">
|
||||
<IconAlertTriangle className="h-3 w-3" />
|
||||
Not mergeable
|
||||
</span>
|
||||
);
|
||||
}
|
||||
|
||||
export function PRMergeabilityNotice({
|
||||
state,
|
||||
mergeable,
|
||||
isDraft,
|
||||
prState,
|
||||
baseBranch,
|
||||
onResolveConflicts,
|
||||
resolveDisabled,
|
||||
}: {
|
||||
state: MergeableState | undefined;
|
||||
mergeable: boolean;
|
||||
isDraft: boolean;
|
||||
prState: string;
|
||||
baseBranch: string;
|
||||
onResolveConflicts?: () => void;
|
||||
resolveDisabled?: boolean;
|
||||
}) {
|
||||
const notice = describeMergeability({ state, mergeable, isDraft, prState });
|
||||
if (notice.kind === "none") return null;
|
||||
if (notice.kind === "banner")
|
||||
return (
|
||||
<ConflictBanner
|
||||
baseBranch={baseBranch}
|
||||
onResolveConflicts={onResolveConflicts}
|
||||
resolveDisabled={resolveDisabled}
|
||||
/>
|
||||
);
|
||||
return (
|
||||
<div className="flex items-center gap-1.5 flex-wrap">
|
||||
{notice.kind === "chip" ? <MergeabilityChip label={notice.label} /> : <NotMergeableText />}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -52,7 +52,7 @@ export type FileEditorComment = CommentBase & {
|
||||
export type PRFeedbackComment = CommentBase & {
|
||||
source: "pr-feedback";
|
||||
prNumber: number;
|
||||
feedbackType: "check" | "review" | "comment";
|
||||
feedbackType: "check" | "review" | "comment" | "conflict";
|
||||
/** Pre-formatted markdown text for display and sending */
|
||||
content: string;
|
||||
};
|
||||
|
||||
@@ -55,6 +55,9 @@ export type GitHubPR = {
|
||||
repo_name: string;
|
||||
draft: boolean;
|
||||
mergeable: boolean;
|
||||
/** Rich merge state (clean | blocked | behind | dirty | ...). Optional because
|
||||
* legacy payloads predate it; falls back to TaskPR.mergeable_state. */
|
||||
mergeable_state?: MergeableState;
|
||||
additions: number;
|
||||
deletions: number;
|
||||
requested_reviewers: RequestedReviewer[];
|
||||
|
||||
Reference in New Issue
Block a user