From 1af32e1e020cd82938214047fc120542e6724bd9 Mon Sep 17 00:00:00 2001 From: sashml Date: Wed, 1 Jul 2026 09:29:30 +0200 Subject: [PATCH 1/3] feat(pr): add merge-state column to PR tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a colorblind-safe "State" column to the All PRs / Inbox / per-repo tables showing GitHub's mergeable status (✓ ok / ✗ cf / ? unknown). - queries: request mergeable + mergeStateStatus in search & repo PR queries - models: PullRequest.mergeable + merge_state_status (Option, #[serde(default)] so older cache entries still deserialize) - graphql: parse both fields; UNKNOWN/absent -> None (search API is lazy) - ui: State column with theme colors, widths adjusted - tests: parse roundtrip, CONFLICTING, and legacy-cache serde-default coverage Refs: #1, zombo-sash-eco-rpjg --- src/github/graphql.rs | 2 ++ src/github/models.rs | 10 ++++++ src/github/queries.rs | 4 +++ src/ui/theme.rs | 6 ++++ src/ui/widgets.rs | 21 +++++++++++++ tests/graphql_parse_tests.rs | 61 ++++++++++++++++++++++++++++++++++++ tests/state_tests.rs | 2 ++ 7 files changed, 106 insertions(+) diff --git a/src/github/graphql.rs b/src/github/graphql.rs index 810c778..68ce2bf 100644 --- a/src/github/graphql.rs +++ b/src/github/graphql.rs @@ -307,6 +307,8 @@ fn parse_search_pr(node: &Value) -> PullRequest { additions: node["additions"].as_u64().unwrap_or(0) as u32, deletions: node["deletions"].as_u64().unwrap_or(0) as u32, review_decision: node["reviewDecision"].as_str().map(|s| s.to_string()), + mergeable: node["mergeable"].as_str().map(|s| s.to_string()), + merge_state_status: node["mergeStateStatus"].as_str().map(|s| s.to_string()), labels, } } diff --git a/src/github/models.rs b/src/github/models.rs index f64561a..db08497 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -38,6 +38,16 @@ pub struct PullRequest { pub deletions: u32, pub review_decision: Option, pub labels: Vec, + /// GitHub `mergeable` enum: `MERGEABLE` / `CONFLICTING` / `UNKNOWN`. + /// `None` when absent (e.g. older cache entries). Note: GitHub computes this + /// lazily, so the search API frequently returns `UNKNOWN`. + #[serde(default)] + pub mergeable: Option, + /// GitHub `mergeStateStatus` enum: `CLEAN` / `DIRTY` / `BLOCKED` / `BEHIND` / + /// `UNSTABLE` / `HAS_HOOKS` / `DRAFT` / `UNKNOWN`. Richer than `mergeable`; + /// same lazy-compute caveat. + #[serde(default)] + pub merge_state_status: Option, } impl PullRequest { diff --git a/src/github/queries.rs b/src/github/queries.rs index cc4ca89..6cd7321 100644 --- a/src/github/queries.rs +++ b/src/github/queries.rs @@ -87,6 +87,8 @@ query($owner: String!, $name: String!, $cursor: String) { additions deletions reviewDecision + mergeable + mergeStateStatus labels(first: 10) { nodes { name } } @@ -124,6 +126,8 @@ query($query: String!, $cursor: String) { additions deletions reviewDecision + mergeable + mergeStateStatus labels(first: 10) { nodes { name } } diff --git a/src/ui/theme.rs b/src/ui/theme.rs index 4be722b..7682f2c 100644 --- a/src/ui/theme.rs +++ b/src/ui/theme.rs @@ -34,3 +34,9 @@ pub const NAV_VIRTUAL: Style = Style::new().fg(Color::Magenta).add_modifier(Modi pub const PR_NUMBER: Style = Style::new().fg(Color::Cyan); pub const PR_AUTHOR: Style = Style::new().fg(Color::Yellow); + +// Merge-state column. Color is paired with a distinct glyph in the widget so the +// signal survives colorblindness and monochrome terminals. +pub const MERGE_CLEAN: Style = Style::new().fg(Color::Green); + +pub const MERGE_CONFLICT: Style = Style::new().fg(Color::Red).add_modifier(Modifier::BOLD); diff --git a/src/ui/widgets.rs b/src/ui/widgets.rs index 313be2f..8ebe05f 100644 --- a/src/ui/widgets.rs +++ b/src/ui/widgets.rs @@ -6,6 +6,7 @@ use ratatui::{ }; use crate::app::state::{AppState, ContentView, FocusedPane, NavNode}; +use crate::github::models::PullRequest; use crate::ui::theme; use crate::util::time::relative_time; @@ -114,6 +115,17 @@ pub fn render_content_pane(f: &mut Frame, area: Rect, state: &AppState) { } } +/// Compact, colorblind-safe label + color for a PR's merge state. +/// Driven by GitHub's `mergeable` enum; `UNKNOWN`/absent renders as a dim `?` +/// because the search API computes `mergeable` lazily (often `UNKNOWN` at first). +fn merge_state_display(pr: &PullRequest) -> (&'static str, ratatui::style::Style) { + match pr.mergeable.as_deref() { + Some("MERGEABLE") => ("✓ ok", theme::MERGE_CLEAN), + Some("CONFLICTING") => ("✗ cf", theme::MERGE_CONFLICT), + _ => ("?", theme::DIM), + } +} + fn render_pr_table( f: &mut Frame, area: Rect, @@ -151,6 +163,7 @@ fn render_pr_table( let header = Row::new(vec![ Cell::from("#").style(theme::HEADER), + Cell::from("State").style(theme::HEADER), Cell::from("Title").style(theme::HEADER), Cell::from("Author").style(theme::HEADER), Cell::from("Repo").style(theme::HEADER), @@ -176,12 +189,19 @@ fn render_pr_table( _ => "", }; + let (merge_label, merge_style) = merge_state_display(pr); + Row::new(vec![ Cell::from(format!("#{}", pr.number)).style(if style == theme::HIGHLIGHT { style } else { theme::PR_NUMBER }), + Cell::from(merge_label).style(if style == theme::HIGHLIGHT { + style + } else { + merge_style + }), Cell::from(format!( "{}{}{}", if pr.is_draft { "[Draft] " } else { "" }, @@ -207,6 +227,7 @@ fn render_pr_table( let widths = [ Constraint::Length(7), + Constraint::Length(5), Constraint::Min(20), Constraint::Length(16), Constraint::Length(24), diff --git a/tests/graphql_parse_tests.rs b/tests/graphql_parse_tests.rs index 6852186..b2b7646 100644 --- a/tests/graphql_parse_tests.rs +++ b/tests/graphql_parse_tests.rs @@ -28,6 +28,8 @@ fn test_pr_repo_full_name() { additions: 0, deletions: 0, review_decision: None, + mergeable: None, + merge_state_status: None, labels: vec![], }; assert_eq!(pr.repo_full_name(), "org/repo"); @@ -69,6 +71,8 @@ fn test_pr_serialization_roundtrip() { additions: 100, deletions: 50, review_decision: Some("APPROVED".into()), + mergeable: Some("MERGEABLE".into()), + merge_state_status: Some("CLEAN".into()), labels: vec!["bug".into(), "urgent".into()], }; @@ -80,6 +84,8 @@ fn test_pr_serialization_roundtrip() { assert_eq!(deserialized.author, "alice"); assert!(deserialized.is_draft); assert_eq!(deserialized.review_decision, Some("APPROVED".into())); + assert_eq!(deserialized.mergeable, Some("MERGEABLE".into())); + assert_eq!(deserialized.merge_state_status, Some("CLEAN".into())); assert_eq!(deserialized.labels, vec!["bug", "urgent"]); } @@ -121,9 +127,64 @@ fn test_pr_with_no_review_decision() { additions: 0, deletions: 0, review_decision: None, + mergeable: None, + merge_state_status: None, labels: vec![], }; assert!(pr.review_decision.is_none()); assert!(pr.labels.is_empty()); } + +#[test] +fn test_pr_deserializes_without_merge_fields() { + // Older cache entries won't have mergeable / mergeStateStatus. #[serde(default)] + // must let them deserialize to None rather than failing (which would drop the + // whole cache entry). Guards the CI-3 cache-schema-evolution concern. + let legacy = r#"{ + "number": 7, + "title": "Legacy cached PR", + "author": "bob", + "repo_owner": "org", + "repo_name": "repo", + "url": "https://github.com/org/repo/pull/7", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-02T00:00:00Z", + "is_draft": false, + "additions": 3, + "deletions": 1, + "review_decision": null, + "labels": [] + }"#; + + let pr: PullRequest = serde_json::from_str(legacy).expect("legacy cache must deserialize"); + assert_eq!(pr.number, 7); + assert!(pr.mergeable.is_none()); + assert!(pr.merge_state_status.is_none()); +} + +#[test] +fn test_pr_conflicting_merge_state_roundtrip() { + let pr = PullRequest { + number: 9, + title: "Conflicting PR".into(), + author: "carol".into(), + repo_owner: "org".into(), + repo_name: "repo".into(), + url: "https://github.com/org/repo/pull/9".into(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + is_draft: false, + additions: 1, + deletions: 1, + review_decision: None, + mergeable: Some("CONFLICTING".into()), + merge_state_status: Some("DIRTY".into()), + labels: vec![], + }; + + let json = serde_json::to_string(&pr).unwrap(); + let deserialized: PullRequest = serde_json::from_str(&json).unwrap(); + assert_eq!(deserialized.mergeable, Some("CONFLICTING".into())); + assert_eq!(deserialized.merge_state_status, Some("DIRTY".into())); +} diff --git a/tests/state_tests.rs b/tests/state_tests.rs index db74768..8718eed 100644 --- a/tests/state_tests.rs +++ b/tests/state_tests.rs @@ -35,6 +35,8 @@ fn make_pr(repo_owner: &str, repo_name: &str, number: u32, title: &str) -> PullR additions: 10, deletions: 5, review_decision: None, + mergeable: None, + merge_state_status: None, labels: vec![], } } From 5235664543f8e95127b0333a2795281a47649aa2 Mon Sep 17 00:00:00 2001 From: sashml Date: Wed, 1 Jul 2026 09:31:50 +0200 Subject: [PATCH 2/3] feat(pr): add detail-on-highlight pane (fresh mergeable, CI, git log) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Press `d` on a highlighted PR to open a detail overlay that fetches the PR directly — forcing GitHub to compute a fresh mergeable/mergeStateStatus (the search API returns UNKNOWN) — plus the CI check rollup and recent commits. - queries: PR_DETAIL_QUERY (mergeable, mergeStateStatus, commits(last:5) + per-commit statusCheckRollup) - github: fetch_pr_detail + parse_pr_detail; CommitInfo + PrDetail models - state/update: detail_open + pr_details cache; ToggleDetail; Back closes the pane; Refresh clears cached details; resolved detail upgrades the list column - event_loop: `d` key; 200ms debounce so holding j/k never sprays API calls and navigation stays responsive; fetch runs behind the existing semaphore - ui: render_pr_detail_overlay (merge state + CI + commits), status-bar hint - tests: toggle, back-closes-pane, list-upgrade, refresh-clears, failure path Refs: #1, zombo-sash-eco-zkk5 --- src/app/actions.rs | 20 +++++++- src/app/event_loop.rs | 84 ++++++++++++++++++++++++++++++ src/app/state.rs | 37 ++++++++++++- src/app/update.rs | 33 +++++++++++- src/app/view.rs | 1 + src/github/graphql.rs | 63 +++++++++++++++++++++++ src/github/models.rs | 29 +++++++++++ src/github/queries.rs | 30 +++++++++++ src/ui/widgets.rs | 117 ++++++++++++++++++++++++++++++++++++++++-- tests/state_tests.rs | 105 +++++++++++++++++++++++++++++++++++++ 10 files changed, 513 insertions(+), 6 deletions(-) diff --git a/src/app/actions.rs b/src/app/actions.rs index 1375a98..3ae13a4 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -1,4 +1,4 @@ -use crate::github::models::{PullRequest, RateLimit, Repo}; +use crate::github::models::{PrDetail, PullRequest, RateLimit, Repo}; #[derive(Debug)] #[allow(dead_code)] @@ -11,6 +11,7 @@ pub enum Action { Refresh, OpenInBrowser, ToggleSearch, + ToggleDetail, SearchInput(char), SearchBackspace, SearchClear, @@ -36,6 +37,16 @@ pub enum DataPayload { prs: Vec, rate_limit: RateLimit, }, + PrDetailLoaded { + /// PR url — the key into `AppState::pr_details`. + key: String, + detail: PrDetail, + rate_limit: RateLimit, + }, + PrDetailFailed { + key: String, + msg: String, + }, } #[derive(Debug)] @@ -45,5 +56,12 @@ pub enum SideEffect { FetchUserRepos(String), FetchInbox, FetchAllOpenPrs, + FetchPrDetail { + owner: String, + name: String, + number: u32, + /// PR url — echoed back so the result can be stored under the right key. + key: String, + }, OpenUrl(String), } diff --git a/src/app/event_loop.rs b/src/app/event_loop.rs index 9d65b61..2759776 100644 --- a/src/app/event_loop.rs +++ b/src/app/event_loop.rs @@ -91,6 +91,14 @@ async fn run_loop( // First tick fires immediately (already handled by initial fetch above) refresh_timer.tick().await; + // PR detail debounce: when the highlighted PR changes while the detail pane is + // open, wait for ~200ms of stable selection before fetching, so holding j/k + // does not spray API calls. Starts far in the future (disarmed). + let detail_debounce = tokio::time::sleep(tokio::time::Duration::from_secs(86_400)); + tokio::pin!(detail_debounce); + let mut armed_key: Option = None; + let mut pending_detail: Option = None; + loop { // Render terminal.draw(|f| view::render(f, &state))?; @@ -99,6 +107,27 @@ async fn run_loop( break; } + // (Re)arm the detail debounce whenever the highlighted PR changes while the + // pane is open and we don't already have (or are fetching) its detail. + let desired_pr = if state.detail_open { + state.selected_pr() + } else { + None + }; + let desired_key = desired_pr.as_ref().map(|p| p.url.clone()); + if desired_key != armed_key { + armed_key = desired_key; + match desired_pr { + Some(pr) if !state.pr_details.contains_key(&pr.url) => { + pending_detail = Some(pr); + detail_debounce.as_mut().reset( + tokio::time::Instant::now() + tokio::time::Duration::from_millis(200), + ); + } + _ => pending_detail = None, + } + } + // Wait for events tokio::select! { // Terminal events @@ -151,6 +180,28 @@ async fn run_loop( } } } + // Debounced PR detail fetch (only polled while a fetch is pending) + _ = &mut detail_debounce, if pending_detail.is_some() => { + if let Some(pr) = pending_detail.take() { + state + .pr_details + .insert(pr.url.clone(), crate::app::state::PrDetailEntry::Loading); + spawn_side_effect( + SideEffect::FetchPrDetail { + owner: pr.repo_owner.clone(), + name: pr.repo_name.clone(), + number: pr.number, + key: pr.url.clone(), + }, + &config, + &client, + &viewer_login, + &cache_store, + &action_tx, + &semaphore, + ); + } + } } } @@ -199,6 +250,7 @@ fn map_event_to_action(event: &Event, state: &AppState) -> Option { KeyCode::BackTab => Some(Action::SwitchPane), KeyCode::Char('r') => Some(Action::Refresh), KeyCode::Char('o') => Some(Action::OpenInBrowser), + KeyCode::Char('d') => Some(Action::ToggleDetail), KeyCode::Char('/') => Some(Action::ToggleSearch), _ => None, } @@ -470,6 +522,38 @@ fn spawn_side_effect( } }); } + SideEffect::FetchPrDetail { + owner, + name, + number, + key, + } => { + let client = client.clone(); + let tx = action_tx.clone(); + let sem = semaphore.clone(); + + tokio::spawn(async move { + let _permit = sem.acquire().await; + debug!(owner = %owner, name = %name, number = number, "Fetching PR detail"); + + match client.fetch_pr_detail(&owner, &name, number).await { + Ok((detail, rate_limit)) => { + let _ = tx.send(Action::DataLoaded(DataPayload::PrDetailLoaded { + key, + detail, + rate_limit, + })); + } + Err(e) => { + error!(error = %e, "Failed to fetch PR detail"); + let _ = tx.send(Action::DataLoaded(DataPayload::PrDetailFailed { + key, + msg: format!("{}", e), + })); + } + } + }); + } SideEffect::OpenUrl(url) => { tokio::task::spawn_blocking(move || { if let Err(e) = crate::util::browser::open_url(&url) { diff --git a/src/app/state.rs b/src/app/state.rs index 46dd596..4437e2d 100644 --- a/src/app/state.rs +++ b/src/app/state.rs @@ -1,6 +1,14 @@ use std::collections::{HashMap, HashSet}; -use crate::github::models::{PullRequest, RateLimit, Repo}; +use crate::github::models::{PrDetail, PullRequest, RateLimit, Repo}; + +/// State of an on-demand PR detail fetch, keyed by PR url in `AppState::pr_details`. +#[derive(Debug, Clone)] +pub enum PrDetailEntry { + Loading, + Loaded(PrDetail), + Failed(String), +} #[derive(Debug, Clone, PartialEq, Eq)] pub enum FocusedPane { @@ -58,6 +66,10 @@ pub struct AppState { pub search_active: bool, pub search_query: String, + // PR detail pane (fetched on-highlight) + pub detail_open: bool, + pub pr_details: HashMap, + // UI flags pub loading: bool, pub loading_orgs: HashSet, @@ -96,6 +108,8 @@ impl AppState { content_cursor: 0, search_active: false, search_query: String::new(), + detail_open: false, + pr_details: HashMap::new(), loading: true, loading_orgs: HashSet::new(), error_message: None, @@ -188,6 +202,27 @@ impl AppState { prs.get(self.content_cursor).map(|pr| pr.url.clone()) } + /// The currently highlighted PR (in the content pane), cloned. + pub fn selected_pr(&self) -> Option { + self.current_pr_list().into_iter().nth(self.content_cursor) + } + + /// Apply a freshly fetched merge state to the matching PR in every list, so + /// the list column reflects the authoritative value once detail resolves. + pub fn apply_fresh_merge_state( + &mut self, + url: &str, + mergeable: Option, + merge_state_status: Option, + ) { + for pr in self.all_open_prs.iter_mut().chain(self.inbox.iter_mut()) { + if pr.url == url { + pr.mergeable = mergeable.clone(); + pr.merge_state_status = merge_state_status.clone(); + } + } + } + pub fn selected_nav_url(&self) -> Option { self.nav_nodes .get(self.nav_cursor) diff --git a/src/app/update.rs b/src/app/update.rs index 51acbc2..0cda2db 100644 --- a/src/app/update.rs +++ b/src/app/update.rs @@ -1,5 +1,5 @@ use crate::app::actions::{Action, DataPayload, SideEffect}; -use crate::app::state::{AppState, ContentView, FocusedPane, NavNode, OrgData}; +use crate::app::state::{AppState, ContentView, FocusedPane, NavNode, OrgData, PrDetailEntry}; pub fn update(state: &mut AppState, action: Action) -> Vec { match action { @@ -83,6 +83,8 @@ pub fn update(state: &mut AppState, action: Action) -> Vec { state.search_query.clear(); } else if state.error_message.is_some() { state.error_message = None; + } else if state.detail_open { + state.detail_open = false; } else if state.focused_pane == FocusedPane::Content { state.focused_pane = FocusedPane::Navigation; } @@ -98,6 +100,8 @@ pub fn update(state: &mut AppState, action: Action) -> Vec { Action::Refresh => { state.loading = true; state.error_message = None; + // Drop cached PR details so merge state / CI is recomputed fresh. + state.pr_details.clear(); vec![SideEffect::RefreshAll] } Action::OpenInBrowser => { @@ -121,6 +125,12 @@ pub fn update(state: &mut AppState, action: Action) -> Vec { } vec![] } + Action::ToggleDetail => { + // Only meaningful in the content pane; the event loop fetches detail + // for the highlighted PR (debounced) while the pane is open. + state.detail_open = !state.detail_open; + vec![] + } Action::SearchInput(ch) => { if state.search_active { state.search_query.push(ch); @@ -160,6 +170,27 @@ pub fn update(state: &mut AppState, action: Action) -> Vec { state.rate_limit = rate_limit; state.all_open_prs = prs; } + DataPayload::PrDetailLoaded { + key, + detail, + rate_limit, + } => { + state.rate_limit = rate_limit; + // Upgrade the list column to the freshly computed merge state. + state.apply_fresh_merge_state( + &key, + detail.mergeable.clone(), + detail.merge_state_status.clone(), + ); + state.pr_details.insert(key, PrDetailEntry::Loaded(detail)); + // A detail fetch is not part of the initial load; leave the + // global loading flag untouched by returning early. + return vec![]; + } + DataPayload::PrDetailFailed { key, msg } => { + state.pr_details.insert(key, PrDetailEntry::Failed(msg)); + return vec![]; + } } // Check if all loading complete diff --git a/src/app/view.rs b/src/app/view.rs index ad78aaf..21c1c70 100644 --- a/src/app/view.rs +++ b/src/app/view.rs @@ -30,6 +30,7 @@ pub fn render(f: &mut Frame, state: &AppState) { widgets::render_status_bar(f, status_area, state); // Overlays + widgets::render_pr_detail_overlay(f, state); widgets::render_search_overlay(f, state); if state.error_message.is_some() { widgets::render_error_modal(f, f.area(), state); diff --git a/src/github/graphql.rs b/src/github/graphql.rs index 68ce2bf..d64bb60 100644 --- a/src/github/graphql.rs +++ b/src/github/graphql.rs @@ -267,6 +267,69 @@ impl GithubClient { let query_string = format!("is:open is:pr archived:false {}", filter); self.search_prs(&query_string).await } + + /// Fetch on-demand detail for a single PR (fresh merge state, recent commits, + /// CI rollup). Used by the detail-on-highlight pane. + pub async fn fetch_pr_detail( + &self, + owner: &str, + name: &str, + number: u32, + ) -> Result<(PrDetail, RateLimit)> { + let variables = json!({ + "owner": owner, + "name": name, + "number": number, + }); + + let data = self.query(queries::PR_DETAIL_QUERY, variables).await?; + let rate_limit = Self::extract_rate_limit(&data); + + let pr_node = &data["data"]["repository"]["pullRequest"]; + if pr_node.is_null() { + bail!("Pull request {}/{}#{} not found", owner, name, number); + } + + Ok((parse_pr_detail(pr_node), rate_limit)) + } +} + +fn parse_pr_detail(node: &Value) -> PrDetail { + let commit_nodes = node["commits"]["nodes"].as_array(); + + let commits: Vec = commit_nodes + .map(|arr| { + arr.iter() + .filter_map(|n| { + let commit = &n["commit"]; + let oid = commit["oid"].as_str()?.to_string(); + Some(CommitInfo { + oid, + headline: commit["messageHeadline"].as_str().unwrap_or("").to_string(), + committed_date: commit["committedDate"] + .as_str() + .and_then(|s| s.parse().ok()) + .unwrap_or_default(), + author: commit["author"]["name"].as_str().unwrap_or("").to_string(), + }) + }) + .collect() + }) + .unwrap_or_default(); + + // GitHub returns commits oldest-first; the CI rollup on the newest (last) commit + // reflects the PR's current check status. + let checks_status = commit_nodes + .and_then(|arr| arr.last()) + .and_then(|n| n["commit"]["statusCheckRollup"]["state"].as_str()) + .map(|s| s.to_string()); + + PrDetail { + mergeable: node["mergeable"].as_str().map(|s| s.to_string()), + merge_state_status: node["mergeStateStatus"].as_str().map(|s| s.to_string()), + checks_status, + commits, + } } fn parse_search_pr(node: &Value) -> PullRequest { diff --git a/src/github/models.rs b/src/github/models.rs index db08497..7c04d7b 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -56,6 +56,35 @@ impl PullRequest { } } +/// A single commit shown in the PR detail pane ("git log"). +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CommitInfo { + pub oid: String, + pub headline: String, + pub committed_date: DateTime, + pub author: String, +} + +impl CommitInfo { + /// Short 7-char SHA for display. + pub fn short_oid(&self) -> &str { + let end = self.oid.len().min(7); + &self.oid[..end] + } +} + +/// On-demand detail for a single PR, fetched when its row is highlighted. +/// Unlike the list, this forces GitHub to compute a fresh `mergeable`. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct PrDetail { + pub mergeable: Option, + pub merge_state_status: Option, + /// `statusCheckRollup.state`: `SUCCESS` / `FAILURE` / `PENDING` / `ERROR` / `EXPECTED`. + pub checks_status: Option, + /// Recent commits, oldest-first as returned by GitHub (`commits(last: N)`). + pub commits: Vec, +} + #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct RateLimit { pub remaining: u32, diff --git a/src/github/queries.rs b/src/github/queries.rs index 6cd7321..fe17518 100644 --- a/src/github/queries.rs +++ b/src/github/queries.rs @@ -141,3 +141,33 @@ query($query: String!, $cursor: String) { } } "#; + +/// Detail for a single PR, fetched on-demand when a row is highlighted. +/// Accessing the PR directly (vs. the search API) makes GitHub compute a fresh +/// `mergeable`/`mergeStateStatus`, and lets us pull the recent commits + CI rollup. +pub const PR_DETAIL_QUERY: &str = r#" +query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + mergeable + mergeStateStatus + commits(last: 5) { + nodes { + commit { + oid + messageHeadline + committedDate + author { name } + statusCheckRollup { state } + } + } + } + } + } + rateLimit { + remaining + limit + resetAt + } +} +"#; diff --git a/src/ui/widgets.rs b/src/ui/widgets.rs index 8ebe05f..0b46b34 100644 --- a/src/ui/widgets.rs +++ b/src/ui/widgets.rs @@ -5,8 +5,8 @@ use ratatui::{ widgets::{Block, Borders, Cell, Clear, List, ListItem, Paragraph, Row, Table}, }; -use crate::app::state::{AppState, ContentView, FocusedPane, NavNode}; -use crate::github::models::PullRequest; +use crate::app::state::{AppState, ContentView, FocusedPane, NavNode, PrDetailEntry}; +use crate::github::models::{PrDetail, PullRequest}; use crate::ui::theme; use crate::util::time::relative_time; @@ -309,7 +309,7 @@ pub fn render_status_bar(f: &mut Frame, area: Rect, state: &AppState) { let key_hints = if state.search_active { "Esc: close search | Enter: filter" } else { - "j/k: nav | Tab: switch pane | Enter: select | /: search | r: refresh | o: open | q: quit" + "j/k: nav | Tab: switch pane | Enter: select | d: detail | /: search | r: refresh | o: open | q: quit" }; let status = if state.loading { @@ -419,3 +419,114 @@ pub fn render_error_modal(f: &mut Frame, area: Rect, state: &AppState) { let para = Paragraph::new(text).block(block); f.render_widget(para, modal_area); } + +/// Human label + style for a PR's `mergeable` value, used in the detail pane. +fn mergeable_label(mergeable: Option<&str>) -> (String, ratatui::style::Style) { + match mergeable { + Some("MERGEABLE") => ("✓ mergeable".to_string(), theme::MERGE_CLEAN), + Some("CONFLICTING") => ("✗ conflicting".to_string(), theme::MERGE_CONFLICT), + _ => ("? unknown".to_string(), theme::DIM), + } +} + +/// Human label + style for a `statusCheckRollup.state` value. +fn checks_label(checks: Option<&str>) -> (String, ratatui::style::Style) { + match checks { + Some("SUCCESS") => ("✓ passing".to_string(), theme::MERGE_CLEAN), + Some("FAILURE") | Some("ERROR") => ("✗ failing".to_string(), theme::MERGE_CONFLICT), + Some("PENDING") | Some("EXPECTED") => ("… pending".to_string(), theme::WARNING), + Some(other) => (other.to_string(), theme::DIM), + None => ("— no checks".to_string(), theme::DIM), + } +} + +fn detail_body_lines(detail: &PrDetail, max_commits: usize) -> Vec> { + let mut lines = Vec::new(); + + let (merge_text, merge_style) = mergeable_label(detail.mergeable.as_deref()); + let (checks_text, checks_style) = checks_label(detail.checks_status.as_deref()); + let state_suffix = detail + .merge_state_status + .as_deref() + .map(|s| format!(" ({})", s)) + .unwrap_or_default(); + + lines.push(Line::from(vec![ + Span::styled("Merge: ", theme::HEADER), + Span::styled(format!("{}{}", merge_text, state_suffix), merge_style), + Span::raw(" "), + Span::styled("CI: ", theme::HEADER), + Span::styled(checks_text, checks_style), + ])); + lines.push(Line::from("")); + lines.push(Line::from(Span::styled("Recent commits:", theme::HEADER))); + + if detail.commits.is_empty() { + lines.push(Line::from(Span::styled(" (none)", theme::DIM))); + } else { + // GitHub returns oldest-first; show newest first. + for commit in detail.commits.iter().rev().take(max_commits) { + lines.push(Line::from(vec![ + Span::styled(format!(" {} ", commit.short_oid()), theme::PR_NUMBER), + Span::raw(commit.headline.clone()), + Span::styled( + format!(" ({})", relative_time(&commit.committed_date)), + theme::DIM, + ), + ])); + } + } + + lines +} + +/// Detail-on-highlight overlay: fresh merge state, CI rollup, and recent commits +/// for the currently highlighted PR. +pub fn render_pr_detail_overlay(f: &mut Frame, state: &AppState) { + if !state.detail_open { + return; + } + let Some(pr) = state.selected_pr() else { + return; + }; + + let area = f.area(); + let modal_width = (area.width * 3 / 4).clamp(40, area.width.saturating_sub(4)); + let modal_height = 14u16.min(area.height.saturating_sub(2)); + let x = (area.width.saturating_sub(modal_width)) / 2; + let y = (area.height.saturating_sub(modal_height)) / 2; + let modal_area = Rect { + x, + y, + width: modal_width, + height: modal_height, + }; + + let title = format!(" PR #{} — {} ", pr.number, pr.title); + let block = Block::default() + .title(title) + .borders(Borders::ALL) + .border_style(theme::BORDER_FOCUSED); + + let body_capacity = modal_area.height.saturating_sub(4) as usize; + let mut lines: Vec = match state.pr_details.get(&pr.url) { + Some(PrDetailEntry::Loaded(detail)) => { + detail_body_lines(detail, body_capacity.saturating_sub(3)) + } + Some(PrDetailEntry::Failed(msg)) => { + vec![Line::from(Span::styled(msg.clone(), theme::ERROR))] + } + Some(PrDetailEntry::Loading) | None => { + vec![Line::from(Span::styled("Loading detail…", theme::DIM))] + } + }; + lines.push(Line::from("")); + lines.push(Line::from(Span::styled( + "Press d or Esc to close", + theme::DIM, + ))); + + f.render_widget(Clear, modal_area); + let para = Paragraph::new(lines).block(block); + f.render_widget(para, modal_area); +} diff --git a/tests/state_tests.rs b/tests/state_tests.rs index 8718eed..03339ae 100644 --- a/tests/state_tests.rs +++ b/tests/state_tests.rs @@ -470,3 +470,108 @@ fn test_archived_repos_excluded_from_nav() { assert_eq!(repo_names, vec!["active-repo"]); } + +// --- PR detail pane (task zkk5) --- + +#[test] +fn test_toggle_detail_flips_flag() { + let mut state = make_state(); + assert!(!state.detail_open); + update(&mut state, Action::ToggleDetail); + assert!(state.detail_open); + update(&mut state, Action::ToggleDetail); + assert!(!state.detail_open); +} + +#[test] +fn test_back_closes_detail_before_switching_pane() { + let mut state = make_state(); + state.focused_pane = FocusedPane::Content; + update(&mut state, Action::ToggleDetail); + assert!(state.detail_open); + + // Back should close the detail pane first, leaving focus on Content. + update(&mut state, Action::Back); + assert!(!state.detail_open); + assert_eq!(state.focused_pane, FocusedPane::Content); +} + +#[test] +fn test_pr_detail_loaded_upgrades_list_merge_state() { + use ghdash::app::state::PrDetailEntry; + use ghdash::github::models::PrDetail; + + let mut state = make_state(); + // A PR whose list value is UNKNOWN (typical of the search API). + let mut pr = make_pr("org-a", "repo1", 7, "Needs fresh state"); + pr.mergeable = Some("UNKNOWN".into()); + let url = pr.url.clone(); + update( + &mut state, + Action::DataLoaded(DataPayload::AllOpenPrs { + prs: vec![pr], + rate_limit: RateLimit::default(), + }), + ); + + let detail = PrDetail { + mergeable: Some("CONFLICTING".into()), + merge_state_status: Some("DIRTY".into()), + checks_status: Some("FAILURE".into()), + commits: vec![], + }; + update( + &mut state, + Action::DataLoaded(DataPayload::PrDetailLoaded { + key: url.clone(), + detail, + rate_limit: RateLimit::default(), + }), + ); + + // Detail is cached and the list column reflects the fresh value. + assert!(matches!( + state.pr_details.get(&url), + Some(PrDetailEntry::Loaded(_)) + )); + assert_eq!( + state.all_open_prs[0].mergeable.as_deref(), + Some("CONFLICTING") + ); + assert_eq!( + state.all_open_prs[0].merge_state_status.as_deref(), + Some("DIRTY") + ); +} + +#[test] +fn test_refresh_clears_pr_details() { + use ghdash::app::state::PrDetailEntry; + + let mut state = make_state(); + state + .pr_details + .insert("some-url".into(), PrDetailEntry::Loading); + assert!(!state.pr_details.is_empty()); + + update(&mut state, Action::Refresh); + assert!(state.pr_details.is_empty()); +} + +#[test] +fn test_pr_detail_failed_records_error() { + use ghdash::app::state::PrDetailEntry; + + let mut state = make_state(); + update( + &mut state, + Action::DataLoaded(DataPayload::PrDetailFailed { + key: "url-x".into(), + msg: "boom".into(), + }), + ); + assert!(matches!( + state.pr_details.get("url-x"), + Some(PrDetailEntry::Failed(_)) + )); +} From ee0ad7c87927065f49b4a80cde01c2493edd0751 Mon Sep 17 00:00:00 2001 From: sashml Date: Wed, 1 Jul 2026 10:14:40 +0200 Subject: [PATCH 3/3] feat(pr): add CI check status column to PR tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Show each PR's statusCheckRollup as a compact CI glyph in the list. Unlike mergeable, statusCheckRollup is not computed lazily, so the search API returns real values — a mergeable-but-red PR is now obvious at a glance. - queries: request commits(last:1){...statusCheckRollup{state}} in search & repo queries - models: PullRequest.checks_status (Option, #[serde(default)]) + CiStatus classifier (Passing/Failing/Pending/None) decoupled from the raw GitHub enum - graphql: parse checks_status from the latest commit's rollup - ui: new CI column (✓ passing / ✗ failing / … pending / · none), shape+color - tests: ci_status covers SUCCESS / FAILURE+ERROR / PENDING+EXPECTED / none Refs: #6, zombo-sash-eco-9h3o --- src/github/graphql.rs | 5 +++ src/github/models.rs | 25 +++++++++++++ src/github/queries.rs | 14 ++++++++ src/ui/widgets.rs | 21 ++++++++++- tests/graphql_parse_tests.rs | 68 +++++++++++++++++++++++++++++++++++- tests/state_tests.rs | 1 + 6 files changed, 132 insertions(+), 2 deletions(-) diff --git a/src/github/graphql.rs b/src/github/graphql.rs index d64bb60..1314b7e 100644 --- a/src/github/graphql.rs +++ b/src/github/graphql.rs @@ -372,6 +372,11 @@ fn parse_search_pr(node: &Value) -> PullRequest { review_decision: node["reviewDecision"].as_str().map(|s| s.to_string()), mergeable: node["mergeable"].as_str().map(|s| s.to_string()), merge_state_status: node["mergeStateStatus"].as_str().map(|s| s.to_string()), + checks_status: node["commits"]["nodes"] + .as_array() + .and_then(|arr| arr.last()) + .and_then(|n| n["commit"]["statusCheckRollup"]["state"].as_str()) + .map(|s| s.to_string()), labels, } } diff --git a/src/github/models.rs b/src/github/models.rs index 7c04d7b..affbfb4 100644 --- a/src/github/models.rs +++ b/src/github/models.rs @@ -48,12 +48,37 @@ pub struct PullRequest { /// same lazy-compute caveat. #[serde(default)] pub merge_state_status: Option, + /// `statusCheckRollup.state` of the PR's latest commit: `SUCCESS` / `FAILURE` / + /// `PENDING` / `ERROR` / `EXPECTED`. Unlike `mergeable`, this is not computed + /// lazily, so the search API returns real values. `None` = no checks / absent. + #[serde(default)] + pub checks_status: Option, +} + +/// Coarse CI outcome derived from `checks_status`, decoupled from the raw GitHub +/// enum so the UI (and tests) don't hard-code string matching. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CiStatus { + Passing, + Failing, + Pending, + None, } impl PullRequest { pub fn repo_full_name(&self) -> String { format!("{}/{}", self.repo_owner, self.repo_name) } + + /// Classify the CI check rollup into a coarse outcome for display. + pub fn ci_status(&self) -> CiStatus { + match self.checks_status.as_deref() { + Some("SUCCESS") => CiStatus::Passing, + Some("FAILURE") | Some("ERROR") => CiStatus::Failing, + Some("PENDING") | Some("EXPECTED") => CiStatus::Pending, + _ => CiStatus::None, + } + } } /// A single commit shown in the PR detail pane ("git log"). diff --git a/src/github/queries.rs b/src/github/queries.rs index fe17518..304e6d6 100644 --- a/src/github/queries.rs +++ b/src/github/queries.rs @@ -89,6 +89,13 @@ query($owner: String!, $name: String!, $cursor: String) { reviewDecision mergeable mergeStateStatus + commits(last: 1) { + nodes { + commit { + statusCheckRollup { state } + } + } + } labels(first: 10) { nodes { name } } @@ -128,6 +135,13 @@ query($query: String!, $cursor: String) { reviewDecision mergeable mergeStateStatus + commits(last: 1) { + nodes { + commit { + statusCheckRollup { state } + } + } + } labels(first: 10) { nodes { name } } diff --git a/src/ui/widgets.rs b/src/ui/widgets.rs index 0b46b34..40f1c7a 100644 --- a/src/ui/widgets.rs +++ b/src/ui/widgets.rs @@ -6,7 +6,7 @@ use ratatui::{ }; use crate::app::state::{AppState, ContentView, FocusedPane, NavNode, PrDetailEntry}; -use crate::github::models::{PrDetail, PullRequest}; +use crate::github::models::{CiStatus, PrDetail, PullRequest}; use crate::ui::theme; use crate::util::time::relative_time; @@ -126,6 +126,17 @@ fn merge_state_display(pr: &PullRequest) -> (&'static str, ratatui::style::Style } } +/// Single-glyph CI check indicator for the list column. `statusCheckRollup` is not +/// lazily computed, so this is reliable straight from the search API. +fn ci_display(pr: &PullRequest) -> (&'static str, ratatui::style::Style) { + match pr.ci_status() { + CiStatus::Passing => ("✓", theme::MERGE_CLEAN), + CiStatus::Failing => ("✗", theme::MERGE_CONFLICT), + CiStatus::Pending => ("…", theme::WARNING), + CiStatus::None => ("·", theme::DIM), + } +} + fn render_pr_table( f: &mut Frame, area: Rect, @@ -164,6 +175,7 @@ fn render_pr_table( let header = Row::new(vec![ Cell::from("#").style(theme::HEADER), Cell::from("State").style(theme::HEADER), + Cell::from("CI").style(theme::HEADER), Cell::from("Title").style(theme::HEADER), Cell::from("Author").style(theme::HEADER), Cell::from("Repo").style(theme::HEADER), @@ -190,6 +202,7 @@ fn render_pr_table( }; let (merge_label, merge_style) = merge_state_display(pr); + let (ci_label, ci_style) = ci_display(pr); Row::new(vec![ Cell::from(format!("#{}", pr.number)).style(if style == theme::HIGHLIGHT { @@ -202,6 +215,11 @@ fn render_pr_table( } else { merge_style }), + Cell::from(ci_label).style(if style == theme::HIGHLIGHT { + style + } else { + ci_style + }), Cell::from(format!( "{}{}{}", if pr.is_draft { "[Draft] " } else { "" }, @@ -228,6 +246,7 @@ fn render_pr_table( let widths = [ Constraint::Length(7), Constraint::Length(5), + Constraint::Length(3), Constraint::Min(20), Constraint::Length(16), Constraint::Length(24), diff --git a/tests/graphql_parse_tests.rs b/tests/graphql_parse_tests.rs index b2b7646..ee0d3f3 100644 --- a/tests/graphql_parse_tests.rs +++ b/tests/graphql_parse_tests.rs @@ -1,4 +1,4 @@ -use ghdash::github::models::{PullRequest, Repo}; +use ghdash::github::models::{CiStatus, PullRequest, Repo}; #[test] fn test_repo_full_name() { @@ -30,6 +30,7 @@ fn test_pr_repo_full_name() { review_decision: None, mergeable: None, merge_state_status: None, + checks_status: None, labels: vec![], }; assert_eq!(pr.repo_full_name(), "org/repo"); @@ -73,6 +74,7 @@ fn test_pr_serialization_roundtrip() { review_decision: Some("APPROVED".into()), mergeable: Some("MERGEABLE".into()), merge_state_status: Some("CLEAN".into()), + checks_status: Some("SUCCESS".into()), labels: vec!["bug".into(), "urgent".into()], }; @@ -86,6 +88,7 @@ fn test_pr_serialization_roundtrip() { assert_eq!(deserialized.review_decision, Some("APPROVED".into())); assert_eq!(deserialized.mergeable, Some("MERGEABLE".into())); assert_eq!(deserialized.merge_state_status, Some("CLEAN".into())); + assert_eq!(deserialized.checks_status, Some("SUCCESS".into())); assert_eq!(deserialized.labels, vec!["bug", "urgent"]); } @@ -129,6 +132,7 @@ fn test_pr_with_no_review_decision() { review_decision: None, mergeable: None, merge_state_status: None, + checks_status: None, labels: vec![], }; @@ -161,6 +165,7 @@ fn test_pr_deserializes_without_merge_fields() { assert_eq!(pr.number, 7); assert!(pr.mergeable.is_none()); assert!(pr.merge_state_status.is_none()); + assert!(pr.checks_status.is_none()); } #[test] @@ -180,6 +185,7 @@ fn test_pr_conflicting_merge_state_roundtrip() { review_decision: None, mergeable: Some("CONFLICTING".into()), merge_state_status: Some("DIRTY".into()), + checks_status: Some("FAILURE".into()), labels: vec![], }; @@ -187,4 +193,64 @@ fn test_pr_conflicting_merge_state_roundtrip() { let deserialized: PullRequest = serde_json::from_str(&json).unwrap(); assert_eq!(deserialized.mergeable, Some("CONFLICTING".into())); assert_eq!(deserialized.merge_state_status, Some("DIRTY".into())); + assert_eq!(deserialized.checks_status, Some("FAILURE".into())); +} + +// --- CI status classification (task 9h3o) --- + +fn pr_with_checks(state: Option<&str>) -> PullRequest { + PullRequest { + number: 1, + title: "t".into(), + author: "a".into(), + repo_owner: "o".into(), + repo_name: "r".into(), + url: "u".into(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + is_draft: false, + additions: 0, + deletions: 0, + review_decision: None, + mergeable: None, + merge_state_status: None, + checks_status: state.map(|s| s.to_string()), + labels: vec![], + } +} + +#[test] +fn test_ci_status_success() { + assert_eq!( + pr_with_checks(Some("SUCCESS")).ci_status(), + CiStatus::Passing + ); +} + +#[test] +fn test_ci_status_failure() { + assert_eq!( + pr_with_checks(Some("FAILURE")).ci_status(), + CiStatus::Failing + ); + assert_eq!(pr_with_checks(Some("ERROR")).ci_status(), CiStatus::Failing); +} + +#[test] +fn test_ci_status_pending() { + assert_eq!( + pr_with_checks(Some("PENDING")).ci_status(), + CiStatus::Pending + ); + assert_eq!( + pr_with_checks(Some("EXPECTED")).ci_status(), + CiStatus::Pending + ); +} + +#[test] +fn test_ci_status_none() { + assert_eq!(pr_with_checks(None).ci_status(), CiStatus::None); + // Unknown/other states fall back to None rather than misreporting. + assert_eq!(pr_with_checks(Some("WEIRD")).ci_status(), CiStatus::None); } diff --git a/tests/state_tests.rs b/tests/state_tests.rs index 03339ae..298ead1 100644 --- a/tests/state_tests.rs +++ b/tests/state_tests.rs @@ -37,6 +37,7 @@ fn make_pr(repo_owner: &str, repo_name: &str, number: u32, title: &str) -> PullR review_decision: None, mergeable: None, merge_state_status: None, + checks_status: None, labels: vec![], } }