fix(web): show open PR status when task has merged and open PRs (#1322)
This commit is contained in:
@@ -1,6 +1,7 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
aggregatePRStatusColor,
|
||||
areAllOpenPRsReadyToMerge,
|
||||
getPRStatusColor,
|
||||
getPRTooltip,
|
||||
isPRAwaitingReview,
|
||||
@@ -421,6 +422,68 @@ describe("aggregatePRStatusColor", () => {
|
||||
const merged = makePR({ state: "merged" });
|
||||
expect(aggregatePRStatusColor([merged, pending])).toBe("text-yellow-500");
|
||||
});
|
||||
|
||||
it("ignores a merged PR when a fresh open PR has no status yet", () => {
|
||||
// Repro: first PR merged, then a new branch + PR opened on the same task.
|
||||
// The open PR has no checks/reviews yet so its color rank ties merged at
|
||||
// 0, but the icon must reflect the live PR, not the closed one.
|
||||
const merged = makePR({ state: "merged" });
|
||||
const fresh = makePR({ state: "open", review_state: "", checks_state: "" });
|
||||
expect(aggregatePRStatusColor([merged, fresh])).toBe("text-muted-foreground");
|
||||
});
|
||||
|
||||
it("ignores a closed PR when a fresh open PR is present", () => {
|
||||
// Closed PRs rank red (5, the highest), so without filtering they would
|
||||
// dominate every open sibling. Same reasoning as the merged case — a
|
||||
// terminal PR shouldn't drive the live status indicator.
|
||||
const closed = makePR({ state: "closed" });
|
||||
const fresh = makePR({ state: "open", review_state: "", checks_state: "" });
|
||||
expect(aggregatePRStatusColor([closed, fresh])).toBe("text-muted-foreground");
|
||||
});
|
||||
|
||||
it("falls back to terminal state when every PR is merged/closed", () => {
|
||||
// All-terminal tasks bypass the open-PR filter and rank across every PR,
|
||||
// so the result depends on which terminal state is present. Merged ranks
|
||||
// 0 (purple), closed ranks 5 (red) — both paths need a guard.
|
||||
const merged = makePR({ state: "merged" });
|
||||
expect(aggregatePRStatusColor([merged, merged])).toBe("text-purple-500");
|
||||
const closed = makePR({ state: "closed" });
|
||||
expect(aggregatePRStatusColor([closed, closed])).toBe("text-red-500");
|
||||
});
|
||||
});
|
||||
|
||||
describe("areAllOpenPRsReadyToMerge", () => {
|
||||
const ready = () =>
|
||||
makePR({
|
||||
state: "open",
|
||||
review_state: "approved",
|
||||
checks_state: "success",
|
||||
mergeable_state: "clean",
|
||||
});
|
||||
|
||||
it("is false when the list is empty", () => {
|
||||
expect(areAllOpenPRsReadyToMerge([])).toBe(false);
|
||||
});
|
||||
|
||||
it("is false when no PR is open", () => {
|
||||
// A fully-merged task isn't "ready to merge" — there's nothing left to do.
|
||||
expect(areAllOpenPRsReadyToMerge([makePR({ state: "merged" })])).toBe(false);
|
||||
});
|
||||
|
||||
it("is true when the only open PR is ready, even with a merged sibling", () => {
|
||||
// The fix: a merged sibling used to drag the result to false because
|
||||
// prs.every(isPRReadyToMerge) saw the merged PR as not-ready.
|
||||
expect(areAllOpenPRsReadyToMerge([makePR({ state: "merged" }), ready()])).toBe(true);
|
||||
});
|
||||
|
||||
it("is false when any open PR is not ready", () => {
|
||||
const pending = makePR({ state: "open", checks_state: "pending" });
|
||||
expect(areAllOpenPRsReadyToMerge([ready(), pending])).toBe(false);
|
||||
});
|
||||
|
||||
it("is true when every open PR is ready", () => {
|
||||
expect(areAllOpenPRsReadyToMerge([ready(), ready()])).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("getPRTooltip", () => {
|
||||
|
||||
@@ -89,13 +89,18 @@ export function getPRTooltip(pr: TaskPR): string {
|
||||
|
||||
/**
|
||||
* Picks the most attention-worthy color across N PRs. For multi-repo tasks one
|
||||
* red PR should dominate the visual even if the others are green.
|
||||
* red PR should dominate the visual even if the others are green. Terminal
|
||||
* (merged/closed) PRs are dropped when at least one PR is still open so a
|
||||
* task whose first PR landed and was followed by a new open PR surfaces the
|
||||
* live PR's status instead of the merged-purple from the closed one.
|
||||
*/
|
||||
export function aggregatePRStatusColor(prs: TaskPR[]): string {
|
||||
if (prs.length === 0) return "text-muted-foreground";
|
||||
const open = prs.filter((p) => p.state === "open");
|
||||
const target = open.length > 0 ? open : prs;
|
||||
let bestColor = "text-muted-foreground";
|
||||
let bestRank = -1;
|
||||
for (const pr of prs) {
|
||||
for (const pr of target) {
|
||||
const color = getPRStatusColor(pr);
|
||||
const rank = STATUS_RANK[color] ?? 0;
|
||||
if (rank > bestRank) {
|
||||
@@ -106,6 +111,16 @@ export function aggregatePRStatusColor(prs: TaskPR[]): string {
|
||||
return bestColor;
|
||||
}
|
||||
|
||||
/**
|
||||
* True when at least one PR is open AND every open PR is ready to merge.
|
||||
* Terminal (merged/closed) siblings are ignored so they can't drag the result
|
||||
* to false. Extracted so the rule is testable without mounting MultiPRIcon.
|
||||
*/
|
||||
export function areAllOpenPRsReadyToMerge(prs: TaskPR[]): boolean {
|
||||
const openPRs = prs.filter((p) => p.state === "open");
|
||||
return openPRs.length > 0 && openPRs.every(isPRReadyToMerge);
|
||||
}
|
||||
|
||||
export function PRTaskIcon({ taskId }: { taskId: string }) {
|
||||
const prs = useAppStore((state) => state.taskPRs.byTaskId[taskId] ?? null);
|
||||
|
||||
@@ -138,7 +153,7 @@ function SinglePRIcon({ taskId, pr }: { taskId: string; pr: TaskPR }) {
|
||||
|
||||
function MultiPRIcon({ taskId, prs }: { taskId: string; prs: TaskPR[] }) {
|
||||
const aggregateColor = aggregatePRStatusColor(prs);
|
||||
const allReady = prs.every(isPRReadyToMerge);
|
||||
const allReady = areAllOpenPRsReadyToMerge(prs);
|
||||
return (
|
||||
<Tooltip>
|
||||
<TooltipTrigger asChild>
|
||||
|
||||
Reference in New Issue
Block a user