Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src-tauri/src/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -166,6 +167,10 @@ fn parse_pr_node(node: &Value) -> Option<PullRequest> {
.unwrap_or("")
.to_string(),
is_review_requested: false,
merge_state_status: node
.get("mergeStateStatus")
.and_then(|v| v.as_str())
.map(str::to_owned),
})
}

Expand Down
178 changes: 171 additions & 7 deletions src-tauri/src/menu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PrSection> {
let review_requested: Vec<_> = all_prs
Expand Down Expand Up @@ -86,11 +94,11 @@ fn group_prs(all_prs: &[PullRequest]) -> Vec<PrSection> {
.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(),
Expand All @@ -116,11 +124,12 @@ fn group_prs(all_prs: &[PullRequest]) -> Vec<PrSection> {
.iter()
.filter(|pr| {
pr.merge_queue_info.is_none()
&& pr.check_status == CheckStatus::None
&& !is_actually_mergeable(pr)
Comment thread
ibetitsmike marked this conversation as resolved.
&& 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(),
Expand All @@ -132,6 +141,7 @@ fn group_prs(all_prs: &[PullRequest]) -> Vec<PrSection> {
.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")
Expand Down Expand Up @@ -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::<Vec<_>>()
);
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(&sections, "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(&sections, "Mergeable").expect("should be in Mergeable");
assert_eq!(mergeable.prs.len(), 1);
assert_only_in_section(&sections, "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(&sections, "Mergeable").expect("should be in Mergeable");
assert_eq!(mergeable.prs.len(), 1);
assert_only_in_section(&sections, "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(&sections, "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(&sections, "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(&sections, "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(&sections, "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(&sections, "Mergeable");
}
}
1 change: 1 addition & 0 deletions src-tauri/src/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
Expand Down
65 changes: 65 additions & 0 deletions src/lib/components.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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");
});
});

// ---------------------------------------------------------------------------
Expand Down
32 changes: 22 additions & 10 deletions src/lib/stores.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@ export const authenticated = writable(false);
export const loading = writable(true);
export const lastUpdated = writable<Date | null>(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);
Expand All @@ -43,15 +53,15 @@ 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)
),
},
{
title: "Changes Requested",
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"
),
},
Expand All @@ -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"
),
},
{
Expand All @@ -81,18 +91,20 @@ export function groupPrs(allPrs: PullRequest[]): PrSection[] {
icon: Eye,
prs: nonDraftOpen.filter(pr =>
pr.merge_queue_info == null &&
pr.check_status === "none" &&
!isActuallyMergeable(pr) &&
Comment thread
ibetitsmike marked this conversation as resolved.
!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"
),
},
{
title: "Approved",
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"
),
},
Expand Down
Loading
Loading