diff --git a/src-tauri/src/github.rs b/src-tauri/src/github.rs index ae23d33..d76bfaa 100644 --- a/src-tauri/src/github.rs +++ b/src-tauri/src/github.rs @@ -53,7 +53,8 @@ const PR_FRAGMENT: &str = r#"id mergeQueueEntry { state position - }"#; + } + mergeStateStatus"#; fn build_pr_query() -> String { let since = chrono::Utc::now() - chrono::Duration::days(14); @@ -166,6 +167,10 @@ fn parse_pr_node(node: &Value) -> Option { .unwrap_or("") .to_string(), is_review_requested: false, + merge_state_status: node + .get("mergeStateStatus") + .and_then(|v| v.as_str()) + .map(str::to_owned), }) } diff --git a/src-tauri/src/menu.rs b/src-tauri/src/menu.rs index b1fa91a..7201d1f 100644 --- a/src-tauri/src/menu.rs +++ b/src-tauri/src/menu.rs @@ -10,6 +10,14 @@ struct PrSection { default_collapsed: bool, } +/// Returns true when GitHub's merge-state check is satisfied (CLEAN or HAS_HOOKS). +fn is_actually_mergeable(pr: &PullRequest) -> bool { + matches!( + pr.merge_state_status.as_deref(), + Some("CLEAN") | Some("HAS_HOOKS") + ) +} + /// Port of src/lib/stores.ts groupPrs() matching its section ordering. fn group_prs(all_prs: &[PullRequest]) -> Vec { let review_requested: Vec<_> = all_prs @@ -86,11 +94,11 @@ fn group_prs(all_prs: &[PullRequest]) -> Vec { .iter() .filter(|pr| { pr.merge_queue_info.is_none() - && pr.check_status == CheckStatus::Success + && is_actually_mergeable(pr) + && pr.check_status != CheckStatus::Failure + && pr.check_status != CheckStatus::Error + && pr.check_status != CheckStatus::Pending && pr.review_decision.as_deref() != Some("CHANGES_REQUESTED") - && pr.review_decision.as_deref() != Some("APPROVED") - && (pr.review_decision.as_deref() == Some("REVIEW_REQUIRED") - || pr.review_decision.is_none()) }) .cloned() .collect(), @@ -116,11 +124,12 @@ fn group_prs(all_prs: &[PullRequest]) -> Vec { .iter() .filter(|pr| { pr.merge_queue_info.is_none() - && pr.check_status == CheckStatus::None + && !is_actually_mergeable(pr) + && pr.check_status != CheckStatus::Failure + && pr.check_status != CheckStatus::Error + && pr.check_status != CheckStatus::Pending && pr.review_decision.as_deref() != Some("CHANGES_REQUESTED") && pr.review_decision.as_deref() != Some("APPROVED") - && (pr.review_decision.as_deref() == Some("REVIEW_REQUIRED") - || pr.review_decision.is_none()) }) .cloned() .collect(), @@ -132,6 +141,7 @@ fn group_prs(all_prs: &[PullRequest]) -> Vec { .iter() .filter(|pr| { pr.merge_queue_info.is_none() + && !is_actually_mergeable(pr) && pr.check_status != CheckStatus::Failure && pr.check_status != CheckStatus::Error && pr.review_decision.as_deref() == Some("APPROVED") @@ -338,3 +348,157 @@ fn truncate(s: &str, max: usize) -> String { truncated } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::*; + + fn make_pr() -> PullRequest { + PullRequest { + id: "PR_1".into(), + number: 1, + title: "test".into(), + url: "https://github.com/test/repo/pull/1".into(), + state: PrState::Open, + repository: "repo".into(), + owner: "test".into(), + head_ref: "feature".into(), + base_ref: "main".into(), + check_status: CheckStatus::Success, + is_draft: false, + labels: vec![], + merge_queue_info: None, + created_at: "2025-01-01T00:00:00Z".into(), + updated_at: "2025-01-01T00:00:00Z".into(), + review_decision: None, + additions: 0, + deletions: 0, + comment_count: 0, + author_login: "user".into(), + author_avatar_url: "".into(), + is_review_requested: false, + merge_state_status: None, + } + } + + fn find_section<'a>(sections: &'a [PrSection], title: &str) -> Option<&'a PrSection> { + sections.iter().find(|s| s.title == title) + } + + /// Assert a PR appears in exactly one section with the given title and no others + /// (ignoring empty sections from group_prs output). + fn assert_only_in_section(sections: &[PrSection], expected_title: &str) { + let non_empty: Vec<_> = sections.iter().filter(|s| !s.prs.is_empty()).collect(); + assert_eq!( + non_empty.len(), + 1, + "Expected PR in exactly one section, but found {} non-empty sections: {:?}", + non_empty.len(), + non_empty.iter().map(|s| &s.title).collect::>() + ); + assert_eq!(non_empty[0].title, expected_title); + } + + #[test] + fn blocked_review_required_not_in_mergeable() { + let pr = PullRequest { + review_decision: Some("REVIEW_REQUIRED".into()), + merge_state_status: Some("BLOCKED".into()), + check_status: CheckStatus::Success, + ..make_pr() + }; + let sections = group_prs(&[pr]); + assert_only_in_section(§ions, "Waiting for Review"); + } + + #[test] + fn clean_pr_is_mergeable() { + let pr = PullRequest { + review_decision: None, + merge_state_status: Some("CLEAN".into()), + check_status: CheckStatus::Success, + ..make_pr() + }; + let sections = group_prs(&[pr]); + let mergeable = find_section(§ions, "Mergeable").expect("should be in Mergeable"); + assert_eq!(mergeable.prs.len(), 1); + assert_only_in_section(§ions, "Mergeable"); + } + + #[test] + fn has_hooks_pr_is_mergeable() { + let pr = PullRequest { + review_decision: None, + merge_state_status: Some("HAS_HOOKS".into()), + check_status: CheckStatus::Success, + ..make_pr() + }; + let sections = group_prs(&[pr]); + let mergeable = find_section(§ions, "Mergeable").expect("should be in Mergeable"); + assert_eq!(mergeable.prs.len(), 1); + assert_only_in_section(§ions, "Mergeable"); + } + + #[test] + fn approved_and_clean_is_mergeable() { + let pr = PullRequest { + review_decision: Some("APPROVED".into()), + merge_state_status: Some("CLEAN".into()), + check_status: CheckStatus::Success, + ..make_pr() + }; + let sections = group_prs(&[pr]); + assert_only_in_section(§ions, "Mergeable"); + } + + #[test] + fn approved_but_blocked_stays_in_approved() { + let pr = PullRequest { + review_decision: Some("APPROVED".into()), + merge_state_status: Some("BLOCKED".into()), + check_status: CheckStatus::Success, + ..make_pr() + }; + let sections = group_prs(&[pr]); + assert_only_in_section(§ions, "Approved"); + } + + #[test] + fn unknown_merge_state_not_mergeable() { + let pr = PullRequest { + review_decision: None, + merge_state_status: None, + check_status: CheckStatus::Success, + ..make_pr() + }; + let sections = group_prs(&[pr]); + assert_only_in_section(§ions, "Waiting for Review"); + } + + #[test] + fn review_requested_pr_excluded_from_authored_sections() { + let pr = PullRequest { + is_review_requested: true, + merge_state_status: Some("CLEAN".into()), + check_status: CheckStatus::Success, + review_decision: None, + ..make_pr() + }; + let sections = group_prs(&[pr]); + // Should only appear in "Needs Your Review" (review-requested bucket) + assert_only_in_section(§ions, "Needs Your Review"); + } + + #[test] + fn clean_pr_with_no_checks_is_mergeable() { + let pr = PullRequest { + review_decision: None, + merge_state_status: Some("CLEAN".into()), + check_status: CheckStatus::None, + ..make_pr() + }; + let sections = group_prs(&[pr]); + assert_only_in_section(§ions, "Mergeable"); + } +} diff --git a/src-tauri/src/models.rs b/src-tauri/src/models.rs index 6162c5e..8c7be2d 100644 --- a/src-tauri/src/models.rs +++ b/src-tauri/src/models.rs @@ -54,6 +54,7 @@ pub struct PullRequest { pub author_login: String, pub author_avatar_url: String, pub is_review_requested: bool, + pub merge_state_status: Option, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] diff --git a/src/lib/components.test.ts b/src/lib/components.test.ts index f85e7a6..83a56ea 100644 --- a/src/lib/components.test.ts +++ b/src/lib/components.test.ts @@ -36,6 +36,7 @@ const mockPr: PullRequest = { author_login: "testuser", author_avatar_url: "https://avatars.githubusercontent.com/u/1?v=4", is_review_requested: false, + merge_state_status: null, }; const mockUser: GitHubUser = { @@ -97,6 +98,70 @@ describe("groupPrs", () => { expect(sections.find(s => s.title === "Needs Your Review")?.prs).toHaveLength(1); expect(sections.find(s => s.title === "Approved")?.prs).toHaveLength(1); }); + + it("does NOT put review-blocked PR with passing checks in Mergeable (bug fix)", () => { + const pr = { ...mockPr, review_decision: "REVIEW_REQUIRED", merge_state_status: "BLOCKED" as const, check_status: "success" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Waiting for Review"); + }); + + it("puts a clean PR in Mergeable", () => { + const pr = { ...mockPr, review_decision: null, merge_state_status: "CLEAN" as const, check_status: "success" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Mergeable"); + }); + + it("puts a HAS_HOOKS PR in Mergeable", () => { + const pr = { ...mockPr, review_decision: null, merge_state_status: "HAS_HOOKS" as const, check_status: "success" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Mergeable"); + }); + + it("promotes approved + clean PR to Mergeable (not Approved)", () => { + const pr = { ...mockPr, review_decision: "APPROVED", merge_state_status: "CLEAN" as const, check_status: "success" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Mergeable"); + }); + + it("keeps approved + blocked PR in Approved", () => { + const pr = { ...mockPr, review_decision: "APPROVED", merge_state_status: "BLOCKED" as const, check_status: "success" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Approved"); + }); + + it("treats unknown merge state as not mergeable", () => { + const pr = { ...mockPr, review_decision: null, merge_state_status: null, check_status: "success" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Waiting for Review"); + }); + + it("puts a CLEAN PR with no checks in Mergeable (repos without required checks)", () => { + const pr = { ...mockPr, review_decision: null, merge_state_status: "CLEAN" as const, check_status: "none" as const }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Mergeable"); + }); + + it("excludes review-requested PRs from authored merge-state sections", () => { + const pr = { ...mockPr, is_review_requested: true, merge_state_status: "CLEAN" as const, check_status: "success" as const, review_decision: null }; + const sections = groupPrs([pr]); + const nonEmpty = sections.filter(s => s.prs.length > 0); + expect(nonEmpty).toHaveLength(1); + expect(nonEmpty[0].title).toBe("Needs Your Review"); + }); }); // --------------------------------------------------------------------------- diff --git a/src/lib/stores.ts b/src/lib/stores.ts index ba5c9a4..3e8d332 100644 --- a/src/lib/stores.ts +++ b/src/lib/stores.ts @@ -18,6 +18,16 @@ export const authenticated = writable(false); export const loading = writable(true); export const lastUpdated = writable(null); +/** Returns true when GitHub's merge-state check is satisfied (CLEAN or HAS_HOOKS). */ +function isActuallyMergeable(pr: PullRequest): boolean { + return pr.merge_state_status === "CLEAN" || pr.merge_state_status === "HAS_HOOKS"; +} + +/** Returns true when checks have failed or errored. */ +function isCheckFailed(pr: PullRequest): boolean { + return pr.check_status === "failure" || pr.check_status === "error"; +} + export function groupPrs(allPrs: PullRequest[]): PrSection[] { const reviewRequested = allPrs.filter(pr => pr.is_review_requested); const myPrs = allPrs.filter(pr => !pr.is_review_requested); @@ -43,7 +53,7 @@ export function groupPrs(allPrs: PullRequest[]): PrSection[] { icon: XCircle, prs: nonDraftOpen.filter(pr => pr.merge_queue_info == null && - (pr.check_status === "failure" || pr.check_status === "error") + isCheckFailed(pr) ), }, { @@ -51,7 +61,7 @@ export function groupPrs(allPrs: PullRequest[]): PrSection[] { icon: RotateCcw, prs: nonDraftOpen.filter(pr => pr.merge_queue_info == null && - pr.check_status !== "failure" && pr.check_status !== "error" && + !isCheckFailed(pr) && pr.review_decision === "CHANGES_REQUESTED" ), }, @@ -60,10 +70,10 @@ export function groupPrs(allPrs: PullRequest[]): PrSection[] { icon: CircleDot, prs: nonDraftOpen.filter(pr => pr.merge_queue_info == null && - pr.check_status === "success" && - pr.review_decision !== "CHANGES_REQUESTED" && - pr.review_decision !== "APPROVED" && - (pr.review_decision === "REVIEW_REQUIRED" || pr.review_decision == null) + isActuallyMergeable(pr) && + !isCheckFailed(pr) && + pr.check_status !== "pending" && + pr.review_decision !== "CHANGES_REQUESTED" ), }, { @@ -81,10 +91,11 @@ export function groupPrs(allPrs: PullRequest[]): PrSection[] { icon: Eye, prs: nonDraftOpen.filter(pr => pr.merge_queue_info == null && - pr.check_status === "none" && + !isActuallyMergeable(pr) && + !isCheckFailed(pr) && + pr.check_status !== "pending" && pr.review_decision !== "CHANGES_REQUESTED" && - pr.review_decision !== "APPROVED" && - (pr.review_decision === "REVIEW_REQUIRED" || pr.review_decision == null) + pr.review_decision !== "APPROVED" ), }, { @@ -92,7 +103,8 @@ export function groupPrs(allPrs: PullRequest[]): PrSection[] { icon: CheckCircle, prs: nonDraftOpen.filter(pr => pr.merge_queue_info == null && - pr.check_status !== "failure" && pr.check_status !== "error" && + !isActuallyMergeable(pr) && + !isCheckFailed(pr) && pr.review_decision === "APPROVED" ), }, diff --git a/src/lib/types.ts b/src/lib/types.ts index 69320c7..f2dc18d 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -2,6 +2,7 @@ import type { Component, ComponentType, SvelteComponent } from "svelte"; export type PrState = "open" | "closed" | "merged"; export type CheckStatus = "pending" | "success" | "failure" | "error" | "none"; +export type MergeStateStatus = "BEHIND" | "BLOCKED" | "CLEAN" | "DIRTY" | "DRAFT" | "HAS_HOOKS" | "UNKNOWN" | "UNSTABLE" | null; export interface MergeQueueInfo { state: string; @@ -36,6 +37,7 @@ export interface PullRequest { author_login: string; author_avatar_url: string; is_review_requested: boolean; + merge_state_status: MergeStateStatus; } export interface GitHubUser {