fix: only skip mergeability checks for merged PRs, not closed PRs
Addresses review comment: A closed-but-unmerged PR should still have its mergeability checked accurately. Only merged PRs should skip the checks since they're already merged. Changes: - SCM plugin: Only return clean status for state === "merged" - Dashboard: Show conflicts for closed PRs (pr.state !== "merged") - Tests: Updated to verify closed PRs still get checked Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
e0f4a9dc27
commit
696c365802
|
|
@ -506,11 +506,12 @@ function createGitHubSCM(): SCM {
|
|||
async getMergeability(pr: PRInfo): Promise<MergeReadiness> {
|
||||
const blockers: string[] = [];
|
||||
|
||||
// First, check if the PR is merged or closed
|
||||
// GitHub returns mergeable=null for merged/closed PRs, which is not useful
|
||||
// First, check if the PR is merged
|
||||
// GitHub returns mergeable=null for merged PRs, which is not useful
|
||||
// Note: We only skip checks for merged PRs. Closed PRs still need accurate status.
|
||||
const state = await this.getPRState(pr);
|
||||
if (state === "merged" || state === "closed") {
|
||||
// For merged/closed PRs, return a clean result without querying mergeable status
|
||||
if (state === "merged") {
|
||||
// For merged PRs, return a clean result without querying mergeable status
|
||||
return {
|
||||
mergeable: true,
|
||||
ciPassing: true,
|
||||
|
|
|
|||
|
|
@ -552,20 +552,18 @@ describe("scm-github plugin", () => {
|
|||
expect(ghMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("returns clean result for closed PRs without querying mergeable status", async () => {
|
||||
it("still checks mergeability for closed PRs (not merged)", async () => {
|
||||
// getPRState call
|
||||
mockGh({ state: "CLOSED" });
|
||||
// PR view (closed PRs still get checked)
|
||||
mockGh({ mergeable: "CONFLICTING", reviewDecision: "APPROVED", mergeStateStatus: "DIRTY", isDraft: false });
|
||||
// CI checks
|
||||
mockGh([]);
|
||||
|
||||
const result = await scm.getMergeability(pr);
|
||||
expect(result).toEqual({
|
||||
mergeable: true,
|
||||
ciPassing: true,
|
||||
approved: true,
|
||||
noConflicts: true,
|
||||
blockers: [],
|
||||
});
|
||||
// Should only call gh once (for getPRState), not for mergeable/CI
|
||||
expect(ghMock).toHaveBeenCalledTimes(1);
|
||||
expect(result.noConflicts).toBe(false);
|
||||
expect(result.blockers).toContain("Merge conflicts");
|
||||
// Closed PRs go through normal checks, unlike merged PRs
|
||||
});
|
||||
|
||||
it("returns mergeable when everything is clear", async () => {
|
||||
|
|
|
|||
|
|
@ -450,8 +450,8 @@ function IssuesList({ pr }: { pr: DashboardPR }) {
|
|||
});
|
||||
}
|
||||
|
||||
// Only show merge conflicts for open PRs (merged/closed PRs don't have mergeable status)
|
||||
if (pr.state === "open" && !pr.mergeability.noConflicts) {
|
||||
// Only show merge conflicts for open/closed PRs (merged PRs don't have meaningful mergeable status)
|
||||
if (pr.state !== "merged" && !pr.mergeability.noConflicts) {
|
||||
issues.push({
|
||||
icon: "\u2717",
|
||||
color: "var(--color-accent-red)",
|
||||
|
|
|
|||
Loading…
Reference in New Issue