feat: stream newest pull requests by category

This commit is contained in:
Valdemar Lorenzen
2026-10-14 14:08:21 +02:00
parent 5243de66fe
commit 0000089f42
8 changed files with 1465 additions and 377 deletions
+1
View File
@@ -8,6 +8,7 @@
- three tabs: open, closed, all - three tabs: open, closed, all
- search under the given category - search under the given category
- selecting a pr opens the diff viewer for that pr using the branch merge base (standard github diff view on prs; already implemented as default diff viewer behavior in vamp) - selecting a pr opens the diff viewer for that pr using the branch merge base (standard github diff view on prs; already implemented as default diff viewer behavior in vamp)
- [ ] move the checkmark in the file picker in the diff viewer right next to the modification type indicator (still left of, but only with a 1 column margin). the modification type indicator should retain it's current position, not be inset further.
- [ ] diff view: github integration (code review) - add comments on a pr, add review comments at specific lines of files, request changes, approve pr, etc. - [ ] diff view: github integration (code review) - add comments on a pr, add review comments at specific lines of files, request changes, approve pr, etc.
- [ ] diff view: local review comments (same features as for github reviews) that allows review of local changes to deliver specific reviews to ai agents used for coding. - [ ] diff view: local review comments (same features as for github reviews) that allows review of local changes to deliver specific reviews to ai agents used for coding.
- design question: would this require a server that the agent can interact with, or would local review files be fine? don't want to use server client stuff, if it would be easier and better to just use local files. - design question: would this require a server that the agent can interact with, or would local review files be fine? don't want to use server client stuff, if it would be easier and better to just use local files.
+6 -2
View File
@@ -384,9 +384,13 @@ Standard GitHub/GitLab/Gitea hosts and Codeberg are detected from HTTPS or SSH r
SSH aliases are resolved with `ssh -G` before provider detection and API authentication, honoring the effective `HostName` and user-specific `Match` settings. SSH ports do not become HTTPS API ports. Git fetches retain the original remote URL, alias, and SSH identity configuration. A failed API load shows its error without also claiming that the search has no matches; the empty-result message appears only after a successful load. SSH aliases are resolved with `ssh -G` before provider detection and API authentication, honoring the effective `HostName` and user-specific `Match` settings. SSH ports do not become HTTPS API ports. Git fetches retain the original remote URL, alias, and SSH identity configuration. A failed API load shows its error without also claiming that the search has no matches; the empty-result message appears only after a successful load.
The picker loads every API page and searches locally without additional network requests when changing tabs or typing. GitLab also reads each request's historical diff metadata; large histories can take longer, and Escape cancels outstanding work. Fork heads are fetched through the target repository's PR refs, into the existing private comparison store, without changing active refs, jj operations, or checkout files. Closed/merged requests retain historical bases and heads rather than comparing against today's branch tips. If the server no longer retains the required commits, opening the request reports an error instead of displaying a different diff. Review submission and comments are separate, not part of this picker. Open and Closed fetch their first pages concurrently, newest-first, and display each page immediately. Subsequent pages load in the background, with the active category dispatched first; loaded rows remain selectable. Pages request 50 entries. GitLab Closed combines separate closed and merged requests (up to 100 entries per batch). All merges the two catalogs without a third fetch stream, deduplicates requests, and sorts by creation time; background updates preserve the selected PR's identity.
Requests with missing/unprepared historical metadata remain listed with an `unavailable` reason. Selecting one reports that error without blocking other requests or substituting a live branch tip. Search filters loaded entries locally and explicitly indicates partial results while more pages remain. A later-page error retains loaded rows; **Ctrl+r** retries failed categories (the active category, or both from All). Enter selects a loaded row; in an empty failed category it retries loading. Escape cancels outstanding work. Selection stops listing and only then retrieves that PR's historical metadata and preserved head reference, so GitLab detail reads and Gitea ref lookups do not delay the catalog.
Fork heads are fetched through the target repository's PR refs into the existing private comparison store, without changing active refs, jj operations, or checkout files. Closed/merged requests retain historical bases and heads rather than comparing against today's branch tips. If the server no longer retains the required commits, opening the request reports an error instead of displaying a different diff. Review submission and comments are separate, not part of this picker.
Missing or unprepared historical metadata is reported when selecting the affected request; it does not block listing other requests or substitute a live branch tip.
Comparison discovery builds the changed-path/status catalog first. File text, notices, and hunks load asynchronously when a file is selected and remain cached for that comparison. Failed file loads can be retried by selecting the file again; cancelled or superseded results cannot replace the current selection. Working-checkout comparisons still capture working bytes and the historical content needed to identify changes, preserving the frozen Before snapshot and unsaved After buffers. Comparison discovery builds the changed-path/status catalog first. File text, notices, and hunks load asynchronously when a file is selected and remain cached for that comparison. Failed file loads can be retried by selecting the file again; cancelled or superseded results cannot replace the current selection. Working-checkout comparisons still capture working bytes and the historical content needed to identify changes, preserving the frozen Before snapshot and unsaved After buffers.
+461 -169
View File
@@ -74,12 +74,8 @@ pub enum RevisionTarget {
}, },
PullRequest { PullRequest {
remote: String, remote: String,
base_ref: String, number: u64,
base_sha: String,
head_ref: String,
head_sha: String,
}, },
UnavailablePullRequest(String),
ListRemote(String), ListRemote(String),
FetchRemote(String), FetchRemote(String),
} }
@@ -96,6 +92,19 @@ pub struct Catalog {
pub entries: Vec<Revision>, pub entries: Vec<Revision>,
pub notice: Option<String>, pub notice: Option<String>,
} }
#[derive(Debug)]
pub struct PullRequestPage {
pub kind: RepoKind,
pub remote: String,
pub entries: Vec<(Revision, String)>,
pub has_more: bool,
}
#[derive(Debug, Default)]
struct PullRequestCategory {
complete: bool,
error: Option<String>,
}
#[derive(Clone, Copy, Debug, Eq, PartialEq)] #[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum FileStatus { pub enum FileStatus {
Added, Added,
@@ -137,6 +146,7 @@ pub struct Comparison {
pub enum Operation { pub enum Operation {
Discover, Discover,
PullRequests(String), PullRequests(String),
ResolvePullRequest(Revision),
ListRemote(String), ListRemote(String),
FetchRemote(String), FetchRemote(String),
Compare { Compare {
@@ -158,6 +168,14 @@ pub enum Operation {
#[derive(Debug)] #[derive(Debug)]
pub enum Outcome { pub enum Outcome {
Catalog(Catalog), Catalog(Catalog),
PullRequestPage {
closed: bool,
result: Result<PullRequestPage, String>,
},
ResolvedPullRequest {
before: Revision,
after: Revision,
},
Comparison(Comparison), Comparison(Comparison),
CheckoutRefresh(Option<Comparison>), CheckoutRefresh(Option<Comparison>),
File { File {
@@ -232,6 +250,9 @@ enum RequestKind {
pub struct DiffState { pub struct DiffState {
pub catalog: Option<Catalog>, pub catalog: Option<Catalog>,
pub pull_request_remote: Option<String>, pub pull_request_remote: Option<String>,
pull_request_categories: [PullRequestCategory; 2],
pull_request_order: BTreeMap<u64, (String, usize)>,
pub(crate) pull_request_retries: [u64; 2],
pub phase: Phase, pub phase: Phase,
pub word_wrap: bool, pub word_wrap: bool,
pub view_mode: ViewMode, pub view_mode: ViewMode,
@@ -281,6 +302,9 @@ impl DiffState {
Self { Self {
catalog: None, catalog: None,
pull_request_remote: None, pull_request_remote: None,
pull_request_categories: Default::default(),
pull_request_order: BTreeMap::new(),
pull_request_retries: [0; 2],
phase: Phase::Before, phase: Phase::Before,
word_wrap: false, word_wrap: false,
view_mode: ViewMode::Split, view_mode: ViewMode::Split,
@@ -406,7 +430,7 @@ impl DiffState {
return; return;
} }
self.request_kind = Some(match operation { self.request_kind = Some(match operation {
Operation::Compare { .. } => RequestKind::Comparison, Operation::Compare { .. } | Operation::ResolvePullRequest(_) => RequestKind::Comparison,
Operation::LoadFile { Operation::LoadFile {
comparison_id, comparison_id,
file_index, file_index,
@@ -422,6 +446,71 @@ impl DiffState {
} }
pub(crate) fn apply_result(&mut self, id: u64, result: Result<Outcome, String>) -> bool { pub(crate) fn apply_result(&mut self, id: u64, result: Result<Outcome, String>) -> bool {
if let Ok(Outcome::PullRequestPage { closed, result }) = result {
if self.pull_request_remote.is_none()
|| self.request_kind != Some(RequestKind::Catalog)
|| self.pending.is_some()
{
return false;
}
self.error = None;
let status = &mut self.pull_request_categories[usize::from(closed)];
match result {
Ok(page) => {
status.complete = !page.has_more;
status.error = None;
let selected = self
.entries()
.nth(self.selected)
.map(|entry| entry.target.clone());
let catalog = self.catalog.get_or_insert_with(|| Catalog {
kind: page.kind,
entries: Vec::new(),
notice: Some(format!("Pull requests from {}", page.remote)),
});
for (revision, created_at) in page.entries {
let RevisionTarget::PullRequest { number, .. } = &revision.target else {
continue;
};
if let Some((date, index)) = self.pull_request_order.get_mut(number) {
*date = created_at;
catalog.entries[*index] = revision;
} else {
self.pull_request_order
.insert(*number, (created_at, catalog.entries.len()));
catalog.entries.push(revision);
}
}
let number = |revision: &Revision| match revision.target {
RevisionTarget::PullRequest { number, .. } => number,
_ => unreachable!("pull request catalog"),
};
catalog.entries.sort_unstable_by(|a, b| {
self.pull_request_order[&number(b)]
.0
.cmp(&self.pull_request_order[&number(a)].0)
.then_with(|| number(b).cmp(&number(a)))
});
for (index, revision) in catalog.entries.iter().enumerate() {
self.pull_request_order
.get_mut(&number(revision))
.expect("known pull request")
.1 = index;
}
self.prepare_search();
self.refilter();
if let Some(target) = selected {
self.select_target(&target);
}
}
Err(error) => status.error = Some(error),
}
self.loading = self
.pull_request_categories
.iter()
.any(|status| !status.complete && status.error.is_none());
return true;
}
if self.request_kind.is_none() { if self.request_kind.is_none() {
return false; return false;
} }
@@ -530,27 +619,20 @@ impl DiffState {
self.loading = false; self.loading = false;
self.pending = None; self.pending = None;
match result { match result {
Ok(Outcome::ResolvedPullRequest { before, after })
if expected == Some(RequestKind::Comparison) =>
{
self.before = Some(before);
self.after = Some(after);
self.queue_comparison();
}
Ok(Outcome::Catalog(catalog)) if expected == Some(RequestKind::Catalog) => { Ok(Outcome::Catalog(catalog)) if expected == Some(RequestKind::Catalog) => {
let selected = self let selected = self
.entries() .entries()
.nth(self.selected) .nth(self.selected)
.map(|entry| entry.target.clone()); .map(|entry| entry.target.clone());
self.search_fields = catalog
.entries
.iter()
.map(|entry| {
let id = match &entry.target {
RevisionTarget::Commit(id) => id.as_str(),
_ => "",
};
[
entry.label.to_lowercase(),
id.to_lowercase(),
entry.description.to_lowercase(),
]
})
.collect();
self.catalog = Some(catalog); self.catalog = Some(catalog);
self.prepare_search();
self.category = self.category.min(self.categories().len() - 1); self.category = self.category.min(self.categories().len() - 1);
self.refilter(); self.refilter();
if let Some(target) = selected { if let Some(target) = selected {
@@ -626,6 +708,80 @@ impl DiffState {
} }
true true
} }
fn prepare_search(&mut self) {
self.search_fields = self
.catalog
.as_ref()
.into_iter()
.flat_map(|catalog| &catalog.entries)
.map(|entry| {
let id = match &entry.target {
RevisionTarget::Commit(id) => id.as_str(),
_ => "",
};
[
entry.label.to_lowercase(),
id.to_lowercase(),
entry.description.to_lowercase(),
]
})
.collect();
}
pub(crate) fn pull_request_partial(&self) -> bool {
self.pull_request_remote.is_some()
&& self
.pull_request_categories
.iter()
.enumerate()
.any(|(index, status)| {
(self.category == 2 || self.category == index) && !status.complete
})
}
pub(crate) fn pull_request_notice(&self) -> Option<String> {
self.pull_request_remote.as_ref()?;
let statuses: Vec<_> = self
.pull_request_categories
.iter()
.enumerate()
.filter(|(index, _)| self.category == 2 || self.category == *index)
.map(|(index, status)| {
let category = [Category::Open, Category::Closed][index];
let count = self.catalog.as_ref().map_or(0, |catalog| {
catalog
.entries
.iter()
.filter(|entry| entry.category == category)
.count()
});
let state = status.error.as_deref().unwrap_or(if status.complete {
"complete"
} else {
"loading…"
});
format!("{}: {count} loaded · {state}", category.label())
})
.collect();
Some(format!(
"{}{}",
statuses.join(" · "),
if self.pull_request_partial() {
" · partial search"
} else {
""
}
))
}
fn retry_pull_requests(&mut self) {
for (index, status) in self.pull_request_categories.iter_mut().enumerate() {
if (self.category == 2 || self.category == index) && status.error.take().is_some() {
self.pull_request_retries[index] += 1;
self.loading = true;
}
}
}
fn refilter(&mut self) { fn refilter(&mut self) {
self.visible.clear(); self.visible.clear();
@@ -767,7 +923,14 @@ impl DiffState {
Input::Down | Input::Ctrl('j') => self.navigate(true), Input::Down | Input::Ctrl('j') => self.navigate(true),
Input::Home => self.selected = 0, Input::Home => self.selected = 0,
Input::End => self.selected = self.visible.len().saturating_sub(1), Input::End => self.selected = self.visible.len().saturating_sub(1),
Input::Enter if !self.loading => self.choose(), Input::Enter
if !self.loading
|| (self.pull_request_remote.is_some()
&& self.request_kind == Some(RequestKind::Catalog)) =>
{
self.choose()
}
Input::Ctrl('r') if self.pull_request_remote.is_some() => self.retry_pull_requests(),
input if BackspaceKind::from_input(&input).is_some() => { input if BackspaceKind::from_input(&input).is_some() => {
if BackspaceKind::from_input(&input) if BackspaceKind::from_input(&input)
.expect("guard checked backspace input") .expect("guard checked backspace input")
@@ -791,8 +954,11 @@ impl DiffState {
fn choose(&mut self) { fn choose(&mut self) {
let revision = self.entries().nth(self.selected).cloned(); let revision = self.entries().nth(self.selected).cloned();
let Some(revision) = revision else { let Some(revision) = revision else {
self.retry_pull_requests();
// Listing can be retried without closing the modal. // Listing can be retried without closing the modal.
if self.catalog.is_none() { if self.catalog.is_none()
&& (self.pull_request_remote.is_none() || self.error.is_some())
{
self.queue(match &self.pull_request_remote { self.queue(match &self.pull_request_remote {
Some(remote) => Operation::PullRequests(remote.clone()), Some(remote) => Operation::PullRequests(remote.clone()),
None => Operation::Discover, None => Operation::Discover,
@@ -800,44 +966,18 @@ impl DiffState {
} }
return; return;
}; };
if let RevisionTarget::UnavailablePullRequest(error) = &revision.target { if matches!(revision.target, RevisionTarget::PullRequest { .. }) {
self.before = None; self.before = None;
self.after = None; self.after = None;
self.error = Some(error.clone()); self.comparison_mode = ComparisonMode::BranchChanges;
self.phase = Phase::After;
self.queue(Operation::ResolvePullRequest(revision));
return; return;
} }
if revision.target == RevisionTarget::Working { if revision.target == RevisionTarget::Working {
self.comparison_mode = ComparisonMode::Snapshots; self.comparison_mode = ComparisonMode::Snapshots;
} }
if let RevisionTarget::PullRequest { if let RevisionTarget::FetchRemote(remote) = &revision.target {
remote,
base_ref,
base_sha,
head_ref,
head_sha,
} = &revision.target
{
self.before = Some(Revision {
target: RevisionTarget::RemoteCommit {
remote: remote.clone(),
reference: base_ref.clone(),
id: base_sha.clone(),
},
label: base_ref.clone(),
description: String::new(),
category: revision.category,
});
self.after = Some(Revision {
target: RevisionTarget::RemoteCommit {
remote: remote.clone(),
reference: head_ref.clone(),
id: head_sha.clone(),
},
..revision
});
self.comparison_mode = ComparisonMode::BranchChanges;
self.queue_comparison();
} else if let RevisionTarget::FetchRemote(remote) = &revision.target {
self.queue(Operation::FetchRemote(remote.clone())); self.queue(Operation::FetchRemote(remote.clone()));
} else if let RevisionTarget::ListRemote(remote) = &revision.target { } else if let RevisionTarget::ListRemote(remote) = &revision.target {
self.queue(Operation::ListRemote(remote.clone())); self.queue(Operation::ListRemote(remote.clone()));
@@ -2421,36 +2561,40 @@ mod tests {
} }
} }
fn pull_request_catalog() -> Catalog { fn pull_page(closed: bool, rows: &[(u64, &str, &str)], has_more: bool) -> Outcome {
Catalog { Outcome::PullRequestPage {
kind: RepoKind::Git, closed,
entries: [ result: Ok(PullRequestPage {
("#1 Fix Ünicode", Category::Open), kind: RepoKind::Git,
("#2 Fix Ünicode", Category::Closed), remote: "origin".into(),
("#3 Other work", Category::Open), has_more,
] entries: rows
.into_iter() .iter()
.map(|(label, category)| { .map(|&(number, title, date)| {
revision( (
RevisionTarget::PullRequest { revision(
remote: "origin".into(), RevisionTarget::PullRequest {
base_ref: "refs/heads/main".into(), remote: "origin".into(),
base_sha: "base-sha".into(), number,
head_ref: "refs/pull/1/head".into(), },
head_sha: "head-sha".into(), &format!("#{number} {title}"),
}, "Alice · main ← topic",
label, if closed {
"Alice · main ← topic", Category::Closed
category, } else {
) Category::Open
}) },
.collect(), ),
notice: None, date.into(),
)
})
.collect(),
}),
} }
} }
#[test] #[test]
fn pull_request_categories_keep_query_and_select_both_pinned_branch_tips() { fn pull_request_categories_keep_query_and_select_loaded_rows_before_completion() {
let mut state = DiffState::pull_requests("origin".into()); let mut state = DiffState::pull_requests("origin".into());
assert_eq!( assert_eq!(
state.categories(), state.categories(),
@@ -2461,136 +2605,273 @@ mod tests {
for ch in "FIX üni Alice".chars() { for ch in "FIX üni Alice".chars() {
state.handle(Input::Char(ch)); state.handle(Input::Char(ch));
} }
state.apply_result(1, Ok(Outcome::Catalog(pull_request_catalog()))); state.apply_result(
1,
Ok(pull_page(
true,
&[(2, "Fix Ünicode", "2026-01-02T00:00:00Z")],
true,
)),
);
assert!(state.loading);
assert!(state.pull_request_partial());
assert_eq!(state.entries().next().unwrap().label, "#2 Fix Ünicode");
state.handle(Input::Char('\t'));
state.apply_result(
1,
Ok(pull_page(
false,
&[
(1, "Fix Ünicode", "2026-01-03T00:00:00Z"),
(3, "Other work", "2026-01-01T00:00:00Z"),
],
true,
)),
);
assert_eq!( assert_eq!(
state state
.entries() .entries()
.map(|entry| entry.label.as_str()) .map(|entry| entry.label.as_str())
.collect::<Vec<_>>(), .collect::<Vec<_>>(),
["#2 Fix Ünicode"] ["#1 Fix Ünicode", "#2 Fix Ünicode"]
); );
state.handle(Input::Char('\t')); state.handle(Input::Char('\t'));
assert_eq!(state.entries().len(), 2);
state.handle(Input::Char('\t'));
assert_eq!(state.query, "FIX üni Alice"); assert_eq!(state.query, "FIX üni Alice");
assert_eq!(
state
.entries()
.map(|entry| entry.label.as_str())
.collect::<Vec<_>>(),
["#1 Fix Ünicode"]
);
state.handle(Input::Ctrl('u'));
assert_eq!(state.entries().len(), 2);
state.comparison_mode = ComparisonMode::Snapshots; state.comparison_mode = ComparisonMode::Snapshots;
state.handle(Input::Enter); state.handle(Input::Enter);
assert_eq!(state.phase, Phase::After); assert_eq!(state.phase, Phase::After);
let Operation::Compare { assert!(state.before.is_none() && state.after.is_none());
before, assert_eq!(state.comparison_mode, ComparisonMode::BranchChanges);
after, assert!(matches!(
mode, state.take_request(),
buffers, Some(Operation::ResolvePullRequest(Revision {
} = state.take_request().unwrap() target: RevisionTarget::PullRequest { number: 1, .. },
else { ..
panic!("expected direct paired comparison"); }))
}; ));
assert_eq!(mode, ComparisonMode::BranchChanges); }
assert!(buffers.is_empty());
assert_eq!(before.label, "refs/heads/main"); #[test]
assert_eq!(after.label, "#1 Fix Ünicode"); fn pull_request_all_streams_sort_deduplicate_and_preserve_selection_identity() {
let mut state = DiffState::pull_requests("origin".into());
state.take_request();
state.handle(Input::BackTab);
state.apply_result(
1,
Ok(pull_page(
false,
&[
(1, "First", "2026-01-01T00:00:00Z"),
(2, "Second", "2026-01-02T00:00:00Z"),
],
true,
)),
);
state.handle(Input::End);
assert_eq!( assert_eq!(
before.target, state.entries().nth(state.selected).unwrap().label,
RevisionTarget::RemoteCommit { "#1 First"
remote: "origin".into(), );
reference: "refs/heads/main".into(), state.apply_result(1, Ok(pull_page(true, &[], true)));
id: "base-sha".into(), assert!(state.pull_request_partial());
} state.apply_result(
1,
Ok(pull_page(
true,
&[(3, "Newest", "2026-01-03T00:00:00Z")],
true,
)),
); );
assert_eq!( assert_eq!(
after.target, state.entries().nth(state.selected).unwrap().label,
RevisionTarget::RemoteCommit { "#1 First"
remote: "origin".into(), );
reference: "refs/pull/1/head".into(), state.apply_result(
id: "head-sha".into(), 1,
} Ok(pull_page(
false,
&[(2, "Second updated", "2026-01-02T00:00:00Z")],
false,
)),
);
assert_eq!(state.entries().len(), 3);
assert_eq!(
state
.entries()
.map(|entry| entry.label.as_str())
.collect::<Vec<_>>(),
["#3 Newest", "#2 Second updated", "#1 First"]
);
assert!(state.pull_request_partial());
state.apply_result(1, Ok(pull_page(true, &[], false)));
assert!(!state.pull_request_partial());
assert!(!state.loading);
assert_eq!(
state.entries().nth(state.selected).unwrap().label,
"#1 First"
); );
} }
#[test] #[test]
fn pull_request_loading_retry_and_escape_reject_stale_results() { fn pull_request_late_category_errors_retry_without_losing_rows_or_other_results() {
let mut editor = editor();
editor.execute("pull_requests origin");
let (id, _) = editor.take_diff_request().unwrap();
editor.apply_diff_result(
id,
Ok(pull_page(
false,
&[(1, "Open", "2026-01-01T00:00:00Z")],
true,
)),
);
editor.apply_diff_result(
id,
Ok(pull_page(
true,
&[(2, "Closed", "2026-01-02T00:00:00Z")],
true,
)),
);
editor.handle(Input::Char('\t'));
editor.apply_diff_result(
id,
Ok(Outcome::PullRequestPage {
closed: true,
result: Err("offline".into()),
}),
);
let diff = editor.diff().unwrap();
assert!(diff.pull_request_notice().unwrap().contains("offline"));
assert_eq!(diff.entries().next().unwrap().label, "#2 Closed");
assert!(diff.pull_request_partial());
editor.handle(Input::Ctrl('r'));
assert_eq!(editor.diff().unwrap().pull_request_retries, [0, 1]);
assert!(editor.take_diff_request().is_none());
editor.apply_diff_result(
id,
Ok(pull_page(
false,
&[(3, "More open", "2026-01-03T00:00:00Z")],
false,
)),
);
assert_eq!(
editor.diff().unwrap().entries().next().unwrap().label,
"#2 Closed"
);
editor.apply_diff_result(id, Ok(pull_page(true, &[], false)));
assert!(!editor.diff().unwrap().pull_request_partial());
editor.handle(Input::Enter);
let (_, operation) = editor.take_diff_request().unwrap();
assert!(matches!(operation, Operation::ResolvePullRequest(_)));
assert!(!editor.apply_diff_result(id, Ok(pull_page(false, &[], false))));
}
#[test]
fn pull_request_loading_retry_escape_and_reopen_reject_stale_results() {
let mut editor = editor(); let mut editor = editor();
editor editor
.apply(vec![Effect::Replace("unsaved source".into())]) .apply(vec![Effect::Replace("unsaved source".into())])
.unwrap(); .unwrap();
editor.execute("pull_requests origin"); editor.execute("pull_requests origin");
let (first, operation) = editor.take_diff_request().unwrap(); let (first, _) = editor.take_diff_request().unwrap();
assert!(matches!(operation, Operation::PullRequests(remote) if remote == "origin"));
editor.handle(Input::Ctrl('b'));
editor.handle(Input::Enter); editor.handle(Input::Enter);
assert!(editor.diff().unwrap().loading);
assert!(editor.take_diff_request().is_none()); assert!(editor.take_diff_request().is_none());
text(&mut editor, "alice"); text(&mut editor, "alice");
assert!(editor.apply_diff_result(first, Err("API unavailable".into()))); editor.apply_diff_result(
first,
Ok(Outcome::PullRequestPage {
closed: false,
result: Err("API unavailable".into()),
}),
);
editor.handle(Input::Enter);
assert_eq!(editor.diff().unwrap().pull_request_retries, [1, 0]);
assert!(editor.take_diff_request().is_none());
assert!(editor.apply_diff_result(
first,
Ok(pull_page(
false,
&[
(1, "Open", "2026-01-02T00:00:00Z"),
(2, "Other open", "2026-01-01T00:00:00Z"),
],
true
))
));
editor.handle(Input::Enter);
let (resolve, operation) = editor.take_diff_request().unwrap();
assert!(matches!(operation, Operation::ResolvePullRequest(_)));
assert!(!editor.apply_diff_result(first, Ok(pull_page(true, &[], false))));
editor.apply_diff_result(resolve, Err("Historical head no longer retained".into()));
assert!(editor.diff().unwrap().before.is_none());
assert!(editor.diff().unwrap().after.is_none());
editor.handle(Input::Down);
editor.handle(Input::Enter); editor.handle(Input::Enter);
let (retry, operation) = editor.take_diff_request().unwrap(); let (retry, operation) = editor.take_diff_request().unwrap();
assert!(matches!(operation, Operation::PullRequests(remote) if remote == "origin"));
assert_eq!(editor.diff().unwrap().query, "alice");
assert!(!editor.apply_diff_result(first, Ok(Outcome::Catalog(pull_request_catalog()))));
assert!(editor.apply_diff_result(retry, Ok(Outcome::Catalog(pull_request_catalog()))));
editor.handle(Input::Enter);
let (compare, operation) = editor.take_diff_request().unwrap();
assert!(matches!( assert!(matches!(
operation, operation,
Operation::Compare { Operation::ResolvePullRequest(Revision {
mode: ComparisonMode::BranchChanges, target: RevisionTarget::PullRequest { number: 2, .. },
.. ..
} })
)); ));
assert!(editor.apply_diff_result(compare, Err("fetch unavailable".into())));
editor.handle(Input::Enter);
let (compare_retry, operation) = editor.take_diff_request().unwrap();
assert!(matches!(operation, Operation::Compare { .. }));
editor.handle(Input::Escape); editor.handle(Input::Escape);
assert!(editor.diff().is_none()); assert!(editor.diff().is_none());
assert!(!editor.apply_diff_result(compare_retry, Ok(Outcome::Comparison(comparison())))); assert!(!editor.apply_diff_result(retry, Ok(Outcome::Comparison(comparison()))));
assert_eq!(editor.lines(), ["unsaved source"]); assert_eq!(editor.lines(), ["unsaved source"]);
assert!(editor.dirty()); assert!(editor.dirty());
editor.execute("pull_requests"); editor.execute("pull_requests");
let (reopened, operation) = editor.take_diff_request().unwrap(); let (reopened, _) = editor.take_diff_request().unwrap();
assert!(matches!(operation, Operation::PullRequests(remote) if remote.is_empty())); assert!(!editor.apply_diff_result(first, Ok(pull_page(false, &[], false))));
assert!(!editor.apply_diff_result(retry, Ok(Outcome::Catalog(pull_request_catalog())))); assert!(editor.apply_diff_result(reopened, Ok(pull_page(false, &[], false))));
let mut empty = pull_request_catalog(); assert!(editor.apply_diff_result(reopened, Ok(pull_page(true, &[], false))));
empty.entries.clear();
assert!(editor.apply_diff_result(reopened, Ok(Outcome::Catalog(empty))));
editor.handle(Input::Enter); editor.handle(Input::Enter);
assert!(editor.take_diff_request().is_none()); assert!(editor.take_diff_request().is_none());
editor.handle(Input::Escape);
assert!(editor.diff().is_none());
} }
#[test] #[test]
fn unavailable_pull_request_does_not_block_other_catalog_entries() { fn resolved_pull_request_modes_reuse_pinned_endpoints_without_resolving_again() {
let mut state = DiffState::pull_requests("origin".into()); let mut state = DiffState::pull_requests("origin".into());
state.take_request(); state.take_request();
let mut catalog = pull_request_catalog(); state.apply_result(
catalog.entries.insert( 1,
0, Ok(pull_page(
revision( false,
RevisionTarget::UnavailablePullRequest("Historical head no longer retained".into()), &[(1, "Open", "2026-01-01T00:00:00Z")],
"#194 Deleted head", true,
"unavailable", )),
Category::Open,
),
); );
state.apply_result(1, Ok(Outcome::Catalog(catalog)));
state.handle(Input::Enter); state.handle(Input::Enter);
assert_eq!( state.take_request();
state.error.as_deref(), let before = revision(
Some("Historical head no longer retained") RevisionTarget::RemoteCommit {
remote: "origin".into(),
reference: "refs/heads/main".into(),
id: "base-sha".into(),
},
"main",
"",
Category::Open,
);
let after = revision(
RevisionTarget::RemoteCommit {
remote: "origin".into(),
reference: "refs/pull/1/head".into(),
id: "head-sha".into(),
},
"#1 Open",
"",
Category::Open,
);
state.apply_result(
2,
Ok(Outcome::ResolvedPullRequest {
before: before.clone(),
after: after.clone(),
}),
); );
assert!(state.take_request().is_none());
state.handle(Input::Down);
state.handle(Input::Enter);
assert!(matches!( assert!(matches!(
state.take_request(), state.take_request(),
Some(Operation::Compare { Some(Operation::Compare {
@@ -2598,12 +2879,23 @@ mod tests {
.. ..
}) })
)); ));
state.apply_result(2, Err("fetch failed".into())); state.apply_result(3, Err("No common ancestor".into()));
state.handle(Input::Home); assert_eq!(state.before.as_ref(), Some(&before));
state.handle(Input::Enter); assert_eq!(state.after.as_ref(), Some(&after));
assert!(state.before.is_none() && state.after.is_none()); state.toggle_comparison_mode().unwrap();
assert!(state.toggle_comparison_mode().is_err()); let Some(Operation::Compare {
assert!(state.take_request().is_none()); before: pinned_before,
after: pinned_after,
mode,
..
}) = state.take_request()
else {
panic!("expected ordinary pinned comparison");
};
assert_eq!(
(pinned_before, pinned_after, mode),
(before, after, ComparisonMode::Snapshots)
);
} }
fn editor() -> Editor { fn editor() -> Editor {
+6
View File
@@ -2812,6 +2812,12 @@ impl Editor {
if let Some(owner) = self.diff_owner.filter(|owner| *owner != self.pane_id) { if let Some(owner) = self.diff_owner.filter(|owner| *owner != self.pane_id) {
return self.with_pane(owner, |editor| editor.apply_diff_result(id, result)); return self.with_pane(owner, |editor| editor.apply_diff_result(id, result));
} }
if matches!(&result, Ok(Outcome::PullRequestPage { .. })) {
return self
.diff
.as_mut()
.is_some_and(|diff| diff.apply_result(id, result));
}
self.outstanding_diff_request = None; self.outstanding_diff_request = None;
if self.diff.as_ref().is_some_and(DiffState::refresh_pending) if self.diff.as_ref().is_some_and(DiffState::refresh_pending)
&& !matches!(&result, Ok(Outcome::CheckoutRefresh(None))) && !matches!(&result, Ok(Outcome::CheckoutRefresh(None)))
+523 -135
View File
@@ -2,17 +2,26 @@
use crate::repository::{checked, run}; use crate::repository::{checked, run};
use serde_json::Value; use serde_json::Value;
use std::{ use std::{
collections::{BTreeMap, BTreeSet}, collections::BTreeMap,
path::Path, path::Path,
process::Command, process::Command,
sync::atomic::{AtomicBool, Ordering}, sync::atomic::{AtomicBool, Ordering},
}; };
pub(crate) struct PullRequest { pub(crate) struct PullRequestSummary {
pub number: u64, pub number: u64,
pub title: String, pub title: String,
pub description: String, pub description: String,
pub closed: bool, pub closed: bool,
pub created_at: String,
}
pub(crate) struct PullRequestPage {
pub entries: Vec<PullRequestSummary>,
pub has_more: bool,
}
pub(crate) struct PullRequest {
pub revisions: Result<PullRequestRevisions, String>, pub revisions: Result<PullRequestRevisions, String>,
} }
@@ -360,6 +369,246 @@ impl Forge {
} }
} }
impl Forge {
fn page_with(
&self,
closed: bool,
page: u32,
mut get: impl FnMut(&str) -> Result<Value, String>,
) -> Result<PullRequestPage, String> {
if page == 0 {
return Err("Forge page numbers start at 1".into());
}
let states: &[&str] = match (self.provider, closed) {
(Provider::Gitlab, true) => &["closed", "merged"],
(Provider::Gitlab, false) => &["opened"],
(_, true) => &["closed"],
(_, false) => &["open"],
};
let mut entries = BTreeMap::new();
let mut has_more = false;
for state in states {
let query = match self.provider {
Provider::Github => "per_page=50&sort=created&direction=desc",
Provider::Gitlab => "scope=all&per_page=50&order_by=created_at&sort=desc",
// Gitea's default sort is descending creation time; "oldest" reverses it.
Provider::Gitea => "limit=50",
};
let values = get(&format!(
"{}?state={state}&{query}&page={page}",
self.endpoint
))?;
let values = values
.as_array()
.ok_or("Forge API returned a non-array pull request list")?;
// Servers may cap page sizes. Only an empty page proves exhaustion.
has_more |= !values.is_empty();
for value in values {
let entry = summary(self.provider, value)?;
if entry.closed != closed {
return Err("Forge API returned a pull request in the wrong category".into());
}
entries.insert(entry.number, entry);
}
}
// GitLab needs two independently paged states for Closed. Later batches may
// precede older rows from the other state; consumers sort the partial catalog.
let mut entries: Vec<_> = entries.into_values().collect();
entries.sort_unstable_by(|a, b| {
b.created_at
.cmp(&a.created_at)
.then_with(|| b.number.cmp(&a.number))
});
Ok(PullRequestPage { entries, has_more })
}
fn resolve_with(
&self,
id: u64,
mut get: impl FnMut(&str) -> Result<Value, String>,
) -> Result<PullRequest, String> {
if id == 0 {
return Err("Pull request numbers start at 1".into());
}
let detail = get(&format!("{}/{id}", self.endpoint))?;
if number(
&detail,
if self.provider == Provider::Gitlab {
"iid"
} else {
"number"
},
)? != id
{
return Err("Forge API returned a different pull request".into());
}
let preserved_head = if self.provider == Provider::Gitea {
let repo = self.endpoint.strip_suffix("/pulls").unwrap();
let reference = get(&format!("{repo}/git/refs/pull/{id}/head"))?;
Some(gitea_head(&reference, id)?)
} else {
None
};
pull_request(self.provider, &detail, preserved_head)
}
}
fn gitea_head(value: &Value, id: u64) -> Result<String, String> {
// Gitea versions return either an object or an array for the filtered refs API.
let value = if let Some(values) = value.as_array() {
if values.len() != 1 {
return Err(format!(
"Gitea pull request #{id} has no unique preserved head reference"
));
}
&values[0]
} else {
value
};
if field(value, "ref")? != format!("refs/pull/{id}/head") {
return Err("Gitea API returned a different pull request reference".into());
}
let object = value
.get("object")
.ok_or("Gitea API returned no reference object")?;
if field(object, "type")? != "commit" {
return Err("Gitea API returned a non-commit pull request head".into());
}
sha(object, "sha")
}
fn created_at(value: &Value) -> Result<String, String> {
let invalid = || "Forge API returned invalid created_at".to_owned();
let text = field(value, "created_at")?;
let digits = |text: &str| -> Option<i32> {
(!text.is_empty() && text.bytes().all(|b| b.is_ascii_digit()))
.then(|| text.parse().ok())
.flatten()
};
if !text.is_ascii()
|| text.len() < 20
|| &text[4..5] != "-"
|| &text[7..8] != "-"
|| &text[10..11] != "T"
|| &text[13..14] != ":"
|| &text[16..17] != ":"
{
return Err(invalid());
}
let mut year = digits(&text[..4]).ok_or_else(invalid)?;
let mut month = digits(&text[5..7]).ok_or_else(invalid)?;
let mut day = digits(&text[8..10]).ok_or_else(invalid)?;
let hour = digits(&text[11..13]).ok_or_else(invalid)?;
let minute = digits(&text[14..16]).ok_or_else(invalid)?;
let second = digits(&text[17..19]).ok_or_else(invalid)?;
let days = |year: i32, month: i32| match month {
2 => 28 + i32::from(year % 4 == 0 && (year % 100 != 0 || year % 400 == 0)),
4 | 6 | 9 | 11 => 30,
_ => 31,
};
if year == 0
|| !(1..=12).contains(&month)
|| !(1..=days(year, month)).contains(&day)
|| hour > 23
|| minute > 59
|| second > 59
{
return Err(invalid());
}
let mut rest = &text[19..];
let mut nanos = 0;
if let Some(fraction) = rest.strip_prefix('.') {
let end = fraction
.find(|c: char| !c.is_ascii_digit())
.ok_or_else(invalid)?;
if end == 0 || end > 9 {
return Err(invalid());
}
nanos = digits(&fraction[..end]).ok_or_else(invalid)? * 10i32.pow(9 - end as u32);
rest = &fraction[end..];
}
let offset = if rest == "Z" {
0
} else if rest.len() == 6 && matches!(&rest[..1], "+" | "-") && &rest[3..4] == ":" {
let hours = digits(&rest[1..3]).ok_or_else(invalid)?;
let minutes = digits(&rest[4..6]).ok_or_else(invalid)?;
if hours > 23 || minutes > 59 {
return Err(invalid());
}
(hours * 60 + minutes) * if rest.starts_with('+') { 1 } else { -1 }
} else {
return Err(invalid());
};
let minutes = hour * 60 + minute - offset;
day += minutes.div_euclid(1440);
if day == 0 {
month -= 1;
if month == 0 {
year -= 1;
month = 12;
}
day = days(year, month);
} else if day > days(year, month) {
day = 1;
month += 1;
if month == 13 {
year += 1;
month = 1;
}
}
if !(1..=9999).contains(&year) {
return Err(invalid());
}
let minutes = minutes.rem_euclid(1440);
Ok(format!(
"{year:04}-{month:02}-{day:02}T{:02}:{:02}:{second:02}.{nanos:09}Z",
minutes / 60,
minutes % 60
))
}
fn summary(provider: Provider, value: &Value) -> Result<PullRequestSummary, String> {
let gitlab = provider == Provider::Gitlab;
let (closed, merged) = status(provider, value)?;
let author = author(provider, value)?;
let state = if merged {
"merged"
} else if closed {
"closed"
} else {
"open"
};
let (base, head) = if gitlab {
(
value.get("target_branch").and_then(Value::as_str),
value.get("source_branch").and_then(Value::as_str),
)
} else {
(
value.pointer("/base/ref").and_then(Value::as_str),
value
.pointer(if provider == Provider::Gitea {
"/head/label"
} else {
"/head/ref"
})
.and_then(Value::as_str)
.filter(|s| !s.is_empty())
.or_else(|| value.pointer("/head/ref").and_then(Value::as_str)),
)
};
Ok(PullRequestSummary {
number: number(value, if gitlab { "iid" } else { "number" })?,
title: field(value, "title")?.to_owned(),
description: match (base, head) {
(Some(base), Some(head)) => format!("{author} · {state} · {head} → {base}"),
_ => format!("{author} · {state}"),
},
closed,
created_at: created_at(value)?,
})
}
fn field<'a>(value: &'a Value, key: &str) -> Result<&'a str, String> { fn field<'a>(value: &'a Value, key: &str) -> Result<&'a str, String> {
value value
.get(key) .get(key)
@@ -401,13 +650,8 @@ fn branch<'a>(value: &'a Value, key: &str) -> Result<&'a str, String> {
} }
} }
fn pull_request( fn status(provider: Provider, value: &Value) -> Result<(bool, bool), String> {
provider: Provider,
value: &Value,
preserved_head: Option<String>,
) -> Result<PullRequest, String> {
let gitlab = provider == Provider::Gitlab; let gitlab = provider == Provider::Gitlab;
let number = number(value, if gitlab { "iid" } else { "number" })?;
let state = field(value, "state")?; let state = field(value, "state")?;
let closed = match (gitlab, state) { let closed = match (gitlab, state) {
(false, "open") | (true, "opened" | "locked") => false, (false, "open") | (true, "opened" | "locked") => false,
@@ -429,6 +673,29 @@ fn pull_request(
if merged && !closed { if merged && !closed {
return Err("Forge API returned inconsistent merged state".into()); return Err("Forge API returned inconsistent merged state".into());
} }
Ok((closed, merged))
}
fn author(provider: Provider, value: &Value) -> Result<&str, String> {
let gitlab = provider == Provider::Gitlab;
let author = value
.get(if gitlab { "author" } else { "user" })
.ok_or("Forge API returned no author")?;
if author.is_null() {
Ok("deleted user")
} else {
field(author, if gitlab { "username" } else { "login" })
}
}
fn pull_request(
provider: Provider,
value: &Value,
preserved_head: Option<String>,
) -> Result<PullRequest, String> {
let gitlab = provider == Provider::Gitlab;
let number = number(value, if gitlab { "iid" } else { "number" })?;
status(provider, value)?;
let revisions = (|| -> Result<_, String> { let revisions = (|| -> Result<_, String> {
let (base, head, base_sha, head_sha) = if gitlab { let (base, head, base_sha, head_sha) = if gitlab {
let refs = value.get("diff_refs").filter(|v| v.is_object()).ok_or( let refs = value.get("diff_refs").filter(|v| v.is_object()).ok_or(
@@ -502,143 +769,46 @@ fn pull_request(
head, head,
)) ))
})(); })();
let author = value
.get(if gitlab { "author" } else { "user" })
.ok_or("Forge API returned no author")?;
let author = if author.is_null() {
"deleted user"
} else {
field(author, if gitlab { "username" } else { "login" })?
};
let state = if merged {
"merged"
} else if closed {
"closed"
} else {
"open"
};
Ok(PullRequest { Ok(PullRequest {
number,
title: field(value, "title")?.to_owned(),
description: match &revisions {
Ok((_, base, head)) => format!("{author} · {state} · {head} → {base}"),
Err(error) => format!("{author} · {state} · unavailable: {error}"),
},
closed,
revisions: revisions.map(|(revisions, _, _)| revisions), revisions: revisions.map(|(revisions, _, _)| revisions),
}) })
} }
pub(crate) fn list( pub(crate) fn page(
url: &str, url: &str,
provider: Option<&str>, provider: Option<&str>,
closed: bool,
page: u32,
root: &Path, root: &Path,
cancelled: &AtomicBool, cancelled: &AtomicBool,
) -> Result<Vec<PullRequest>, String> { ) -> Result<PullRequestPage, String> {
let forge = Forge::parse(url, provider, |destination, port| { let forge = Forge::parse(url, provider, |destination, port| {
let mut command = Command::new("ssh"); let mut command = Command::new("ssh");
command.current_dir(root); command.current_dir(root);
ssh_hostname(command, destination, port, cancelled) ssh_hostname(command, destination, port, cancelled)
})?; })?;
let token = forge.token(root, cancelled)?; let token = forge.token(root, cancelled)?;
let mut gitea_heads = BTreeMap::new(); forge.page_with(closed, page, |url| {
let query = match forge.provider { forge.get(url, token.as_deref(), root, cancelled)
Provider::Github => "state=all&per_page=100&sort=created&direction=asc", })
Provider::Gitlab => "state=all&scope=all&per_page=100&order_by=created_at&sort=asc", }
Provider::Gitea => "state=all&limit=100&sort=oldest",
}; pub(crate) fn resolve(
let mut requests = Vec::new(); url: &str,
let mut seen = BTreeSet::new(); provider: Option<&str>,
for page in 1u64.. { number: u64,
let values = forge.get( root: &Path,
&format!("{}?{query}&page={page}", forge.endpoint), cancelled: &AtomicBool,
token.as_deref(), ) -> Result<PullRequest, String> {
root, let forge = Forge::parse(url, provider, |destination, port| {
cancelled, let mut command = Command::new("ssh");
)?; command.current_dir(root);
let values = values ssh_hostname(command, destination, port, cancelled)
.as_array() })?;
.ok_or("Forge API returned a non-array pull request list")?; let token = forge.token(root, cancelled)?;
// A server may cap the requested page size. Only an empty page proves exhaustion. forge.resolve_with(number, |url| {
if values.is_empty() { forge.get(url, token.as_deref(), root, cancelled)
return Ok(requests); })
}
if page == 1 && forge.provider == Provider::Gitea {
let repo = forge.endpoint.strip_suffix("/pulls").unwrap();
let refs = forge.get(
&format!("{repo}/git/refs/pull"),
token.as_deref(),
root,
cancelled,
)?;
for reference in refs
.as_array()
.ok_or("Gitea API returned a non-array reference list")?
{
let name = field(reference, "ref")?;
if let Some(id) = name
.strip_prefix("refs/pull/")
.and_then(|s| s.strip_suffix("/head"))
{
let id = id
.parse::<u64>()
.ok()
.filter(|n| *n > 0)
.ok_or("Gitea API returned an invalid pull request reference")?;
let object = reference
.get("object")
.ok_or("Gitea API returned no reference object")?;
if field(object, "type")? != "commit"
|| gitea_heads.insert(id, sha(object, "sha")?).is_some()
{
return Err(
"Gitea API returned an invalid or repeated pull request head".into(),
);
}
}
}
}
for value in values {
let id = number(
value,
if forge.provider == Provider::Gitlab {
"iid"
} else {
"number"
},
)?;
if !seen.insert(id) {
return Err(
"Forge API repeated a pull request while paginating; retry the list".into(),
);
}
let detail;
// ponytail: GitLab needs one detail read per MR; batch if large histories become slow.
let value = if forge.provider == Provider::Gitlab
&& value.get("diff_refs").is_none_or(Value::is_null)
{
detail = forge.get(
&format!("{}/{id}", forge.endpoint),
token.as_deref(),
root,
cancelled,
)?;
if number(&detail, "iid")? != id {
return Err("GitLab API returned a different merge request".into());
}
&detail
} else {
value
};
let preserved_head = if forge.provider == Provider::Gitea {
gitea_heads.remove(&id)
} else {
None
};
requests.push(pull_request(forge.provider, value, preserved_head)?);
}
}
Err("Forge pagination overflow".into())
} }
#[cfg(test)] #[cfg(test)]
@@ -652,6 +822,229 @@ mod tests {
}) })
} }
#[test]
fn pages_are_bounded_newest_first_summaries_without_historical_metadata() {
for (provider, url, query) in [
(
Provider::Github,
"https://github.com/owner/repo",
"state=open&per_page=50&sort=created&direction=desc&page=3",
),
(
Provider::Gitlab,
"https://gitlab.com/owner/repo",
"state=opened&scope=all&per_page=50&order_by=created_at&sort=desc&page=3",
),
(
Provider::Gitea,
"https://codeberg.org/owner/repo",
"state=open&limit=50&page=3",
),
] {
let forge = parse(url, None).unwrap();
let mut calls = 0;
let page = forge.page_with(false, 3, |url| {
calls += 1;
assert_eq!(url, format!("{}?{query}", forge.endpoint));
Ok(json!([
{"number": 1, "iid": 1, "title": "Older", "state": if provider == Provider::Gitlab { "opened" } else { "open" },
"user": null, "author": null, "merged_at": null, "merged": false, "created_at": "2024-01-01T01:00:00+01:00"},
{"number": 2, "iid": 2, "title": "Newer", "state": if provider == Provider::Gitlab { "opened" } else { "open" },
"user": {"login": "author"}, "author": {"username": "author"}, "merged_at": null, "merged": false,
"created_at": "2024-01-01T00:00:00.001Z", "diff_refs": null, "head": {"sha": "not-a-commit"}}
]))
}).unwrap();
assert_eq!(calls, 1);
assert_eq!(
page.entries.iter().map(|p| p.number).collect::<Vec<_>>(),
[2, 1]
);
assert_eq!(page.entries[0].description, "author · open");
assert!(page.entries[1].description.contains("deleted user"));
assert!(
page.has_more,
"A short, server-capped page is not exhaustion"
);
let empty = forge.page_with(false, 4, |_| Ok(json!([]))).unwrap();
assert!(empty.entries.is_empty() && !empty.has_more);
assert!(
forge
.page_with(false, 0, |_| panic!("Page zero must not request"))
.is_err()
);
}
}
#[test]
fn gitlab_closed_combines_closed_and_merged_pages_without_open_requests() {
let forge = parse("https://gitlab.com/group/repo", None).unwrap();
let mut all = Vec::new();
for page in 1..=3 {
let mut states = Vec::new();
let result = forge
.page_with(true, page, |url| {
assert!(url.ends_with(&format!("&page={page}")));
let state = if url.contains("?state=closed&") {
"closed"
} else {
assert!(url.contains("?state=merged&"));
"merged"
};
states.push(state);
let (id, date) = match (state, page) {
("closed", 1) => (1, "2024-01-01T00:00:00Z"),
("merged", 1) => (3, "2024-03-01T00:00:00Z"),
("merged", 2) => (2, "2024-02-01T00:00:00Z"),
_ => return Ok(json!([])),
};
Ok(json!([{"iid": id, "title": state, "state": state,
"author": null, "created_at": date, "diff_refs": null}]))
})
.unwrap();
assert_eq!(states, ["closed", "merged"]);
assert_eq!(result.has_more, page != 3);
assert!(result.entries.iter().all(|p| p.closed));
if page == 1 {
assert_eq!(
result.entries.iter().map(|p| p.number).collect::<Vec<_>>(),
[3, 1]
);
}
// The second merged page still precedes the first closed page.
all.extend(result.entries);
}
all.sort_unstable_by(|a, b| b.created_at.cmp(&a.created_at));
assert_eq!(all.iter().map(|p| p.number).collect::<Vec<_>>(), [3, 2, 1]);
let raced = forge.page_with(true, 1, |url| {
Ok(json!([{"iid": 7, "title": "Raced merge", "state": if url.contains("?state=merged&") { "merged" } else { "closed" },
"author": null, "created_at": "2024-01-01T00:00:00Z"}]))
}).unwrap();
assert_eq!(raced.entries.len(), 1);
assert!(raced.entries[0].description.contains("merged"));
}
#[test]
fn timestamps_normalize_offsets_precision_and_calendar_boundaries() {
for (input, expected) in [
(
"2024-03-01T00:30:12.5+01:00",
"2024-02-29T23:30:12.500000000Z",
),
(
"2023-12-31T23:30:00-01:00",
"2024-01-01T00:30:00.000000000Z",
),
(
"2024-01-01T00:00:00.000000001Z",
"2024-01-01T00:00:00.000000001Z",
),
] {
assert_eq!(created_at(&json!({"created_at": input})).unwrap(), expected);
}
for input in [
"",
"not a date",
"2023-02-29T00:00:00Z",
"2024-13-01T00:00:00Z",
"2024-01-01T24:00:00Z",
"2024-01-01T00:00:00",
"2024-01-01T00:00:00+24:00",
"2024-01-01T00:00:00.Z",
"2024-01-01T00:00:00.1234567890Z",
"0001-01-01T00:00:00+01:00",
"9999-12-31T23:00:00-02:00",
] {
assert!(
created_at(&json!({"created_at": input})).is_err(),
"{input}"
);
}
}
#[test]
fn resolution_reads_only_selected_detail_and_exact_preserved_head() {
for (url, provider) in [
("https://github.com/owner/repo", Provider::Github),
("https://gitlab.com/group/repo", Provider::Gitlab),
("https://codeberg.org/owner/repo", Provider::Gitea),
] {
let forge = parse(url, None).unwrap();
let mut calls = Vec::new();
let pr = forge.resolve_with(7, |url| {
calls.push(url.to_owned());
if url == format!("{}/7", forge.endpoint) {
Ok(json!({"number": 7, "iid": 7, "title": "Selected", "state": if provider == Provider::Gitlab { "merged" } else { "closed" },
"merged_at": "2024-01-01T00:00:00Z", "merged": true, "user": null, "author": null,
"base": {"ref": "main", "sha": "1".repeat(40)}, "head": {"ref": "topic", "sha": "2".repeat(40)},
"merge_base": "3".repeat(40), "target_branch": "main", "source_branch": "topic",
"diff_refs": {"start_sha": null, "base_sha": "4".repeat(40), "head_sha": "5".repeat(40)},
"url": "https://evil.example/detail", "references": "https://evil.example/refs"}))
} else {
assert_eq!(url, format!("{}/git/refs/pull/7/head", forge.endpoint.strip_suffix("/pulls").unwrap()));
Ok(json!([{"ref": "refs/pull/7/head", "object": {"type": "commit", "sha": "6".repeat(40)}}]))
}
}).unwrap();
assert_eq!(calls.len(), if provider == Provider::Gitea { 2 } else { 1 });
let revisions = pr.revisions.unwrap();
assert_eq!(
revisions.base_sha,
match provider {
Provider::Github => "1",
Provider::Gitlab => "4",
Provider::Gitea => "3",
}
.repeat(40)
);
assert_eq!(
revisions.head_sha,
match provider {
Provider::Github => "2",
Provider::Gitlab => "5",
Provider::Gitea => "6",
}
.repeat(40)
);
assert!(
forge
.resolve_with(0, |_| panic!("Zero must not request"))
.is_err()
);
let mut calls = 0;
assert!(
forge
.resolve_with(7, |_| {
calls += 1;
Ok(json!({"number": 8, "iid": 8}))
})
.is_err()
);
assert_eq!(
calls, 1,
"Reject wrong detail identity before reading historical refs"
);
}
}
#[test]
fn gitea_preserved_heads_reject_missing_ambiguous_or_foreign_refs() {
let reference =
json!({"ref": "refs/pull/7/head", "object": {"type": "commit", "sha": "a".repeat(40)}});
assert_eq!(gitea_head(&reference, 7).unwrap(), "a".repeat(40));
assert_eq!(
gitea_head(&json!([reference.clone()]), 7).unwrap(),
"a".repeat(40)
);
for bad in [
json!([]),
json!([reference.clone(), reference.clone()]),
json!({"ref": "refs/pull/8/head", "object": reference["object"]}),
json!({"ref": "refs/pull/7/head", "object": {"type": "tag", "sha": "a".repeat(40)}}),
json!({"ref": "refs/pull/7/head", "object": {"type": "commit", "sha": "0".repeat(40)}}),
] {
assert!(gitea_head(&bad, 7).is_err());
}
}
#[test] #[test]
fn ssh_aliases_use_effective_hostname_and_preserve_user_matching() { fn ssh_aliases_use_effective_hostname_and_preserve_user_matching() {
use std::{fs, io::Write, path::PathBuf}; use std::{fs, io::Write, path::PathBuf};
@@ -788,9 +1181,7 @@ mod tests {
} }
.repeat(40) .repeat(40)
); );
assert!(pr.closed && pr.description.contains("merged"));
if provider == Provider::Gitea { if provider == Provider::Gitea {
assert!(pr.description.contains("fork:topic"));
let mut deleted = value.clone(); let mut deleted = value.clone();
deleted["head"]["sha"] = Value::String(String::new()); deleted["head"]["sha"] = Value::String(String::new());
assert_eq!( assert_eq!(
@@ -802,9 +1193,6 @@ mod tests {
"5".repeat(40) "5".repeat(40)
); );
let unavailable = pull_request(provider, &deleted, None).unwrap(); let unavailable = pull_request(provider, &deleted, None).unwrap();
assert_eq!(unavailable.number, 7);
assert!(unavailable.closed);
assert!(unavailable.description.contains("unavailable"));
assert!(unavailable.revisions.is_err()); assert!(unavailable.revisions.is_err());
} }
} }
+80 -27
View File
@@ -1,7 +1,7 @@
//! Read-only active-repository discovery and isolated revision comparison. //! Read-only active-repository discovery and isolated revision comparison.
use crate::diff::{ use crate::diff::{
Catalog, Category, ChangedFile, Comparison, ComparisonMode, FileStatus, RepoKind, Revision, Catalog, Category, ChangedFile, Comparison, ComparisonMode, FileStatus, PullRequestPage,
RevisionTarget, line_hunks, RepoKind, Revision, RevisionTarget, line_hunks,
}; };
use std::{ use std::{
collections::{BTreeMap, BTreeSet}, collections::{BTreeMap, BTreeSet},
@@ -1535,7 +1535,10 @@ impl Repository {
self.load_remote(remote, None)?; self.load_remote(remote, None)?;
self.catalog() self.catalog()
} }
pub fn pull_requests(&mut self, name: &str) -> Result<Catalog, String> { fn pull_request_remote(
&mut self,
name: &str,
) -> Result<(String, String, Option<String>), String> {
self.discover_remotes()?; self.discover_remotes()?;
let remote = if name.is_empty() { let remote = if name.is_empty() {
self.remotes self.remotes
@@ -1561,40 +1564,91 @@ impl Repository {
} else { } else {
Some(text(checked(output)?)?.trim().to_owned()) Some(text(checked(output)?)?.trim().to_owned())
}; };
let pulls = crate::forge::list( Ok((remote.name.clone(), remote.url.clone(), provider))
&remote.url, }
pub fn pull_request_page(
&mut self,
name: &str,
closed: bool,
page: u32,
) -> Result<PullRequestPage, String> {
let (remote, url, provider) = self.pull_request_remote(name)?;
let page = crate::forge::page(
&url,
provider.as_deref(), provider.as_deref(),
closed,
page,
&self.root, &self.root,
&self.cancelled, &self.cancelled,
) )
.map_err(|error| error.replace("remote.<name>", &format!("remote.{}", remote.name)))?; .map_err(|error| error.replace("remote.<name>", &format!("remote.{remote}")))?;
Ok(Catalog { Ok(PullRequestPage {
kind: self.kind, kind: self.kind,
notice: Some(format!("Pull requests from {}", remote.name)), remote: remote.clone(),
entries: pulls has_more: page.has_more,
entries: page
.entries
.into_iter() .into_iter()
.map(|pull| Revision { .map(|pull| {
target: match pull.revisions { (
Ok(revisions) => RevisionTarget::PullRequest { Revision {
remote: remote.name.clone(), target: RevisionTarget::PullRequest {
base_ref: revisions.base_ref, remote: remote.clone(),
base_sha: revisions.base_sha, number: pull.number,
head_ref: revisions.head_ref, },
head_sha: revisions.head_sha, label: format!("#{} {}", pull.number, pull.title),
description: pull.description,
category: if pull.closed {
Category::Closed
} else {
Category::Open
},
}, },
Err(error) => RevisionTarget::UnavailablePullRequest(error), pull.created_at,
}, )
label: format!("#{} {}", pull.number, pull.title),
description: pull.description,
category: if pull.closed {
Category::Closed
} else {
Category::Open
},
}) })
.collect(), .collect(),
}) })
} }
pub fn resolve_pull_request(
&mut self,
revision: Revision,
) -> Result<(Revision, Revision), String> {
let RevisionTarget::PullRequest { remote, number } = &revision.target else {
return Err("Select a pull request".into());
};
let (remote, url, provider) = self.pull_request_remote(remote)?;
let pull = crate::forge::resolve(
&url,
provider.as_deref(),
*number,
&self.root,
&self.cancelled,
)
.map_err(|error| error.replace("remote.<name>", &format!("remote.{remote}")))?;
let revisions = pull.revisions?;
let before = Revision {
target: RevisionTarget::RemoteCommit {
remote: remote.clone(),
reference: revisions.base_ref.clone(),
id: revisions.base_sha,
},
label: revisions.base_ref,
description: String::new(),
category: revision.category,
};
let after = Revision {
target: RevisionTarget::RemoteCommit {
remote,
reference: revisions.head_ref,
id: revisions.head_sha,
},
..revision
};
Ok((before, after))
}
fn resolve(&mut self, target: &RevisionTarget) -> Result<(PathBuf, String), String> { fn resolve(&mut self, target: &RevisionTarget) -> Result<(PathBuf, String), String> {
let id = match target { let id = match target {
RevisionTarget::Commit(id) | RevisionTarget::RemoteCommit { id, .. } => { RevisionTarget::Commit(id) | RevisionTarget::RemoteCommit { id, .. } => {
@@ -1620,7 +1674,6 @@ impl Repository {
})? })?
.clone(), .clone(),
RevisionTarget::Working => return Err("Working checkout is not a commit".into()), RevisionTarget::Working => return Err("Working checkout is not a commit".into()),
RevisionTarget::UnavailablePullRequest(error) => return Err(error.clone()),
RevisionTarget::FetchRemote(_) RevisionTarget::FetchRemote(_)
| RevisionTarget::ListRemote(_) | RevisionTarget::ListRemote(_)
| RevisionTarget::PullRequest { .. } => { | RevisionTarget::PullRequest { .. } => {
+297 -6
View File
@@ -26,6 +26,153 @@ use crate::{
type DiffResult = (u64, Result<DiffOutcome, String>); type DiffResult = (u64, Result<DiffOutcome, String>);
#[derive(Clone, Copy)]
struct PullRequestJob {
page: u32,
busy: bool,
complete: bool,
failed: bool,
retry: u64,
}
struct PullRequestWorker {
id: u64,
requests: Vec<Sender<u32>>,
threads: Vec<JoinHandle<()>>,
cancelled: Arc<AtomicBool>,
jobs: [PullRequestJob; 2],
priority: usize,
}
impl PullRequestWorker {
fn start(
id: u64,
root: &Path,
remote: String,
outgoing: Sender<DiffResult>,
) -> Result<Self, String> {
let cancelled = Arc::new(AtomicBool::new(false));
let fetches = std::array::from_fn(|index| {
let root = root.to_path_buf();
let remote = remote.clone();
let cancelled = Arc::clone(&cancelled);
let mut repository = None;
move |page| {
if repository.is_none() {
repository = Some(Repository::open(&root, Arc::clone(&cancelled))?);
}
repository
.as_mut()
.expect("repository opened")
.pull_request_page(&remote, index == 1, page)
}
});
Self::start_with(id, outgoing, cancelled, fetches)
}
fn start_with(
id: u64,
outgoing: Sender<DiffResult>,
cancelled: Arc<AtomicBool>,
fetches: [impl FnMut(u32) -> Result<crate::diff::PullRequestPage, String> + Send + 'static;
2],
) -> Result<Self, String> {
let mut worker = Self {
id,
requests: Vec::with_capacity(2),
threads: Vec::with_capacity(2),
cancelled,
jobs: [PullRequestJob {
page: 1,
busy: false,
complete: false,
failed: false,
retry: 0,
}; 2],
priority: 0,
};
for (index, mut fetch) in fetches.into_iter().enumerate() {
let (requests, incoming) = mpsc::channel();
let outgoing = outgoing.clone();
let cancelled = Arc::clone(&worker.cancelled);
let thread = thread::Builder::new()
.name(format!("vamp-pull-requests-{index}"))
.spawn(move || {
while let Ok(page) = incoming.recv() {
if cancelled.load(Ordering::Acquire) {
break;
}
let result = fetch(page);
if cancelled.load(Ordering::Acquire)
|| outgoing
.send((
id,
Ok(DiffOutcome::PullRequestPage {
closed: index == 1,
result,
}),
))
.is_err()
{
break;
}
}
})
.map_err(|error| format!("Cannot start pull request worker: {error}"))?;
worker.requests.push(requests);
worker.threads.push(thread);
}
worker.schedule()?;
Ok(worker)
}
fn update(&mut self, category: usize, retries: [u64; 2]) -> Result<(), String> {
self.priority = usize::from(category == 1);
for (job, retry) in self.jobs.iter_mut().zip(retries) {
if job.retry != retry {
job.retry = retry;
job.failed = false;
}
}
self.schedule()
}
fn complete(&mut self, closed: bool, result: &Result<crate::diff::PullRequestPage, String>) {
let job = &mut self.jobs[usize::from(closed)];
job.busy = false;
match result {
Ok(page) => {
job.complete = !page.has_more;
job.page += 1;
}
Err(_) => job.failed = true,
}
}
fn schedule(&mut self) -> Result<(), String> {
for index in [self.priority, 1 - self.priority] {
let job = &mut self.jobs[index];
if !job.busy && !job.complete && !job.failed {
self.requests[index]
.send(job.page)
.map_err(|_| "Pull request worker stopped unexpectedly")?;
job.busy = true;
}
}
Ok(())
}
}
impl Drop for PullRequestWorker {
fn drop(&mut self) {
self.cancelled.store(true, Ordering::Release);
self.requests.clear();
for thread in self.threads.drain(..) {
let _ = thread.join();
}
}
}
struct DiffWorker { struct DiffWorker {
requests: Option<Sender<(u64, DiffOperation)>>, requests: Option<Sender<(u64, DiffOperation)>>,
results: Option<Receiver<DiffResult>>, results: Option<Receiver<DiffResult>>,
@@ -33,13 +180,18 @@ struct DiffWorker {
stopped: Arc<AtomicBool>, stopped: Arc<AtomicBool>,
cancelled_request: Arc<AtomicU64>, cancelled_request: Arc<AtomicU64>,
thread: Option<JoinHandle<()>>, thread: Option<JoinHandle<()>>,
root: PathBuf,
outgoing: Sender<DiffResult>,
pull_requests: Option<PullRequestWorker>,
pending: Option<u64>, pending: Option<u64>,
} }
impl DiffWorker { impl DiffWorker {
fn start(root: PathBuf) -> Result<Self, String> { fn start(root: PathBuf) -> Result<Self, String> {
let (requests, incoming) = mpsc::channel::<(u64, DiffOperation)>(); let (requests, incoming) = mpsc::channel::<(u64, DiffOperation)>();
let (outgoing, results) = mpsc::sync_channel(1); let (outgoing, results) = mpsc::channel();
let sender = outgoing.clone();
let worker_root = root.clone();
let cancelled = Arc::new(AtomicBool::new(false)); let cancelled = Arc::new(AtomicBool::new(false));
let stopped = Arc::new(AtomicBool::new(false)); let stopped = Arc::new(AtomicBool::new(false));
let cancelled_request = Arc::new(AtomicU64::new(0)); let cancelled_request = Arc::new(AtomicU64::new(0));
@@ -63,16 +215,23 @@ impl DiffWorker {
} }
let result = (|| { let result = (|| {
if repository.is_none() { if repository.is_none() {
repository = Some(Repository::open(&root, Arc::clone(&cancellation))?); repository =
Some(Repository::open(&worker_root, Arc::clone(&cancellation))?);
} }
let repository = repository.as_mut().expect("repository opened"); let repository = repository.as_mut().expect("repository opened");
match operation { match operation {
DiffOperation::Discover => { DiffOperation::Discover => {
repository.catalog().map(DiffOutcome::Catalog) repository.catalog().map(DiffOutcome::Catalog)
} }
DiffOperation::PullRequests(remote) => { DiffOperation::PullRequests(_) => {
repository.pull_requests(&remote).map(DiffOutcome::Catalog) unreachable!("listing uses category workers")
} }
DiffOperation::ResolvePullRequest(revision) => repository
.resolve_pull_request(revision)
.map(|(before, after)| DiffOutcome::ResolvedPullRequest {
before,
after,
}),
DiffOperation::ListRemote(remote) => { DiffOperation::ListRemote(remote) => {
repository.list_remote(&remote).map(DiffOutcome::Catalog) repository.list_remote(&remote).map(DiffOutcome::Catalog)
} }
@@ -119,15 +278,29 @@ impl DiffWorker {
stopped, stopped,
cancelled_request, cancelled_request,
thread: Some(thread), thread: Some(thread),
root,
outgoing: sender,
pull_requests: None,
pending: None, pending: None,
}) })
} }
fn request(&mut self, id: u64, operation: DiffOperation) -> Result<(), String> { fn request(&mut self, id: u64, operation: DiffOperation) -> Result<(), String> {
self.pull_requests = None;
if let Some(previous) = self.pending.take() { if let Some(previous) = self.pending.take() {
self.cancelled_request.store(previous, Ordering::Release); self.cancelled_request.store(previous, Ordering::Release);
self.cancelled.store(true, Ordering::Release); self.cancelled.store(true, Ordering::Release);
} }
if let DiffOperation::PullRequests(remote) = operation {
self.pull_requests = Some(PullRequestWorker::start(
id,
&self.root,
remote,
self.outgoing.clone(),
)?);
self.pending = Some(id);
return Ok(());
}
self.requests self.requests
.as_ref() .as_ref()
.expect("live diff worker has a sender") .expect("live diff worker has a sender")
@@ -141,6 +314,7 @@ impl DiffWorker {
impl Drop for DiffWorker { impl Drop for DiffWorker {
fn drop(&mut self) { fn drop(&mut self) {
self.stopped.store(true, Ordering::Release); self.stopped.store(true, Ordering::Release);
self.pull_requests = None;
self.cancelled.store(true, Ordering::Release); self.cancelled.store(true, Ordering::Release);
self.requests.take(); self.requests.take();
self.results.take(); self.results.take();
@@ -401,6 +575,15 @@ impl EditorSession {
let Some(diff) = self.editor.tab_diff(*tab) else { let Some(diff) = self.editor.tab_diff(*tab) else {
return false; return false;
}; };
if let Some(listing) = &mut worker.pull_requests {
if let Err(error) = listing.update(diff.category, diff.pull_request_retries) {
if let Some(id) = worker.pending.take() {
self.editor.apply_diff_result(id, Err(error));
}
return false;
}
return true;
}
if !diff.loading if !diff.loading
&& !diff.refresh_pending() && !diff.refresh_pending()
&& let Some(id) = worker.pending.take() && let Some(id) = worker.pending.take()
@@ -447,12 +630,28 @@ impl EditorSession {
.try_recv(); .try_recv();
match result { match result {
Ok((id, result)) => { Ok((id, result)) => {
if worker.pending == Some(id) { if let Ok(DiffOutcome::PullRequestPage { closed, result }) = &result {
if let Some(listing) = worker
.pull_requests
.as_mut()
.filter(|listing| listing.id == id)
{
listing.complete(*closed, result);
}
} else if worker.pending == Some(id) {
worker.pending = None; worker.pending = None;
} }
changed |= self.editor.apply_diff_result(id, result); changed |= self.editor.apply_diff_result(id, result);
} }
Err(mpsc::TryRecvError::Empty) => return true, Err(mpsc::TryRecvError::Empty) => {
if let Some(listing) = &mut worker.pull_requests
&& let Err(error) = listing.schedule()
{
changed |= self.editor.apply_diff_result(listing.id, Err(error));
return false;
}
return true;
}
Err(mpsc::TryRecvError::Disconnected) => { Err(mpsc::TryRecvError::Disconnected) => {
if let Some(id) = worker.pending.take() { if let Some(id) = worker.pending.take() {
changed |= self.editor.apply_diff_result( changed |= self.editor.apply_diff_result(
@@ -1485,6 +1684,98 @@ mod tests {
sync::atomic::{AtomicU64, Ordering}, sync::atomic::{AtomicU64, Ordering},
}; };
#[test]
fn pull_request_pages_stream_concurrently_and_retry_only_the_failed_category() {
use crate::diff::{PullRequestPage, RepoKind};
let (outgoing, results) = mpsc::channel();
let (started, starts) = mpsc::channel();
let (open, open_gate) = mpsc::channel::<Result<bool, String>>();
let (closed, closed_gate) = mpsc::channel::<Result<bool, String>>();
let mut gates = [Some(open_gate), Some(closed_gate)];
let cancelled = Arc::new(AtomicBool::new(false));
let fetches = std::array::from_fn(|index| {
let gate = gates[index].take().unwrap();
let started = started.clone();
let cancelled = Arc::clone(&cancelled);
move |page| {
started.send((index, page)).unwrap();
let has_more = loop {
match gate.try_recv() {
Ok(result) => break result?,
Err(mpsc::TryRecvError::Empty) if !cancelled.load(Ordering::Acquire) => {
thread::yield_now()
}
Err(_) => return Err("cancelled".into()),
}
};
Ok(PullRequestPage {
kind: RepoKind::Git,
remote: "origin".into(),
entries: vec![],
has_more,
})
}
});
let mut worker = PullRequestWorker::start_with(7, outgoing, cancelled, fetches).unwrap();
let mut first = [
starts.recv_timeout(Duration::from_secs(2)).unwrap(),
starts.recv_timeout(Duration::from_secs(2)).unwrap(),
];
first.sort_unstable();
assert_eq!(first, [(0, 1), (1, 1)]);
let receive = |worker: &mut PullRequestWorker| {
let (id, result) = results.recv_timeout(Duration::from_secs(2)).unwrap();
assert_eq!(id, 7);
let Ok(DiffOutcome::PullRequestPage { closed, result }) = result else {
panic!("expected independently published page");
};
worker.complete(closed, &result);
(closed, result)
};
closed.send(Ok(true)).unwrap();
let (category, page) = receive(&mut worker);
assert!(category && page.unwrap().has_more);
// Open's first request remains blocked while the active Closed category advances.
worker.update(1, [0, 0]).unwrap();
assert_eq!(starts.recv_timeout(Duration::from_secs(2)).unwrap(), (1, 2));
closed.send(Err("late failure".into())).unwrap();
assert_eq!(receive(&mut worker).1.unwrap_err(), "late failure");
worker.update(1, [0, 0]).unwrap();
assert!(starts.try_recv().is_err());
worker.update(1, [0, 1]).unwrap();
assert_eq!(starts.recv_timeout(Duration::from_secs(2)).unwrap(), (1, 2));
closed.send(Ok(false)).unwrap();
assert!(!receive(&mut worker).1.unwrap().has_more);
open.send(Ok(false)).unwrap();
assert!(!receive(&mut worker).0);
worker.update(2, [0, 1]).unwrap();
assert!(starts.try_recv().is_err());
}
#[test]
fn closing_pull_request_listing_cancels_both_outstanding_reads() {
let (outgoing, _results) = mpsc::channel();
let (started, starts) = mpsc::channel();
let cancelled = Arc::new(AtomicBool::new(false));
let fetches = std::array::from_fn(|index| {
let started = started.clone();
let cancelled = Arc::clone(&cancelled);
move |_| {
started.send(index).unwrap();
while !cancelled.load(Ordering::Acquire) {
thread::yield_now();
}
Err("cancelled".into())
}
});
let worker =
PullRequestWorker::start_with(1, outgoing, Arc::clone(&cancelled), fetches).unwrap();
starts.recv_timeout(Duration::from_secs(2)).unwrap();
starts.recv_timeout(Duration::from_secs(2)).unwrap();
drop(worker);
assert!(cancelled.load(Ordering::Acquire));
}
#[test] #[test]
fn normal_picker_tracks_unsaved_text_and_head_with_line_gutter_disabled() { fn normal_picker_tracks_unsaved_text_and_head_with_line_gutter_disabled() {
use crate::diff::FileStatus; use crate::diff::FileStatus;
+91 -38
View File
@@ -2608,21 +2608,30 @@ fn draw_diff(
format!(" Diff · Choose {stage} · {} ", diff.comparison_mode.label()) format!(" Diff · Choose {stage} · {} ", diff.comparison_mode.label())
}; };
let content = diff_panel(frame, panel, &title, "", "", true); let content = diff_panel(frame, panel, &title, "", "", true);
let notice = diff.error.as_deref().or_else(|| { let pull_request_notice = (pull_requests && diff.phase == Phase::Before)
if diff.loading { .then(|| diff.pull_request_notice())
Some(if diff.after.is_some() { .flatten();
"Loading comparison…" let notice = diff
} else if pull_requests { .error
"Loading pull requests…" .as_deref()
.or(pull_request_notice.as_deref())
.or_else(|| {
if diff.loading {
Some(
if diff.after.is_some() || (pull_requests && diff.phase == Phase::After) {
"Loading comparison…"
} else if pull_requests {
"Loading pull requests…"
} else {
"Loading repository…"
},
)
} else { } else {
"Loading repository…" diff.catalog
}) .as_ref()
} else { .and_then(|catalog| catalog.notice.as_deref())
diff.catalog }
.as_ref() });
.and_then(|catalog| catalog.notice.as_deref())
}
});
let [ let [
stage_area, stage_area,
tabs_area, tabs_area,
@@ -2646,7 +2655,11 @@ fn draw_diff(
.as_ref() .as_ref()
.map_or("not selected", |revision| revision.label.as_str()); .map_or("not selected", |revision| revision.label.as_str());
let stage_text = if pull_requests { let stage_text = if pull_requests {
"Select a pull request to compare its branch changes".into() if diff.pull_request_partial() {
"Partial search · Select a loaded pull request".into()
} else {
"Select a pull request to compare its branch changes".into()
}
} else { } else {
format!( format!(
"{} · Ctrl+b toggle · Before: {}", "{} · Ctrl+b toggle · Before: {}",
@@ -2676,7 +2689,15 @@ fn draw_diff(
.collect(); .collect();
frame.render_widget(Paragraph::new(Line::from(tabs)), tabs_area); frame.render_widget(Paragraph::new(Line::from(tabs)), tabs_area);
} }
let query = format!("> {}", display_text(&diff.query)); let query = format!(
"> {}{}",
if pull_requests && diff.pull_request_partial() {
"[partial] "
} else {
""
},
display_text(&diff.query)
);
let width = usize::from(query_area.width); let width = usize::from(query_area.width);
let query_width = display_column(&query, usize::MAX); let query_width = display_column(&query, usize::MAX);
let offset = query_width.saturating_sub(width.saturating_sub(1)); let offset = query_width.saturating_sub(width.saturating_sub(1));
@@ -2790,7 +2811,14 @@ fn draw_diff(
)); ));
} }
} }
if lines.is_empty() && !diff.loading && diff.error.is_none() { if lines.is_empty()
&& !(if pull_requests {
diff.pull_request_partial()
} else {
diff.loading
})
&& diff.error.is_none()
{
let empty = if pull_requests { let empty = if pull_requests {
"No matching pull requests" "No matching pull requests"
} else { } else {
@@ -2805,9 +2833,14 @@ fn draw_diff(
} else { } else {
"select" "select"
}; };
let retry = if pull_requests {
" · Ctrl+r retry category"
} else {
""
};
frame.render_widget( frame.render_widget(
Paragraph::new(format!( Paragraph::new(format!(
"{navigation_hint} · Tab categories · type search · Enter {accept} · Esc close · Ctrl+p actions" "{navigation_hint} · Tab categories · type search · Enter {accept}{retry} · Esc close · Ctrl+p actions"
)) ))
.style(Style::default().fg(Color::DarkGray)), .style(Style::default().fg(Color::DarkGray)),
help, help,
@@ -6739,7 +6772,7 @@ mod tests {
#[test] #[test]
fn pull_request_picker_renders_all_rows_without_a_pinned_offset() { fn pull_request_picker_renders_all_rows_without_a_pinned_offset() {
use crate::diff::{Catalog, Category, RepoKind, Revision, RevisionTarget}; use crate::diff::{Category, PullRequestPage, RepoKind, Revision, RevisionTarget};
for (width, height) in [(100, 16), (44, 12)] { for (width, height) in [(100, 16), (44, 12)] {
let mut diff = DiffState::pull_requests("origin".into()); let mut diff = DiffState::pull_requests("origin".into());
diff.take_request(); diff.take_request();
@@ -6747,7 +6780,8 @@ mod tests {
Terminal::new(ratatui::backend::TestBackend::new(width, height)).unwrap(); Terminal::new(ratatui::backend::TestBackend::new(width, height)).unwrap();
let mut panels = WheelPanels::default(); let mut panels = WheelPanels::default();
render_diff(&mut terminal, &diff, &mut panels); render_diff(&mut terminal, &diff, &mut panels);
assert!(screen_text(&terminal).contains("Loading pull requests")); assert!(screen_text(&terminal).contains("loading"));
assert!(screen_text(&terminal).contains("[partial]"));
diff.apply_result(1, Err("offline".into())); diff.apply_result(1, Err("offline".into()));
render_diff(&mut terminal, &diff, &mut panels); render_diff(&mut terminal, &diff, &mut panels);
assert!(screen_text(&terminal).contains("offline")); assert!(screen_text(&terminal).contains("offline"));
@@ -6756,24 +6790,30 @@ mod tests {
diff.take_request(); diff.take_request();
diff.apply_result( diff.apply_result(
2, 2,
Ok(crate::diff::Outcome::Catalog(Catalog { Ok(crate::diff::Outcome::PullRequestPage {
kind: RepoKind::Git, closed: false,
entries: (0..20) result: Ok(PullRequestPage {
.map(|index| Revision { kind: RepoKind::Git,
target: RevisionTarget::PullRequest { remote: "origin".into(),
remote: "origin".into(), entries: (0..20)
base_ref: "refs/heads/main".into(), .map(|index| {
base_sha: "base".into(), (
head_ref: format!("refs/pull/{index}/head"), Revision {
head_sha: format!("head-{index}"), target: RevisionTarget::PullRequest {
}, remote: "origin".into(),
label: format!("#{index:02} Pull request"), number: index,
description: "open · main ← topic".into(), },
category: Category::Open, label: format!("#{index:02} Pull request"),
}) description: "open · main ← topic".into(),
.collect(), category: Category::Open,
notice: None, },
})), format!("2026-01-{:02}T00:00:00Z", 20 - index),
)
})
.collect(),
has_more: true,
}),
}),
); );
render_diff(&mut terminal, &diff, &mut panels); render_diff(&mut terminal, &diff, &mut panels);
let text = screen_text(&terminal); let text = screen_text(&terminal);
@@ -6781,6 +6821,7 @@ mod tests {
assert!(text.contains("Open") && text.contains("Closed") && text.contains("All")); assert!(text.contains("Open") && text.contains("Closed") && text.contains("All"));
assert!(text.contains("> #00 Pull request")); assert!(text.contains("> #00 Pull request"));
assert!(!text.contains("pinned")); assert!(!text.contains("pinned"));
assert!(text.contains("[partial]"));
diff.handle(Input::End); diff.handle(Input::End);
render_diff(&mut terminal, &diff, &mut panels); render_diff(&mut terminal, &diff, &mut panels);
let text = screen_text(&terminal); let text = screen_text(&terminal);
@@ -6790,6 +6831,18 @@ mod tests {
render_diff(&mut terminal, &diff, &mut panels); render_diff(&mut terminal, &diff, &mut panels);
assert!(screen_text(&terminal).contains("> #00 Pull request")); assert!(screen_text(&terminal).contains("> #00 Pull request"));
diff.handle(Input::Char('\t')); diff.handle(Input::Char('\t'));
diff.apply_result(
2,
Ok(crate::diff::Outcome::PullRequestPage {
closed: true,
result: Ok(PullRequestPage {
kind: RepoKind::Git,
remote: "origin".into(),
entries: vec![],
has_more: false,
}),
}),
);
render_diff(&mut terminal, &diff, &mut panels); render_diff(&mut terminal, &diff, &mut panels);
assert!(screen_text(&terminal).contains("No matching pull requests")); assert!(screen_text(&terminal).contains("No matching pull requests"));
} }