feat: sort and filter pull requests by review state

This commit is contained in:
Valdemar Lorenzen
2026-10-21 12:41:17 +02:00
parent 5c08aa0a67
commit 37b47ebc8a
7 changed files with 206 additions and 7 deletions
+3
View File
@@ -4,6 +4,9 @@
- also change the checkmark displayed for PRs approved by me to the `👍` emoji. - also change the checkmark displayed for PRs approved by me to the `👍` emoji.
- make the symbols displayed for these states configurable. - make the symbols displayed for these states configurable.
- [x] chore: check to make sure we're mocking everything in tests, not pinging/fetching real data from real websites/APIs. - [x] chore: check to make sure we're mocking everything in tests, not pinging/fetching real data from real websites/APIs.
- [x] improve: pull requests - sort and filter based on review state:
- pr approved
- review requested
- [ ] feature: diff view - GitHub code review integration after the local review UX is usable. - [ ] feature: diff view - GitHub code review integration after the local review UX is usable.
- publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes. - publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes.
- preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission. - preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission.
+2
View File
@@ -88,6 +88,8 @@ Clipboard checks use isolated providers rather than reading or overwriting the d
Diff checks cover staged category/filter navigation, stale request cancellation, row alignment, read-only buffer isolation, and rendered panes. Repository integration fixtures exercise Git and Git-backed jj, advertised but unfetched remote branches/bookmarks/tags, imported historical commit/change IDs, conflicts, unusual paths, binary/mode/link/submodule changes, and unchanged active metadata. Remote fixtures are local bare copies; they never push. Git fixture identities and commit dates are fixed. Diff checks cover staged category/filter navigation, stale request cancellation, row alignment, read-only buffer isolation, and rendered panes. Repository integration fixtures exercise Git and Git-backed jj, advertised but unfetched remote branches/bookmarks/tags, imported historical commit/change IDs, conflicts, unusual paths, binary/mode/link/submodule changes, and unchanged active metadata. Remote fixtures are local bare copies; they never push. Git fixture identities and commit dates are fixed.
Pull-request checks cover personal review filters and priority sorting alongside fuzzy queries, category changes, streamed metadata updates, identity-preserving selection, and immediate opening. Live PTY smoke at 120×30 and 44×16 cycles both controls, combines filters with search, restores defaults, and exits cleanly. A disposable curl fixture supplies all forge responses, and the editor and descendants run with networking denied.
Diff-stat checks cover unopened frozen files, additions/deletions/renames, binary/mode changes, CRLF/EOF, refresh, oversized-file laziness, filtering, live edits/undo, and inactive tabs. Live PTY smoke opens a two-file Git Working comparison, observes selected-file `+2 -1` alongside whole-diff `+2 -2` before opening the second file, edits and undoes text, switches to an ordinary tab, and exits with source bytes and HEAD unchanged. Diff-stat checks cover unopened frozen files, additions/deletions/renames, binary/mode changes, CRLF/EOF, refresh, oversized-file laziness, filtering, live edits/undo, and inactive tabs. Live PTY smoke opens a two-file Git Working comparison, observes selected-file `+2 -1` alongside whole-diff `+2 -2` before opening the second file, edits and undoes text, switches to an ordinary tab, and exits with source bytes and HEAD unchanged.
Local review checks cover native comment/summary composition and undo, Visual ranges on either side, verdicts, JSON save/reopen, external-write refusal, hidden drafts and teardown guards, immutable Working identities in Git/jj, renamed paths, live EOF, and stale anchors. Render checks cover selected/range markers, split/unified context, wrapped continuations, and narrow gutters. Live PTY smoke creates and saves a comment/summary/verdict, closes and reopens the review, edits the reopened comment, changes the reviewed source, and observes stale status without altering the saved anchor. Fixtures are local repositories; no forge writes. Local review checks cover native comment/summary composition and undo, Visual ranges on either side, verdicts, JSON save/reopen, external-write refusal, hidden drafts and teardown guards, immutable Working identities in Git/jj, renamed paths, live EOF, and stale anchors. Render checks cover selected/range markers, split/unified context, wrapped continuations, and narrow gutters. Live PTY smoke creates and saves a comment/summary/verdict, closes and reopens the review, edits the reopened comment, changes the reviewed source, and observes stale status without altering the saved anchor. Fixtures are local repositories; no forge writes.
+2
View File
@@ -412,6 +412,8 @@ Immediately before `#number`, **🙏 marks an outstanding personal review reques
A later request for changes or dismissal removes 👍; comment-only and pending reviews do not replace an existing approval. The mark is not the PR's overall approval decision or a guarantee that you reviewed its current head; the forge controls approval resets after new commits. Customize either symbol with [`config.pull_request_review_requested_symbol` and `config.pull_request_approved_symbol`](configuration.md); an empty string hides that symbol. Multi-cell symbols remain aligned, and decorations are not part of searchable PR labels. A later request for changes or dismissal removes 👍; comment-only and pending reviews do not replace an existing approval. The mark is not the PR's overall approval decision or a guarantee that you reviewed its current head; the forge controls approval resets after new commits. Customize either symbol with [`config.pull_request_review_requested_symbol` and `config.pull_request_approved_symbol`](configuration.md); an empty string hides that symbol. Multi-cell symbols remain aligned, and decorations are not part of searchable PR labels.
**Ctrl+f** cycles the review filter through **All → Requested → Approved**. **Ctrl+s** cycles sorting through **Newest → Requested first → Approved first**. Both use the personal review states above, even when their symbols are hidden; a re-review request belongs to Requested, not Approved. Filters combine with the selected category and fuzzy query. Review priority sorts ahead of fuzzy relevance; within each group, fuzzy ranking is unchanged, or newest creation time first when the query is empty (higher PR number breaks date ties). Sorting and filtering preserve the selected PR when it remains visible, including as pages arrive or metadata changes. Category counts still report all loaded requests, and partial-search notices still apply. These controls only use already-loaded metadata and make no additional API requests.
Review-state reads reuse the same API credentials as listing. GitHub matches the current list's [requested reviewers](https://docs.github.com/en/rest/pulls/review-requests#list-requested-reviewers-for-a-pull-request) and filters published [reviews by author and decision state](https://docs.github.com/en/graphql/reference/pulls#pullrequest). GitLab filters approvals by the authenticated approver and reads individual reviewer [interaction states](https://docs.gitlab.com/api/graphql/reference/) in separate bounded, paginated batches; unsupported or denied reviewer-state fields do not discard known approvals or size metadata. Gitea reuses [paginated review reads per PR](https://docs.gitea.com/api/operations/repo-list-pull-reviews/) for both states, so its pages can take longer to load. Review-state reads reuse the same API credentials as listing. GitHub matches the current list's [requested reviewers](https://docs.github.com/en/rest/pulls/review-requests#list-requested-reviewers-for-a-pull-request) and filters published [reviews by author and decision state](https://docs.github.com/en/graphql/reference/pulls#pullrequest). GitLab filters approvals by the authenticated approver and reads individual reviewer [interaction states](https://docs.gitlab.com/api/graphql/reference/) in separate bounded, paginated batches; unsupported or denied reviewer-state fields do not discard known approvals or size metadata. Gitea reuses [paginated review reads per PR](https://docs.gitea.com/api/operations/repo-list-pull-reviews/) for both states, so its pages can take longer to load.
Anonymous, denied, missing, or failed state lookups leave the corresponding indicator unavailable without preventing browsing. A blank is not proof that no request or approval exists. Reopen the picker to fetch current review state. No approval, review request, or other remote write is performed. Anonymous, denied, missing, or failed state lookups leave the corresponding indicator unavailable without preventing browsing. A blank is not proof that no request or approval exists. Reopen the picker to fetch current review state. No approval, review request, or other remote write is performed.
+76 -1
View File
@@ -109,6 +109,40 @@ pub enum PullRequestReviewState {
Requested, Requested,
} }
#[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)]
pub(crate) enum PullRequestReviewScope {
#[default]
All,
Requested,
Approved,
}
impl PullRequestReviewScope {
pub(crate) fn label(self) -> &'static str {
match self {
Self::All => "All",
Self::Requested => "Requested",
Self::Approved => "Approved",
}
}
fn next(self) -> Self {
match self {
Self::All => Self::Requested,
Self::Requested => Self::Approved,
Self::Approved => Self::All,
}
}
fn matches(self, state: PullRequestReviewState) -> bool {
match self {
Self::All => true,
Self::Requested => state == PullRequestReviewState::Requested,
Self::Approved => state == PullRequestReviewState::Approved,
}
}
}
#[derive(Debug)] #[derive(Debug)]
pub struct PullRequestPage { pub struct PullRequestPage {
pub kind: RepoKind, pub kind: RepoKind,
@@ -582,6 +616,8 @@ pub struct DiffState {
pull_request_categories: [PullRequestCategory; 2], pull_request_categories: [PullRequestCategory; 2],
pull_request_order: BTreeMap<u64, (String, usize, PullRequestReviewState)>, pull_request_order: BTreeMap<u64, (String, usize, PullRequestReviewState)>,
pub(crate) pull_request_retries: [u64; 2], pub(crate) pull_request_retries: [u64; 2],
pub(crate) pull_request_filter: PullRequestReviewScope,
pub(crate) pull_request_priority: PullRequestReviewScope,
pub phase: Phase, pub phase: Phase,
pub word_wrap: bool, pub word_wrap: bool,
pub view_mode: ViewMode, pub view_mode: ViewMode,
@@ -646,6 +682,8 @@ impl DiffState {
pull_request_categories: Default::default(), pull_request_categories: Default::default(),
pull_request_order: BTreeMap::new(), pull_request_order: BTreeMap::new(),
pull_request_retries: [0; 2], pull_request_retries: [0; 2],
pull_request_filter: PullRequestReviewScope::All,
pull_request_priority: PullRequestReviewScope::All,
phase: Phase::Before, phase: Phase::Before,
word_wrap: false, word_wrap: false,
view_mode: ViewMode::Split, view_mode: ViewMode::Split,
@@ -1324,6 +1362,10 @@ impl DiffState {
entry.category, entry.category,
Category::Open | Category::Closed | Category::Drafts Category::Open | Category::Closed | Category::Drafts
))) )))
|| (self.pull_request_remote.is_some()
&& !self
.pull_request_filter
.matches(self.pull_request_review_state(entry)))
{ {
continue; continue;
} }
@@ -1359,7 +1401,24 @@ impl DiffState {
self.search_matches.push((score, index)); self.search_matches.push((score, index));
} }
} }
self.search_matches.sort_unstable(); let priority = |index: usize| {
if self.pull_request_priority == PullRequestReviewScope::All {
return false;
}
let RevisionTarget::PullRequest { number, .. } = catalog.entries[index].target
else {
return false;
};
!self
.pull_request_priority
.matches(self.pull_request_order[&number].2)
};
if self.pull_request_priority != PullRequestReviewScope::All {
self.visible
.sort_unstable_by_key(|&index| (priority(index), index));
}
self.search_matches
.sort_unstable_by_key(|&(score, index)| (priority(index), score, index));
self.visible self.visible
.extend(self.search_matches.iter().map(|(_, index)| *index)); .extend(self.search_matches.iter().map(|(_, index)| *index));
} }
@@ -1447,6 +1506,22 @@ impl DiffState {
self.choose() self.choose()
} }
Input::Ctrl('r') if self.pull_request_remote.is_some() => self.retry_pull_requests(), Input::Ctrl('r') if self.pull_request_remote.is_some() => self.retry_pull_requests(),
Input::Ctrl('f' | 's') if self.pull_request_remote.is_some() => {
let selected = self
.entries()
.nth(self.selected)
.map(|entry| entry.target.clone());
let scope = if input == Input::Ctrl('f') {
&mut self.pull_request_filter
} else {
&mut self.pull_request_priority
};
*scope = scope.next();
self.refilter();
if let Some(target) = selected {
self.select_target(&target);
}
}
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")
+1
View File
@@ -108,6 +108,7 @@ Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the
Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/Drafts/All tabs, type to search Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/Drafts/All tabs, type to search
Enter opens the immutable PR head against its merge base; API auth uses provider tokens or gh/glab Enter opens the immutable PR head against its merge base; API auth uses provider tokens or gh/glab
🙏 marks a personal review request; 👍 marks your active approval (API account) 🙏 marks a personal review request; 👍 marks your active approval (API account)
Ctrl+f filters All/Requested/Approved; Ctrl+s sorts Newest/Requested first/Approved first
Symbols: config.pull_request_review_requested_symbol / config.pull_request_approved_symbol Symbols: config.pull_request_review_requested_symbol / config.pull_request_approved_symbol
Resolve jj conflicts: :resolve opens the current working-copy conflicts in a dedicated workspace Resolve jj conflicts: :resolve opens the current working-copy conflicts in a dedicated workspace
Editable marker-free Draft vs readonly Alternative; :resolve.history shows inputs/base Editable marker-free Draft vs readonly Alternative; :resolve.history shows inputs/base
+17 -6
View File
@@ -3397,6 +3397,7 @@ fn draw_diff(
.and_then(|catalog| catalog.notice.as_deref()) .and_then(|catalog| catalog.notice.as_deref())
} }
}); });
let stacked_review_controls = pull_requests && content.width < 68 && content.height >= 8;
let [ let [
stage_area, stage_area,
tabs_area, tabs_area,
@@ -3406,7 +3407,11 @@ fn draw_diff(
results, results,
help, help,
] = Layout::vertical([ ] = Layout::vertical([
Constraint::Length(u16::from(content.height >= 5)), Constraint::Length(if stacked_review_controls {
2
} else {
u16::from(content.height >= 5)
}),
Constraint::Length(u16::from(content.height >= 4)), Constraint::Length(u16::from(content.height >= 4)),
Constraint::Length(1), Constraint::Length(1),
Constraint::Length(u16::from(notice.is_some() && content.height >= 3)), Constraint::Length(u16::from(notice.is_some() && content.height >= 3)),
@@ -3420,11 +3425,15 @@ 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 {
if diff.pull_request_partial() { let sort = match diff.pull_request_priority {
"Partial search · Select a loaded pull request".into() crate::diff::PullRequestReviewScope::All => "Newest".into(),
} else { scope => format!("{} first", scope.label()),
"Select a pull request to compare its branch changes".into() };
} format!(
"Review: {} · Ctrl+f{}Sort: {sort} · Ctrl+s",
diff.pull_request_filter.label(),
if stacked_review_controls { "\n" } else { " " },
)
} else { } else {
format!( format!(
"{} · Ctrl+b toggle · Before: {}", "{} · Ctrl+b toggle · Before: {}",
@@ -3522,6 +3531,8 @@ fn draw_diff(
diff.category, diff.category,
&diff.query, &diff.query,
diff.comparison_mode as u8, diff.comparison_mode as u8,
diff.pull_request_filter,
diff.pull_request_priority,
diff.catalog diff.catalog
.as_ref() .as_ref()
.map(|catalog| (catalog.entries.as_ptr(), catalog.entries.len())), .map(|catalog| (catalog.entries.as_ptr(), catalog.entries.len())),
+105
View File
@@ -486,6 +486,111 @@ fn pull_request_review_refresh_preserves_identity_sorting_and_raw_search_labels(
); );
} }
#[test]
fn pull_request_review_controls_compose_with_search_categories_and_streamed_updates() {
use PullRequestReviewState::{Approved, None, Requested};
let mut editor = editor();
editor.execute("pull_requests origin");
let (id, _) = editor.take_diff_request().unwrap();
let mut page = pull_page(
false,
&[
(1, "Fix", "1"),
(2, "Fix", "2"),
(3, "Fix", "3"),
(4, "Fix", "4"),
(5, "Fix", "4"),
(6, "prefix Fix", "6"),
(7, "Draft Fix", "7"),
],
true,
);
if let Outcome::PullRequestPage {
result: Ok(page), ..
} = &mut page
{
for (entry, state) in page.entries.iter_mut().zip([
Requested, Approved, None, Requested, Approved, None, Requested,
]) {
entry.2 = state;
}
page.entries[6].0.category = Category::Drafts;
}
editor.apply_diff_result(id, Ok(page));
let numbers = |editor: &Editor| {
editor
.diff()
.unwrap()
.entries()
.map(|entry| {
let RevisionTarget::PullRequest { number, .. } = entry.target else {
panic!("expected pull request");
};
number
})
.collect::<Vec<_>>()
};
let selected = |editor: &Editor| {
let diff = editor.diff().unwrap();
diff.entries().nth(diff.selected).unwrap().target.clone()
};
assert_eq!(numbers(&editor), [6, 5, 4, 3, 2, 1]);
editor.handle(Input::End);
let target = selected(&editor);
editor.handle(Input::Ctrl('s'));
assert_eq!(numbers(&editor), [4, 1, 6, 5, 3, 2]);
assert_eq!(selected(&editor), target);
editor.handle(Input::Ctrl('s'));
assert_eq!(numbers(&editor), [5, 2, 6, 4, 3, 1]);
assert_eq!(selected(&editor), target);
editor.handle(Input::Ctrl('s'));
assert_eq!(numbers(&editor), [6, 5, 4, 3, 2, 1]);
for ch in "Fix".chars() {
editor.handle(Input::Char(ch));
}
assert_eq!(numbers(&editor), [5, 4, 3, 2, 1, 6]);
editor.handle(Input::Ctrl('s'));
assert_eq!(numbers(&editor), [4, 1, 5, 3, 2, 6]);
editor.handle(Input::Ctrl('f'));
assert_eq!(numbers(&editor), [4, 1]);
editor.handle(Input::Ctrl('f'));
assert_eq!(numbers(&editor), [5, 2]);
editor.handle(Input::Ctrl('f'));
assert_eq!(numbers(&editor), [4, 1, 5, 3, 2, 6]);
editor.handle(Input::Ctrl('f'));
editor.handle(Input::Home);
let mut update = pull_page(false, &[(4, "Fix", "4")], true);
if let Outcome::PullRequestPage {
result: Ok(page), ..
} = &mut update
{
page.entries[0].2 = Approved;
}
editor.apply_diff_result(id, Ok(update));
assert_eq!(numbers(&editor), [1]);
editor.handle(Input::Char('\t'));
assert!(numbers(&editor).is_empty());
editor.handle(Input::Char('\t'));
assert_eq!(numbers(&editor), [7]);
editor.handle(Input::Char('\t'));
assert_eq!(numbers(&editor), [1, 7]);
assert_eq!(editor.diff().unwrap().query, "Fix");
assert!(editor.diff().unwrap().pull_request_partial());
editor.handle(Input::Ctrl('u'));
assert_eq!(numbers(&editor), [7, 1]);
editor.handle(Input::Enter);
assert!(matches!(
editor.take_diff_request(),
Some((
_,
Operation::ResolvePullRequest(Revision {
target: RevisionTarget::PullRequest { number: 7, .. },
..
})
))
));
}
#[test] #[test]
fn pull_request_categories_keep_query_and_select_loaded_rows_before_completion() { 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());