diff --git a/TODO.md b/TODO.md index 5e23333..2f9f335 100644 --- a/TODO.md +++ b/TODO.md @@ -4,6 +4,9 @@ - also change the checkmark displayed for PRs approved by me to the `๐Ÿ‘` emoji. - 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] 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. - 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. diff --git a/docs/development.md b/docs/development.md index 4347d6d..1589f23 100644 --- a/docs/development.md +++ b/docs/development.md @@ -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. +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. 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. diff --git a/docs/editing.md b/docs/editing.md index 2620588..7594b68 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -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. +**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. 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. diff --git a/src/diff.rs b/src/diff.rs index 686ed0f..10615fc 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -109,6 +109,40 @@ pub enum PullRequestReviewState { 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)] pub struct PullRequestPage { pub kind: RepoKind, @@ -582,6 +616,8 @@ pub struct DiffState { pull_request_categories: [PullRequestCategory; 2], pull_request_order: BTreeMap, pub(crate) pull_request_retries: [u64; 2], + pub(crate) pull_request_filter: PullRequestReviewScope, + pub(crate) pull_request_priority: PullRequestReviewScope, pub phase: Phase, pub word_wrap: bool, pub view_mode: ViewMode, @@ -646,6 +682,8 @@ impl DiffState { pull_request_categories: Default::default(), pull_request_order: BTreeMap::new(), pull_request_retries: [0; 2], + pull_request_filter: PullRequestReviewScope::All, + pull_request_priority: PullRequestReviewScope::All, phase: Phase::Before, word_wrap: false, view_mode: ViewMode::Split, @@ -1324,6 +1362,10 @@ impl DiffState { entry.category, Category::Open | Category::Closed | Category::Drafts ))) + || (self.pull_request_remote.is_some() + && !self + .pull_request_filter + .matches(self.pull_request_review_state(entry))) { continue; } @@ -1359,7 +1401,24 @@ impl DiffState { 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 .extend(self.search_matches.iter().map(|(_, index)| *index)); } @@ -1447,6 +1506,22 @@ impl DiffState { self.choose() } 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() => { if BackspaceKind::from_input(&input) .expect("guard checked backspace input") diff --git a/src/main.rs b/src/main.rs index 8efe64e..fef5d52 100644 --- a/src/main.rs +++ b/src/main.rs @@ -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 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) + 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 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 diff --git a/src/tui.rs b/src/tui.rs index 2349d6a..bab37d4 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -3397,6 +3397,7 @@ fn draw_diff( .and_then(|catalog| catalog.notice.as_deref()) } }); + let stacked_review_controls = pull_requests && content.width < 68 && content.height >= 8; let [ stage_area, tabs_area, @@ -3406,7 +3407,11 @@ fn draw_diff( results, help, ] = 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(1), Constraint::Length(u16::from(notice.is_some() && content.height >= 3)), @@ -3420,11 +3425,15 @@ fn draw_diff( .as_ref() .map_or("not selected", |revision| revision.label.as_str()); let stage_text = if pull_requests { - if diff.pull_request_partial() { - "Partial search ยท Select a loaded pull request".into() - } else { - "Select a pull request to compare its branch changes".into() - } + let sort = match diff.pull_request_priority { + crate::diff::PullRequestReviewScope::All => "Newest".into(), + scope => format!("{} first", scope.label()), + }; + format!( + "Review: {} ยท Ctrl+f{}Sort: {sort} ยท Ctrl+s", + diff.pull_request_filter.label(), + if stacked_review_controls { "\n" } else { " " }, + ) } else { format!( "{} ยท Ctrl+b toggle ยท Before: {}", @@ -3522,6 +3531,8 @@ fn draw_diff( diff.category, &diff.query, diff.comparison_mode as u8, + diff.pull_request_filter, + diff.pull_request_priority, diff.catalog .as_ref() .map(|catalog| (catalog.entries.as_ptr(), catalog.entries.len())), diff --git a/tests/unit/diff.rs b/tests/unit/diff.rs index 8c7e7b1..19f406b 100644 --- a/tests/unit/diff.rs +++ b/tests/unit/diff.rs @@ -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::>() + }; + 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] fn pull_request_categories_keep_query_and_select_loaded_rows_before_completion() { let mut state = DiffState::pull_requests("origin".into());