From 0202644efa6e938252449a2d3f6caf7b4ea7187a Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Wed, 21 Oct 2026 11:14:50 +0200 Subject: [PATCH] feat: configure personal pull request review indicators --- TODO.md | 2 +- docs/configuration.md | 4 + docs/editing.md | 8 +- examples/config.rb | 2 + src/diff.rs | 30 +++- src/editor.rs | 4 + src/forge.rs | 254 ++++++++++++++++++++++++++-------- src/main.rs | 3 +- src/repository.rs | 16 ++- src/ruby.rs | 24 ++++ src/ruby_config.rb | 23 ++++ src/session.rs | 2 + src/tui.rs | 34 ++++- tests/unit/diff.rs | 46 +++++++ tests/unit/forge.rs | 310 +++++++++++++++++++++++++++++++++++++++++- tests/unit/ruby.rs | 87 +++++++++++- tests/unit/session.rs | 17 +++ tests/unit/tui.rs | 123 ++++++++++++++++- 18 files changed, 901 insertions(+), 88 deletions(-) diff --git a/TODO.md b/TODO.md index 1b5af91..7ecd5a1 100644 --- a/TODO.md +++ b/TODO.md @@ -1,6 +1,6 @@ - [x] improve: pull requests - add a tab for drafts so PRs in this state don't populate the "open" tab. - [x] improve: pull requests - add a checkmark left of the PR entry in the PR list if the user in question (me) has already approved it. -- [ ] improve: pull requests - add a `๐Ÿ™` emoji left of the PR entry in the PR list if the user in question (me) has been requested as reviewer. +- [x] improve: pull requests - add a `๐Ÿ™` emoji left of the PR entry in the PR list if the user in question (me) has been requested as reviewer. - also change the checkmark displayed for PRs approved by me to the `๐Ÿ‘` emoji. - make the symbols displayed for these states configurable. - [ ] chore: check to make sure we're mocking everything in tests, not pinging/fetching real data from real websites/APIs. diff --git a/docs/configuration.md b/docs/configuration.md index 1d6818b..a043e6d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -30,6 +30,8 @@ Vamp.configure do |config| config.diff_view_mode = "split" # or "unified" config.diff_split_orientation = "vertical" # or "horizontal" config.diff_follow_working_copy = true + config.pull_request_review_requested_symbol = "๐Ÿ™" + config.pull_request_approved_symbol = "๐Ÿ‘" config.resolve_history_layout = "columns" # or "base_below" config.review_directory = nil # XDG data directory; override with an absolute or config-relative path config.bind "ctrl+h", config.core.left, mode: config.modes.tree @@ -105,6 +107,8 @@ Indicators compare the current editor text (including unsaved edits) against Git `config.diff_follow_working_copy` defaults to `true`. A Working-checkout After follows descendant checkouts automatically, including in inactive tabs; `false` disables endpoint following without disabling ordinary live-buffer editing or reload. Only literal booleans are accepted. Pinned After endpoints never move, but their checkout-aware editability checks remain enabled regardless of this setting. See [repository diff](editing.md#repository-diff) for ancestry and refresh behavior. +`config.pull_request_review_requested_symbol` and `config.pull_request_approved_symbol` control the markers before PR numbers in the pull-request picker. Defaults are `"๐Ÿ™"` for an outstanding personal review request and `"๐Ÿ‘"` for your active approval, using the API-authenticated account. Requests take precedence over approvals when both apply. Each option accepts a string without control characters; multi-character and wide Unicode symbols are supported. Set either to `""` to hide that state, or both to `""` to remove the marker gutter. PR numbers and metadata stay aligned by terminal-cell width, including rows without a marker. Non-strings and strings containing tabs, newlines, or other control characters reject the configuration. + `config.resolve_history_layout` controls the read-only conflict inputs opened by `:resolve.history`: `"columns"` (default) places left/base/right side by side; `"base_below"` places left/right above a full-width base. Both layouts share aligned rows and synchronized vertical scrolling. Tab/Shift+Tab follows layout order (left/base/right or left/right/base); directional pane actions follow the displayed geometry. Narrow terminals show the focused source only; the base-below layout does the same when there is insufficient height. This setting does not change the editable Resolution view or ordinary diffs. Invalid values reject the configuration. ```ruby diff --git a/docs/editing.md b/docs/editing.md index 6cfea33..2620588 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -408,9 +408,13 @@ The selector initially lists local revisions and known tracking refs **without c Each pull request's metadata shows added/deleted line counts after its state, before the branch names: `author ยท open ยท +123 -45 ยท topic โ†’ main`. Zero counts are shown explicitly; missing, denied, or failed statistics show `size unavailable` without preventing selection. Counts come from one batched read-only statistics query per nonempty GitHub/GitLab page ([GitHub](https://docs.github.com/en/graphql/reference/pulls), [GitLab](https://docs.gitlab.com/api/graphql/reference/#diffstatssummary)); [Gitea](https://docs.gitea.com/api/operations/repo-list-pull-requests/) supplies them directly in its list response. GitHub statistics require API authentication even for public repositories. Listing does not fetch source diffs. -A **โœ“ immediately before `#number`** marks your active approval, separately from the `>` selection arrow. โ€œYouโ€ means the account authenticated to that forge's APIโ€”not the repository's Git author or SSH identity. Other users' approvals do not count. A later request for changes or dismissal removes the mark; 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. +Immediately before `#number`, **๐Ÿ™ marks an outstanding personal review request** and **๐Ÿ‘ marks your active approval**, separately from the `>` selection arrow. A review request takes precedence when both apply, including a re-review request after an earlier approval. โ€œYouโ€ means the account authenticated to that forge's APIโ€”not the repository's Git author or SSH identity. Other users' and team-only requests do not count; completed or removed requests lose ๐Ÿ™. -Approval reads reuse the same API credentials as listing. GitHub filters published [reviews by author and decision state](https://docs.github.com/en/graphql/reference/pulls#pullrequest); GitLab filters merge requests by the [authenticated approver](https://docs.gitlab.com/api/graphql/reference/). Both batch approval data with size metadata after resolving the authenticated account. Gitea needs [paginated review reads per PR](https://docs.gitea.com/api/operations/repo-list-pull-reviews/), so its pages can take longer to load. Anonymous, denied, missing, or failed approval lookups leave the mark blank without preventing browsing; a blank is not proof that you have not approved. Reopen the picker to fetch current approval state. No approval or other remote write is performed. +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. + +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. GitHub, GitLab merge requests, and Gitea are supported. API reads require `curl`; public repositories work anonymously. Authentication uses `GH_TOKEN`/`GITHUB_TOKEN` for github.com, `GH_ENTERPRISE_TOKEN`/`GITHUB_ENTERPRISE_TOKEN` for GitHub Enterprise, `GITLAB_TOKEN` for GitLab, and `GITEA_TOKEN` for Gitea. With no matching environment token, GitHub/GitLab reuse `gh auth token --hostname HOST` / `glab auth token --hostname HOST` when available. Tokens are supplied to curl on stdin, not in subprocess arguments, and sent only to the HTTPS API; redirects are not followed. Git object fetching uses the configured remote's existing Git/SSH credentials separately. diff --git a/examples/config.rb b/examples/config.rb index 85a9d7d..99e1f18 100644 --- a/examples/config.rb +++ b/examples/config.rb @@ -18,6 +18,8 @@ Vamp.configure do |config| config.diff_view_mode = "split" # or "unified" config.diff_split_orientation = "vertical" # or "horizontal" config.diff_follow_working_copy = true # false keeps the selected Working After endpoint + config.pull_request_review_requested_symbol = "๐Ÿ™" + config.pull_request_approved_symbol = "๐Ÿ‘" config.resolve_history_layout = "columns" # or "base_below" for left/right above the base # config.popup.command = { anchor: :bottom_left } # config.popup.search = { anchor: :center } diff --git a/src/diff.rs b/src/diff.rs index 04c68f5..686ed0f 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -101,11 +101,19 @@ pub struct Catalog { pub entries: Vec, pub notice: Option, } +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub enum PullRequestReviewState { + #[default] + None, + Approved, + Requested, +} + #[derive(Debug)] pub struct PullRequestPage { pub kind: RepoKind, pub remote: String, - pub entries: Vec<(Revision, String)>, + pub entries: Vec<(Revision, String, PullRequestReviewState)>, pub has_more: bool, } @@ -572,7 +580,7 @@ pub struct DiffState { pub catalog: Option, pub pull_request_remote: Option, pull_request_categories: [PullRequestCategory; 2], - pull_request_order: BTreeMap, + pull_request_order: BTreeMap, pub(crate) pull_request_retries: [u64; 2], pub phase: Phase, pub word_wrap: bool, @@ -934,16 +942,18 @@ impl DiffState { entries: Vec::new(), notice: Some(format!("Pull requests from {}", page.remote)), }); - for (revision, created_at) in page.entries { + for (revision, created_at, review_state) in page.entries { let RevisionTarget::PullRequest { number, .. } = &revision.target else { continue; }; - if let Some((date, index)) = self.pull_request_order.get_mut(number) { + if let Some((date, index, state)) = self.pull_request_order.get_mut(number) + { *date = created_at; + *state = review_state; catalog.entries[*index] = revision; } else { self.pull_request_order - .insert(*number, (created_at, catalog.entries.len())); + .insert(*number, (created_at, catalog.entries.len(), review_state)); catalog.entries.push(revision); } } @@ -1222,6 +1232,16 @@ impl DiffState { } } + pub(crate) fn pull_request_review_state(&self, revision: &Revision) -> PullRequestReviewState { + if let RevisionTarget::PullRequest { number, .. } = revision.target { + self.pull_request_order + .get(&number) + .map_or(PullRequestReviewState::None, |entry| entry.2) + } else { + PullRequestReviewState::None + } + } + pub(crate) fn pull_request_partial(&self) -> bool { self.pull_request_remote.is_some() && self diff --git a/src/editor.rs b/src/editor.rs index 3cdc077..b4e13a8 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -2284,6 +2284,8 @@ pub struct Editor { diff_return_buffer: Option, diff_context_options: (usize, usize), diff_display_options: (ViewMode, SplitAxis), + pub(crate) pull_request_review_requested_symbol: String, + pub(crate) pull_request_approved_symbol: String, next_diff_request: u64, outstanding_diff_request: Option, pending_lsp_navigation: Option, @@ -2424,6 +2426,8 @@ impl Editor { diff_return_buffer: None, diff_context_options: (10, 15), diff_display_options: (ViewMode::Split, SplitAxis::Vertical), + pull_request_review_requested_symbol: "๐Ÿ™".into(), + pull_request_approved_symbol: "๐Ÿ‘".into(), next_diff_request: 0, outstanding_diff_request: None, pending_lsp_navigation: None, diff --git a/src/forge.rs b/src/forge.rs index 6bceb08..4e72027 100644 --- a/src/forge.rs +++ b/src/forge.rs @@ -15,12 +15,14 @@ pub(crate) struct PullRequestSummary { pub closed: bool, pub draft: bool, pub approved: bool, + pub review_requested: bool, pub created_at: String, } struct PullRequestMetadata { size: Option<(u64, u64)>, approved: bool, + review_requested: bool, } pub(crate) struct PullRequestPage { @@ -432,6 +434,7 @@ impl Forge { let details = metadata.get(&id); let mut entry = summary(self.provider, value, details.and_then(|d| d.size))?; entry.approved = details.is_some_and(|d| d.approved); + entry.review_requested = details.is_some_and(|d| d.review_requested); if entry.closed != closed { return Err("Forge API returned a pull request in the wrong category".into()); } @@ -478,28 +481,63 @@ impl Forge { .and_then(Value::as_str) .filter(|name| !name.is_empty()) }); - if self.provider == Provider::Gitea { - let viewer = viewer.as_ref().and_then(|viewer| number(viewer, "id").ok()); - return values - .iter() - .filter_map(|value| { - let id = number(value, "number").ok()?; - Some(( - id, - PullRequestMetadata { - size: value - .get("additions") - .and_then(Value::as_u64) - .zip(value.get("deletions").and_then(Value::as_u64)), - // ponytail: Gitea has no batch reviews API; per-PR reads until it does. - approved: viewer.is_some_and(|viewer| { - self.gitea_approved_with(id, viewer, request) - .unwrap_or(false) + let mut metadata: BTreeMap<_, _> = values + .iter() + .filter_map(|value| { + let id = number( + value, + if self.provider == Provider::Gitlab { + "iid" + } else { + "number" + }, + ) + .ok()?; + Some(( + id, + PullRequestMetadata { + size: value + .get("additions") + .and_then(Value::as_u64) + .zip(value.get("deletions").and_then(Value::as_u64)), + approved: false, + // GitHub's list contains only outstanding direct requests, not teams. + review_requested: self.provider == Provider::Github + && username.is_some_and(|username| { + value + .get("requested_reviewers") + .and_then(Value::as_array) + .is_some_and(|reviewers| { + reviewers.iter().any(|reviewer| { + reviewer + .get("login") + .and_then(Value::as_str) + .is_some_and(|login| { + login.eq_ignore_ascii_case(username) + }) + }) + }) }), - }, - )) - }) - .collect(); + }, + )) + }) + .collect(); + if self.provider == Provider::Gitea { + if let Some(viewer) = viewer.as_ref().and_then(|viewer| number(viewer, "id").ok()) { + for (&id, details) in &mut metadata { + // ponytail: Gitea has no batch reviews API; per-PR reads until it does. + // List requested_reviewers also includes completed reviews. + (details.approved, details.review_requested) = self + .gitea_reviews_with(id, viewer, request) + .unwrap_or_default(); + } + } + return metadata; + } + if self.provider == Provider::Gitlab { + if let Some(username) = username { + self.gitlab_requests_with(values, username, &mut metadata, request); + } } let reviews = if self.provider == Provider::Github { username.map_or_else(String::new, |username| { @@ -583,9 +621,9 @@ impl Forge { ) }; let body = serde_json::json!({ "query": query }).to_string(); - // Missing/denied metadata must not prevent browsing or imply approval. + // Missing/denied metadata must not discard independently known list data. let Ok(response) = request(&url, Some(&body)) else { - return BTreeMap::new(); + return metadata; }; let container = if self.provider == Provider::Github { "/data/repository" @@ -593,42 +631,44 @@ impl Forge { "/data/project" }; let Some(entries) = response.pointer(container).and_then(Value::as_object) else { - return BTreeMap::new(); + return metadata; }; - let mut metadata: BTreeMap<_, _> = entries - .iter() - .filter_map(|(alias, value)| { - let stats = if self.provider == Provider::Gitlab { - value.get("diffStatsSummary") - } else { - Some(value) - }; - let size = stats.and_then(|stats| { + for (alias, value) in entries { + let Some(details) = alias + .strip_prefix('p') + .and_then(|id| id.parse::().ok()) + .and_then(|id| metadata.get_mut(&id)) + else { + continue; + }; + let stats = if self.provider == Provider::Gitlab { + value.get("diffStatsSummary") + } else { + Some(value) + }; + details.size = stats + .and_then(|stats| { stats .get("additions")? .as_u64() .zip(stats.get("deletions")?.as_u64()) + }) + .or(details.size); + details.approved = self.provider == Provider::Github + && username.is_some_and(|username| { + value + .pointer("/reviews/nodes") + .and_then(Value::as_array) + .is_some_and(|reviews| { + reviews.len() == 1 + && reviews[0]["state"] == "APPROVED" + && reviews[0] + .pointer("/author/login") + .and_then(Value::as_str) + .is_some_and(|author| author.eq_ignore_ascii_case(username)) + }) }); - let approved = self.provider == Provider::Github - && username.is_some_and(|username| { - value - .pointer("/reviews/nodes") - .and_then(Value::as_array) - .is_some_and(|reviews| { - reviews.len() == 1 - && reviews[0]["state"] == "APPROVED" - && reviews[0] - .pointer("/author/login") - .and_then(Value::as_str) - .is_some_and(|author| author.eq_ignore_ascii_case(username)) - }) - }); - Some(( - alias.strip_prefix('p')?.parse::().ok()?, - PullRequestMetadata { size, approved }, - )) - }) - .collect(); + } if let Some(args) = approval_args { let mut response = response; let mut cursor = None::; @@ -675,13 +715,90 @@ impl Forge { metadata } - fn gitea_approved_with( + fn gitlab_requests_with( + &self, + values: &[Value], + username: &str, + metadata: &mut BTreeMap, + request: &mut impl FnMut(&str, Option<&str>) -> Result, + ) { + let mut pending: BTreeMap<_, _> = values + .iter() + .filter_map(|value| { + let reviewers = value.get("reviewers")?.as_array()?; + if !reviewers.iter().any(|reviewer| { + reviewer.get("username").and_then(Value::as_str) == Some(username) + }) { + return None; + } + Some((number(value, "iid").ok()?, None::)) + }) + .collect(); + // Keep optional reviewer-state reads separate: unsupported/denied fields + // must not invalidate the independent statistics and approval query. + while !pending.is_empty() { + use std::fmt::Write; + let mut fields = String::new(); + // Ten aliases stay below GitLab's default query-complexity limit. + let batch: BTreeMap<_, _> = (0..10).filter_map(|_| pending.pop_first()).collect(); + for (id, cursor) in &batch { + let after = cursor.as_ref().map_or_else(String::new, |cursor| { + format!(",after:{}", serde_json::to_string(cursor).unwrap()) + }); + write!(fields, "p{id}:mergeRequest(iid:\"{id}\"){{reviewers(first:20{after}){{nodes{{username mergeRequestInteraction{{reviewState}}}} pageInfo{{hasNextPage endCursor}}}}}}").unwrap(); + } + let body = serde_json::json!({ + "query": format!("query{{project(fullPath:{}){{{fields}}}}}", + serde_json::to_string(&self.repository).unwrap()) + }) + .to_string(); + let Ok(response) = request(&format!("https://{}/api/graphql", self.host), Some(&body)) + else { + continue; + }; + for (id, cursor) in batch { + let Some(reviewers) = response.pointer(&format!("/data/project/p{id}/reviewers")) + else { + continue; + }; + let own = reviewers + .get("nodes") + .and_then(Value::as_array) + .and_then(|nodes| { + nodes.iter().find(|node| { + node.get("username").and_then(Value::as_str) == Some(username) + }) + }); + if let Some(own) = own { + if let Some(details) = metadata.get_mut(&id) { + details.review_requested = matches!( + own.pointer("/mergeRequestInteraction/reviewState") + .and_then(Value::as_str), + Some("UNREVIEWED" | "REVIEW_STARTED") + ); + } + } else if reviewers.pointer("/pageInfo/hasNextPage") == Some(&Value::Bool(true)) { + if let Some(after) = reviewers + .pointer("/pageInfo/endCursor") + .and_then(Value::as_str) + .filter(|after| !after.is_empty() && Some(*after) != cursor.as_deref()) + { + pending.insert(id, Some(after.to_owned())); + } + } + } + } + } + + fn gitea_reviews_with( &self, id: u64, viewer: u64, request: &mut impl FnMut(&str, Option<&str>) -> Result, - ) -> Result { + ) -> Result<(bool, bool), String> { let mut latest = None::<(u64, bool)>; + let mut requested = None::; + let mut submitted = 0; for page in 1.. { let response = request( &format!("{}/{id}/reviews?limit=50&page={page}", self.endpoint), @@ -691,19 +808,39 @@ impl Forge { .as_array() .ok_or("Forge API returned a non-array review list")?; if reviews.is_empty() { - return Ok(latest.is_some_and(|(_, approved)| approved)); + return Ok(( + latest.is_some_and(|(_, approved)| approved), + requested.is_some_and(|id| id > submitted), + )); } for review in reviews { - if review.pointer("/user/id").and_then(Value::as_u64) != Some(viewer) { + if review.pointer("/user/id").and_then(Value::as_u64) != Some(viewer) + || review.get("team").is_some_and(|team| !team.is_null()) + { continue; } let state = field(review, "state")?; match state { - "COMMENT" | "PENDING" | "REQUEST_REVIEW" => continue, + "PENDING" => continue, + "REQUEST_REVIEW" => { + if review.get("dismissed").and_then(Value::as_bool) == Some(false) { + if let Some(id) = review.get("id").and_then(Value::as_u64) { + requested = Some(requested.map_or(id, |previous| previous.max(id))); + } + } + continue; + } + "COMMENT" => { + if let Some(id) = review.get("id").and_then(Value::as_u64) { + submitted = submitted.max(id); + } + continue; + } "APPROVED" | "REQUEST_CHANGES" => {} _ => return Err("Forge API returned invalid review state".into()), } let id = number(review, "id")?; + submitted = submitted.max(id); let dismissed = review .get("dismissed") .and_then(Value::as_bool) @@ -917,6 +1054,7 @@ fn summary( closed, draft, approved: false, + review_requested: false, created_at: created_at(value)?, }) } diff --git a/src/main.rs b/src/main.rs index e405529..8efe64e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -107,7 +107,8 @@ Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the Snapshot anchors stay immutable; stale comments lose gutter markers; no publishing 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 - โœ“ before the PR number marks your API account's active approval; blank may mean unavailable + ๐Ÿ™ marks a personal review request; ๐Ÿ‘ marks your active approval (API account) + 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 config.resolve_history_layout = \"columns\" (left/base/right) or \"base_below\" (left/right above base) diff --git a/src/repository.rs b/src/repository.rs index c1a351f..dcdbd8e 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -1,7 +1,7 @@ //! Read-only active-repository discovery and isolated revision comparison. use crate::diff::{ BlameRevision, Catalog, Category, ChangedFile, Comparison, ComparisonMode, FileStatus, - PullRequestPage, RepoKind, Revision, RevisionTarget, line_hunks, + PullRequestPage, PullRequestReviewState, RepoKind, Revision, RevisionTarget, line_hunks, }; use std::{ collections::{BTreeMap, BTreeSet}, @@ -1631,12 +1631,7 @@ impl Repository { remote: remote.clone(), number: pull.number, }, - label: format!( - "{}#{} {}", - if pull.approved { "โœ“ " } else { " " }, - pull.number, - pull.title - ), + label: format!("#{} {}", pull.number, pull.title), description: pull.description, category: if pull.closed { Category::Closed @@ -1647,6 +1642,13 @@ impl Repository { }, }, pull.created_at, + if pull.review_requested { + PullRequestReviewState::Requested + } else if pull.approved { + PullRequestReviewState::Approved + } else { + PullRequestReviewState::None + }, ) }) .collect(), diff --git a/src/ruby.rs b/src/ruby.rs index c0fa546..0100975 100644 --- a/src/ruby.rs +++ b/src/ruby.rs @@ -58,6 +58,8 @@ pub struct Config { pub diff_view_mode: crate::diff::ViewMode, pub diff_split_orientation: crate::panes::SplitAxis, pub diff_follow_working_copy: bool, + pub pull_request_review_requested_symbol: String, + pub pull_request_approved_symbol: String, pub resolve_history_layout: crate::editor::ResolverLayout, } @@ -112,6 +114,8 @@ impl Default for Config { diff_view_mode: crate::diff::ViewMode::Split, diff_split_orientation: crate::panes::SplitAxis::Vertical, diff_follow_working_copy: true, + pull_request_review_requested_symbol: "๐Ÿ™".into(), + pull_request_approved_symbol: "๐Ÿ‘".into(), resolve_history_layout: crate::editor::ResolverLayout::Columns, } } @@ -153,6 +157,8 @@ pub fn load_config(path: &Path) -> Result { diff_view_mode, diff_split_orientation, diff_follow_working_copy, + pull_request_review_requested_symbol, + pull_request_approved_symbol, resolve_history_layout, .. } = *response; @@ -195,6 +201,8 @@ pub fn load_config(path: &Path) -> Result { config.diff_view_mode = diff_view_mode; config.diff_split_orientation = diff_split_orientation; config.diff_follow_working_copy = diff_follow_working_copy; + config.pull_request_review_requested_symbol = pull_request_review_requested_symbol; + config.pull_request_approved_symbol = pull_request_approved_symbol; config.resolve_history_layout = resolve_history_layout; Ok(config) } @@ -556,9 +564,25 @@ struct ConfigFrame { diff_view_mode: crate::diff::ViewMode, diff_split_orientation: crate::panes::SplitAxis, diff_follow_working_copy: bool, + #[serde(deserialize_with = "pull_request_symbol")] + pull_request_review_requested_symbol: String, + #[serde(deserialize_with = "pull_request_symbol")] + pull_request_approved_symbol: String, resolve_history_layout: crate::editor::ResolverLayout, } +fn pull_request_symbol<'de, D: serde::Deserializer<'de>>( + deserializer: D, +) -> Result { + let symbol = String::deserialize(deserializer)?; + if symbol.chars().any(char::is_control) { + return Err(serde::de::Error::custom( + "pull request symbols must not contain control characters", + )); + } + Ok(symbol) +} + fn whitespace_characters<'de, D: serde::Deserializer<'de>>( deserializer: D, ) -> Result, D::Error> { diff --git a/src/ruby_config.rb b/src/ruby_config.rb index 91f451f..32c51c3 100644 --- a/src/ruby_config.rb +++ b/src/ruby_config.rb @@ -157,6 +157,8 @@ module Vamp @diff_view_mode = "split" @diff_split_orientation = "vertical" @diff_follow_working_copy = true + @pull_request_review_requested_symbol = "๐Ÿ™" + @pull_request_approved_symbol = "๐Ÿ‘" @resolve_history_layout = "columns" end @@ -327,6 +329,18 @@ module Vamp @diff_follow_working_copy = enabled end + # @param symbol [String] empty to hide the review-request indicator + # @return [String] + def pull_request_review_requested_symbol=(symbol) + @pull_request_review_requested_symbol = pull_request_symbol!(symbol, "pull_request_review_requested_symbol") + end + + # @param symbol [String] empty to hide the approval indicator + # @return [String] + def pull_request_approved_symbol=(symbol) + @pull_request_approved_symbol = pull_request_symbol!(symbol, "pull_request_approved_symbol") + end + # @param layout [String] "columns" or "base_below" # @return [String] def resolve_history_layout=(layout) @@ -382,10 +396,19 @@ module Vamp "diff_context_lines" => @diff_context_lines, "diff_expand_lines" => @diff_expand_lines, "diff_view_mode" => @diff_view_mode, "diff_split_orientation" => @diff_split_orientation, "diff_follow_working_copy" => @diff_follow_working_copy, + "pull_request_review_requested_symbol" => @pull_request_review_requested_symbol, + "pull_request_approved_symbol" => @pull_request_approved_symbol, "resolve_history_layout" => @resolve_history_layout } end private + + def pull_request_symbol!(symbol, label) + symbol = Vamp.string!(symbol, label) + raise ArgumentError, "#{label} must not contain control characters" if symbol.match?(/[[:cntrl:]]/) + + symbol + end # @param spaces [Integer, nil] # @return [Integer, nil] def tab_spaces!(spaces) diff --git a/src/session.rs b/src/session.rs index 5b8c1b8..fde153f 100644 --- a/src/session.rs +++ b/src/session.rs @@ -542,6 +542,8 @@ impl EditorSession { editor.set_tab_spaces(config.tab_spaces, config.language_tab_spaces); editor.set_diff_context_options(config.diff_context_lines, config.diff_expand_lines); editor.set_diff_display_options(config.diff_view_mode, config.diff_split_orientation); + editor.pull_request_review_requested_symbol = config.pull_request_review_requested_symbol; + editor.pull_request_approved_symbol = config.pull_request_approved_symbol; editor.resolve_history_layout = config.resolve_history_layout; if let Err(error) = editor.set_chrome_config(config.top_bar, config.bottom_bar, config.labels) diff --git a/src/tui.rs b/src/tui.rs index d8465cb..2349d6a 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -3536,6 +3536,18 @@ fn draw_diff( panels.picker_scroll.distance, ); let mut lines = Vec::with_capacity(count * row_height); + let (requested_symbol, approved_symbol) = editor.map_or(("๐Ÿ™", "๐Ÿ‘"), |editor| { + ( + editor.pull_request_review_requested_symbol.as_str(), + editor.pull_request_approved_symbol.as_str(), + ) + }); + let symbol_width = if pull_requests { + requested_symbol.width().max(approved_symbol.width()) + } else { + 0 + }; + let decoration_width = symbol_width + usize::from(symbol_width > 0); for (index, revision) in entries .enumerate() .skip(first) @@ -3549,10 +3561,21 @@ fn draw_diff( } else { Style::default() }; + let symbol = if pull_requests { + match diff.pull_request_review_state(revision) { + crate::diff::PullRequestReviewState::Requested => requested_symbol, + crate::diff::PullRequestReviewState::Approved => approved_symbol, + crate::diff::PullRequestReviewState::None => "", + } + } else { + "" + }; let label = format!( - "{} {}", + "{} {symbol}{:padding$}{}", if index == diff.selected { ">" } else { " " }, - display_text(&revision.label) + "", + display_text(&revision.label), + padding = decoration_width.saturating_sub(symbol.width()), ); let width = usize::from(results.width); if row_height == 2 { @@ -3560,9 +3583,10 @@ fn draw_diff( lines.push(Line::styled( visible_line( &format!( - "{}{}", - if pull_requests { " " } else { " " }, - display_text(&revision.description) + "{:indent$}{}", + "", + display_text(&revision.description), + indent = 2 + decoration_width, ), 0, width, diff --git a/tests/unit/diff.rs b/tests/unit/diff.rs index e0c1a17..8c7e7b1 100644 --- a/tests/unit/diff.rs +++ b/tests/unit/diff.rs @@ -433,6 +433,7 @@ fn pull_page(closed: bool, rows: &[(u64, &str, &str)], has_more: bool) -> Outcom }, ), date.into(), + PullRequestReviewState::None, ) }) .collect(), @@ -440,6 +441,51 @@ fn pull_page(closed: bool, rows: &[(u64, &str, &str)], has_more: bool) -> Outcom } } +#[test] +fn pull_request_review_refresh_preserves_identity_sorting_and_raw_search_labels() { + let mut state = DiffState::pull_requests("origin".into()); + state.take_request(); + state.apply_result( + 1, + Ok(pull_page( + false, + &[(1, "First", "1"), (2, "Second", "2")], + true, + )), + ); + state.handle(Input::End); + for review_state in [ + PullRequestReviewState::Approved, + PullRequestReviewState::Requested, + PullRequestReviewState::None, + ] { + let mut page = pull_page(false, &[(1, "First updated", "3")], true); + if let Outcome::PullRequestPage { + result: Ok(page), .. + } = &mut page + { + page.entries[0].2 = review_state; + } + state.apply_result(1, Ok(page)); + let selected = state.entries().nth(state.selected).unwrap(); + assert_eq!(selected.label, "#1 First updated"); + assert_eq!(state.pull_request_review_state(selected), review_state); + assert_eq!(state.entries().count(), 2); + assert_eq!(state.entries().next().unwrap().target, selected.target); + } + state.handle(Input::Char('๐Ÿ™')); + assert_eq!(state.entries().count(), 0); + state.handle(Input::Backspace); + for ch in "#1 First updated".chars() { + state.handle(Input::Char(ch)); + } + assert_eq!(state.entries().count(), 1); + assert_eq!( + state.pull_request_review_state(&catalog().entries[0]), + PullRequestReviewState::None + ); +} + #[test] fn pull_request_categories_keep_query_and_select_loaded_rows_before_completion() { let mut state = DiffState::pull_requests("origin".into()); diff --git a/tests/unit/forge.rs b/tests/unit/forge.rs index a8a9c21..b7bbdaa 100644 --- a/tests/unit/forge.rs +++ b/tests/unit/forge.rs @@ -174,7 +174,9 @@ fn unavailable_viewer_or_metadata_never_fabricates_personal_approval() { let forge = parse(url, None).unwrap(); for unavailable_viewer in [true, false] { let metadata = forge.metadata_with( - &[json!({"number":7,"iid":7,"additions":0,"deletions":0})], + &[json!({"number":7,"iid":7,"additions":0,"deletions":0, + "requested_reviewers":[{"login":"viewer","id":42}], + "reviewers":[{"username":"viewer"}]})], &mut |url, body| { if url.ends_with("/user") { return if unavailable_viewer { @@ -194,6 +196,8 @@ fn unavailable_viewer_or_metadata_never_fabricates_personal_approval() { }, ); assert!(metadata.values().all(|details| !details.approved)); + assert!(metadata.values().all(|details| details.review_requested + == (provider == Provider::Github && !unavailable_viewer))); if provider == Provider::Gitea { assert_eq!(metadata[&7].size, Some((0, 0))); } @@ -201,6 +205,310 @@ fn unavailable_viewer_or_metadata_never_fabricates_personal_approval() { } } +#[test] +fn github_outstanding_requests_use_list_users_independently_of_review_metadata() { + let forge = parse("https://github.com/owner/repo", None).unwrap(); + for (reviewers, expected) in [ + (json!([{"login":"VIEWER"}]), true), // Direct or re-requested after approval. + (json!([]), false), // Completed or removed requests are absent. + (json!([{"login":"another-user"}]), false), + (Value::Null, false), + ] { + for denied in [false, true] { + let metadata = forge.metadata_with( + &[json!({"number":7,"requested_reviewers":reviewers, + "requested_teams":[{"slug":"viewer"}],"additions":3,"deletions":4})], + &mut |url, body| { + if url.ends_with("/user") { + return Ok(json!({"login":"viewer"})); + } + assert!(body.is_some()); + if denied { + return Err("GraphQL denied".into()); + } + Ok( + json!({"data":{"repository":{"p7":{"additions":3,"deletions":4, + "reviews":{"nodes":[{"state":"APPROVED","author":{"login":"viewer"}}]}}}}}), + ) + }, + ); + assert_eq!(metadata[&7].review_requested, expected); + assert_eq!(metadata[&7].approved, !denied); + assert_eq!(metadata[&7].size, Some((3, 4))); + } + } +} + +#[test] +fn gitlab_outstanding_requests_require_personal_unfinished_review_and_follow_pages() { + let forge = parse("https://gitlab.com/group/repo", None).unwrap(); + let mut values: Vec<_> = (1..=10) + .map(|id| json!({"iid":id,"reviewers":[{"username":"viewer"}]})) + .collect(); + values[8]["reviewers"] = json!([]); // Removed reviewer. + values[9]["reviewers"] = json!([{"username":"another-user"}]); + let mut request_reads = 0; + let metadata = forge.metadata_with(&values, &mut |url, body| { + if url.ends_with("/user") { + return Ok(json!({"username":"viewer"})); + } + let body: Value = serde_json::from_str(body.unwrap()).unwrap(); + let query = body["query"].as_str().unwrap(); + assert!(!query.contains("mutation")); + if !query.contains("reviewState") { + return Ok(json!({"data":{"project":{ + "p1":{"diffStatsSummary":{"additions":5,"deletions":6}}, + "approved":{"nodes":[{"iid":"1"},{"iid":"4"}],"pageInfo":{"hasNextPage":false}} + }}})); + } + request_reads += 1; + assert!(query.contains("reviewers(first:20")); + assert!(!query.contains("p9:") && !query.contains("p10:")); + if query.contains("after:\"next\"") { + return Ok(json!({"data":{"project":{"p2":{"reviewers":{ + "nodes":[{"username":"viewer","mergeRequestInteraction":{"reviewState":"REVIEW_STARTED"}}], + "pageInfo":{"hasNextPage":false} + }}}}})); + } + let mut entries = serde_json::Map::new(); + for (id, state) in [(1, "UNREVIEWED"), (3, "REVIEWED"), (4, "APPROVED"), + (5, "REQUESTED_CHANGES"), (6, "UNAPPROVED"), (7, ""), (8, "UNKNOWN")] { + entries.insert(format!("p{id}"), json!({"reviewers":{ + "nodes":[{"username":"viewer","mergeRequestInteraction":{"reviewState":state}}], + "pageInfo":{"hasNextPage":false} + }})); + } + entries.insert("p2".into(), json!({"reviewers":{ + "nodes":[{"username":"another-user","mergeRequestInteraction":{"reviewState":"UNREVIEWED"}}], + "pageInfo":{"hasNextPage":true,"endCursor":"next"} + }})); + Ok(json!({"data":{"project":entries}})) + }); + assert_eq!(request_reads, 2); + assert_eq!( + metadata + .iter() + .filter_map(|(id, details)| details.review_requested.then_some(*id)) + .collect::>(), + [1, 2] + ); + assert!(metadata[&1].approved && metadata[&4].approved); + assert_eq!(metadata[&1].size, Some((5, 6))); +} + +#[test] +fn gitlab_request_failures_preserve_approval_and_statistics() { + let forge = parse("https://gitlab.com/group/repo", None).unwrap(); + for denied in [ + Err("review states denied".into()), + Ok(json!({"data":{"project":{ + "p7":{"reviewers":null} + }}})), + Ok(json!({"data":{"project":{"p7":{"reviewers":{"nodes":[ + {"username":"viewer","mergeRequestInteraction":{"reviewState":null}} + ]}}}}})), + ] { + let metadata = forge.metadata_with( + &[json!({"iid":7,"reviewers":[{"username":"viewer"}]})], + &mut |url, body| { + if url.ends_with("/user") { + return Ok(json!({"username":"viewer"})); + } + let body: Value = serde_json::from_str(body.unwrap()).unwrap(); + if body["query"].as_str().unwrap().contains("reviewState") { + return denied.clone(); + } + Ok(json!({"data":{"project":{ + "p7":{"diffStatsSummary":{"additions":0,"deletions":1}}, + "approved":{"nodes":[{"iid":"7"}],"pageInfo":{"hasNextPage":false}} + }}})) + }, + ); + assert!(!metadata[&7].review_requested); + assert!(metadata[&7].approved); + assert_eq!(metadata[&7].size, Some((0, 1))); + } +} + +#[test] +fn gitlab_request_batches_cover_fifty_rows_without_exceeding_complexity_limit() { + let forge = parse("https://gitlab.com/group/repo", None).unwrap(); + let values: Vec<_> = (1..=50) + .map(|id| { + json!({ + "iid":id,"reviewers":[{"username":"viewer"}] + }) + }) + .collect(); + let mut batches = 0; + let metadata = forge.metadata_with(&values, &mut |url, body| { + if url.ends_with("/user") { + return Ok(json!({"username":"viewer"})); + } + let body: Value = serde_json::from_str(body.unwrap()).unwrap(); + let query = body["query"].as_str().unwrap(); + if !query.contains("reviewState") { + return Err("independent metadata unavailable".into()); + } + batches += 1; + assert_eq!(query.matches("reviewers(first:20)").count(), 10); + let entries: serde_json::Map<_, _> = (1..=50) + .filter(|id| query.contains(&format!("p{id}:mergeRequest"))) + .map(|id| (format!("p{id}"), json!({"reviewers":{ + "nodes":[{"username":"viewer","mergeRequestInteraction":{"reviewState":"UNREVIEWED"}}], + "pageInfo":{"hasNextPage":false} + }}))).collect(); + Ok(json!({"data":{"project":entries}})) + }); + assert_eq!(batches, 5); + assert_eq!(metadata.len(), 50); + assert!(metadata.values().all(|details| details.review_requested)); +} + +#[test] +fn gitea_requests_ignore_list_assignments_and_use_current_direct_review_rows() { + let forge = parse("https://codeberg.org/owner/repo", None).unwrap(); + let own = |id, state: &str| { + json!({ + "id":id,"user":{"id":42},"team":null,"state":state,"dismissed":false + }) + }; + for (reviews, requested, approved) in [ + (json!([own(10, "REQUEST_REVIEW")]), true, false), + (json!([own(10, "APPROVED")]), false, true), // Completed or removed request row. + (json!([own(10, "COMMENT")]), false, false), + (json!([own(10, "REQUEST_CHANGES")]), false, false), + ( + json!([own(10, "APPROVED"), own(20, "REQUEST_REVIEW")]), + true, + true, + ), + ( + json!([ + own(20, "REQUEST_REVIEW"), + own(10, "APPROVED"), + own(30, "PENDING") + ]), + true, + true, + ), + ( + json!([own(10, "REQUEST_REVIEW"), own(20, "COMMENT")]), + false, + false, + ), + ( + json!([own(10, "REQUEST_REVIEW"), own(20, "APPROVED")]), + false, + true, + ), + ( + json!([{"id":10,"user":{"id":7},"state":"REQUEST_REVIEW","dismissed":false}]), + false, + false, + ), + ( + json!([{"id":10,"user":{"id":42},"team":{"id":1},"state":"REQUEST_REVIEW","dismissed":false}]), + false, + false, + ), + ( + json!([{"id":20,"user":{"id":42},"state":"REQUEST_REVIEW"}, + own(10, "APPROVED")]), + false, + true, + ), + (json!([]), false, false), + ] { + let mut pages = vec![reviews, json!([])].into_iter(); + let metadata = forge.metadata_with( + &[json!({"number":7,"additions":1,"deletions":2,"requested_reviewers":[{"id":42}]})], + &mut |url, body| { + assert!(body.is_none()); + if url.ends_with("/user") { + return Ok(json!({"id":42})); + } + pages.next().ok_or_else(|| "unexpected page".into()) + }, + ); + assert_eq!(metadata[&7].review_requested, requested); + assert_eq!(metadata[&7].approved, approved); + assert_eq!(metadata[&7].size, Some((1, 2))); + } +} + +#[test] +fn gitea_incomplete_request_pages_do_not_fabricate_requests_or_lose_list_statistics() { + let forge = parse("https://codeberg.org/owner/repo", None).unwrap(); + let metadata = forge.metadata_with( + &[json!({"number":7,"additions":0,"deletions":2})], + &mut |url, _| { + if url.ends_with("/user") { + return Ok(json!({"id":42})); + } + if url.ends_with("page=1") { + return Ok(json!([{"id":10,"user":{"id":42},"team":null, + "state":"REQUEST_REVIEW","dismissed":false}])); + } + Err("remaining reviews denied".into()) + }, + ); + assert!(!metadata[&7].review_requested); + assert_eq!(metadata[&7].size, Some((0, 2))); +} + +#[test] +fn summaries_keep_outstanding_requests_and_approvals_independent() { + for (url, provider) in [ + ("https://github.com/owner/repo", Provider::Github), + ("https://gitlab.com/owner/repo", Provider::Gitlab), + ("https://codeberg.org/owner/repo", Provider::Gitea), + ] { + let forge = parse(url, None).unwrap(); + let page = forge.page_with(false, 1, |url, body| { + if url.ends_with("/user") { + return Ok(json!({"id":42,"login":"viewer","username":"viewer"})); + } + if let Some(body) = body { + let body: Value = serde_json::from_str(body).unwrap(); + if body["query"].as_str().unwrap().contains("reviewState") { + return Ok(json!({"data":{"project":{"p7":{"reviewers":{ + "nodes":[{"username":"viewer","mergeRequestInteraction":{"reviewState":"UNREVIEWED"}}], + "pageInfo":{"hasNextPage":false} + }}}}})); + } + return if provider == Provider::Github { + Ok(json!({"data":{"repository":{"p7":{"additions":1,"deletions":2, + "reviews":{"nodes":[{"state":"APPROVED","author":{"login":"viewer"}}]}}}}})) + } else { + Ok(json!({"data":{"project":{ + "p7":{"diffStatsSummary":{"additions":1,"deletions":2}}, + "approved":{"nodes":[{"iid":"7"}],"pageInfo":{"hasNextPage":false}} + }}})) + }; + } + if url.contains("/7/reviews?") { + return Ok(if url.ends_with("page=1") { + json!([ + {"id":10,"user":{"id":42},"state":"APPROVED","dismissed":false}, + {"id":20,"user":{"id":42},"state":"REQUEST_REVIEW","dismissed":false} + ]) + } else { + json!([]) + }); + } + Ok(json!([{"number":7,"iid":7,"title":"Request again","created_at":"2024-01-01T00:00:00Z", + "state":if provider == Provider::Gitlab { "opened" } else { "open" }, + "merged":false,"merged_at":null,"user":{"login":"author"},"author":{"username":"author"}, + "requested_reviewers":[{"login":"viewer","id":42}],"reviewers":[{"username":"viewer"}], + "additions":1,"deletions":2}])) + }).unwrap(); + assert_eq!(page.entries.len(), 1); + assert!(page.entries[0].approved && page.entries[0].review_requested); + assert!(page.entries[0].description.contains("+1 -2")); + } +} + #[test] fn draft_summaries_keep_open_closed_and_merged_states_distinct() { for provider in [Provider::Github, Provider::Gitlab, Provider::Gitea] { diff --git a/tests/unit/ruby.rs b/tests/unit/ruby.rs index 85c4ce3..6e924cc 100644 --- a/tests/unit/ruby.rs +++ b/tests/unit/ruby.rs @@ -106,7 +106,7 @@ fn review_directory_resolves_config_paths_and_keeps_session_startup_lazy() { } #[test] -fn review_directory_wire_field_is_required_nullable_and_validated() { +fn review_directory_and_review_symbol_wire_fields_are_required_and_validated() { let mut frame = serde_json::json!({ "version": 1, "type": "config", "plugins": [], "lsp": {}, "bindings": [], "review_directory": null, "leader": "space", @@ -119,6 +119,7 @@ fn review_directory_wire_field_is_required_nullable_and_validated() { "diff_context_lines": 10, "diff_expand_lines": 15, "diff_view_mode": "split", "diff_split_orientation": "vertical", "resolve_history_layout": "columns", + "pull_request_review_requested_symbol": "๐Ÿ™", "pull_request_approved_symbol": "๐Ÿ‘", "diff_follow_working_copy": true }); for value in [serde_json::Value::Null, serde_json::json!("reviews")] { @@ -136,10 +137,83 @@ fn review_directory_wire_field_is_required_nullable_and_validated() { frame["review_directory"] = value; assert!(serde_json::from_value::(frame.clone()).is_err()); } + frame["review_directory"] = serde_json::Value::Null; + for field in [ + "pull_request_review_requested_symbol", + "pull_request_approved_symbol", + ] { + for value in ["", "๐Ÿ‘ฉโ€๐Ÿš€ R", "ok"] { + frame[field] = serde_json::json!(value); + assert!(serde_json::from_value::(frame.clone()).is_ok()); + } + for value in [ + serde_json::json!(null), + serde_json::json!(false), + serde_json::json!(7), + serde_json::json!([]), + serde_json::json!({}), + serde_json::json!("\0"), + serde_json::json!("\n"), + serde_json::json!("\t"), + serde_json::json!("\u{7f}"), + serde_json::json!("\u{85}"), + ] { + frame[field] = value; + assert!(serde_json::from_value::(frame.clone()).is_err()); + } + frame.as_object_mut().unwrap().remove(field); + assert!(serde_json::from_value::(frame.clone()).is_err()); + frame[field] = serde_json::json!(""); + } frame.as_object_mut().unwrap().remove("review_directory"); assert!(serde_json::from_value::(frame).is_err()); } +#[test] +fn pull_request_symbols_load_defaults_custom_and_empty_and_reject_invalid_ruby_values() { + let fixture = Fixture::new(); + let path = fixture.file("config.rb", "Vamp.configure { |_| }"); + let config = load_config(&path).unwrap(); + assert_eq!(config.pull_request_review_requested_symbol, "๐Ÿ™"); + assert_eq!(config.pull_request_approved_symbol, "๐Ÿ‘"); + for symbol in ["", "๐Ÿ‘ฉโ€๐Ÿš€ R", "ok"] { + let value = serde_json::to_string(symbol).unwrap(); + let path = fixture.file("config.rb", &format!( + "Vamp.configure {{ |c| c.pull_request_review_requested_symbol = {value}; c.pull_request_approved_symbol = {value} }}" + )); + let config = load_config(&path).unwrap(); + assert_eq!(config.pull_request_review_requested_symbol, symbol); + assert_eq!(config.pull_request_approved_symbol, symbol); + } + for field in [ + "pull_request_review_requested_symbol", + "pull_request_approved_symbol", + ] { + for value in [ + "nil", + "false", + "7", + "[]", + "{}", + ":ok", + r#""\n""#, + r#""\t""#, + r#""\0""#, + r#""\u007f""#, + r#""\u0085""#, + ] { + let path = fixture.file( + "config.rb", + &format!("Vamp.configure {{ |c| c.{field} = {value} }}"), + ); + assert!( + load_config(&path).unwrap_err().contains(field), + "{field}: {value}" + ); + } + } +} + #[test] fn tab_spaces_follow_active_language_and_preserve_native_edit_semantics() { let fixture = Fixture::new(); @@ -544,9 +618,9 @@ fn config_leader_defaults_custom_keys_validation_and_required_wire_field() { assert!(load_config(&path).unwrap_err().contains(error), "{value}"); } for frame in [ - r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true,"resolve_history_layout":"columns"}"#, - r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"leader":null,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true,"resolve_history_layout":"columns"}"#, - r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"leader":7,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true,"resolve_history_layout":"columns"}"#, + r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true,"pull_request_review_requested_symbol":"๐Ÿ™","pull_request_approved_symbol":"๐Ÿ‘","resolve_history_layout":"columns"}"#, + r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"leader":null,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true,"pull_request_review_requested_symbol":"๐Ÿ™","pull_request_approved_symbol":"๐Ÿ‘","resolve_history_layout":"columns"}"#, + r#"{"type":"config","version":1,"plugins":[],"lsp":{},"review_directory":null,"bindings":[],"leader":7,"file_explorer_side":"right","file_picker_view":"list","file_explorer_view":"tree","clipboard_mode":"system","git_blame":true,"git_gutter":"bar","scroll_distance":0,"highlight_trailing_whitespace":false,"whitespace_characters":{},"whitespace_scope":"all","tab_spaces":null,"language_tab_spaces":{},"top_bar":{},"bottom_bar":{},"popup":{"command":{},"search":{}},"labels":{},"diff_context_lines":10,"diff_expand_lines":15,"diff_view_mode":"split","diff_split_orientation":"vertical","diff_follow_working_copy":true,"pull_request_review_requested_symbol":"๐Ÿ™","pull_request_approved_symbol":"๐Ÿ‘","resolve_history_layout":"columns"}"#, ] { assert!(serde_json::from_str::(frame).is_err(), "{frame}"); } @@ -632,6 +706,7 @@ end "labels": {}, "diff_context_lines": 10, "diff_expand_lines": 15, "diff_view_mode": "split", "diff_split_orientation": "vertical", "resolve_history_layout": "columns", + "pull_request_review_requested_symbol": "๐Ÿ™", "pull_request_approved_symbol": "๐Ÿ‘", "diff_follow_working_copy": true }); assert!(serde_json::from_value::(frame.clone()).is_ok()); @@ -773,6 +848,7 @@ end"#, "labels": {}, "diff_context_lines": 10, "diff_expand_lines": 15, "diff_view_mode": "split", "diff_split_orientation": "vertical", "resolve_history_layout": "columns", + "pull_request_review_requested_symbol": "๐Ÿ™", "pull_request_approved_symbol": "๐Ÿ‘", "diff_follow_working_copy": true }); for value in ["all", "leading"] { @@ -1036,6 +1112,7 @@ fn config_diff_excerpt_counts_accept_custom_values_and_reject_invalid_values() { "clipboard_mode": "system", "diff_context_lines": 0, "diff_expand_lines": 7, "diff_view_mode": "split", "diff_split_orientation": "vertical", "resolve_history_layout": "columns", + "pull_request_review_requested_symbol": "๐Ÿ™", "pull_request_approved_symbol": "๐Ÿ‘", "diff_follow_working_copy": true }); for setting in ["diff_context_lines", "diff_expand_lines"] { @@ -1148,6 +1225,7 @@ fn config_scroll_distance_accepts_nil_and_counts_and_rejects_invalid_values() { "diff_context_lines": 10, "diff_expand_lines": 15, "diff_view_mode": "split", "diff_split_orientation": "vertical", "resolve_history_layout": "columns", + "pull_request_review_requested_symbol": "๐Ÿ™", "pull_request_approved_symbol": "๐Ÿ‘", "diff_follow_working_copy": true, "scroll_distance": 0 }); for distance in [None, Some(0), Some(7), Some(usize::MAX)] { @@ -1582,6 +1660,7 @@ fn lsp_wire_configuration_is_resolved_after_decoding() { "clipboard_mode": "system", "diff_context_lines": 10, "diff_expand_lines": 15, "diff_view_mode": "split", "diff_split_orientation": "vertical", "resolve_history_layout": "columns", + "pull_request_review_requested_symbol": "๐Ÿ™", "pull_request_approved_symbol": "๐Ÿ‘", "diff_follow_working_copy": true }); let Response::Config(config) = serde_json::from_value::(frame.clone()).unwrap() diff --git a/tests/unit/session.rs b/tests/unit/session.rs index e596547..76c7d11 100644 --- a/tests/unit/session.rs +++ b/tests/unit/session.rs @@ -1481,6 +1481,23 @@ end assert_eq!(session.editor().lines(), &["a!bc"]); } +#[test] +fn configured_pull_request_symbols_reach_editor() { + let fixture = Fixture::new(); + fixture.file("buffer.txt", "abc"); + let session = fixture.open( + r#"Vamp.configure do |config| + config.pull_request_review_requested_symbol = "๐Ÿ‘ฉโ€๐Ÿš€ review" + config.pull_request_approved_symbol = "" +end"#, + ); + assert_eq!( + session.editor().pull_request_review_requested_symbol, + "๐Ÿ‘ฉโ€๐Ÿš€ review" + ); + assert_eq!(session.editor().pull_request_approved_symbol, ""); +} + #[test] fn diff_display_preferences_survive_reopening() { use crate::{diff::ViewMode, panes::SplitAxis}; diff --git a/tests/unit/tui.rs b/tests/unit/tui.rs index 5be9279..0a577d0 100644 --- a/tests/unit/tui.rs +++ b/tests/unit/tui.rs @@ -3515,6 +3515,7 @@ fn pull_request_picker_renders_all_rows_without_a_pinned_offset() { }, }, format!("2026-01-{:02}T00:00:00Z", 20 - index), + crate::diff::PullRequestReviewState::None, ) }) .collect(), @@ -3526,17 +3527,17 @@ fn pull_request_picker_renders_all_rows_without_a_pinned_offset() { let text = screen_text(&terminal); assert!(text.contains("Pull requests")); 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("[partial]")); diff.handle(Input::End); render_diff(&mut terminal, &diff, &mut panels); let text = screen_text(&terminal); - assert!(text.contains("> #19 Pull request")); + assert!(text.contains("> #19 Pull request")); assert!(!text.contains("#00 Pull request")); diff.handle(Input::Home); 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.apply_result( 2, @@ -3555,10 +3556,124 @@ fn pull_request_picker_renders_all_rows_without_a_pinned_offset() { diff.handle(Input::Char('\t')); render_diff(&mut terminal, &diff, &mut panels); let text = screen_text(&terminal); - assert!(text.contains("> #01 Pull request")); + assert!(text.contains("> #01 Pull request")); assert!(!text.contains("#00 Pull request") && !text.contains("#19 Pull request")); } } +#[test] +fn pull_request_review_symbols_use_terminal_cells_and_keep_the_selection_arrow_independent() { + use crate::diff::{ + Category, Outcome, PullRequestPage, PullRequestReviewState, RepoKind, Revision, + RevisionTarget, + }; + let mut diff = DiffState::pull_requests("origin".into()); + diff.take_request(); + diff.apply_result( + 1, + Ok(Outcome::PullRequestPage { + closed: false, + result: Ok(PullRequestPage { + kind: RepoKind::Git, + remote: "origin".into(), + has_more: false, + entries: [ + PullRequestReviewState::Requested, + PullRequestReviewState::Approved, + PullRequestReviewState::None, + ] + .into_iter() + .enumerate() + .map(|(index, state)| { + ( + Revision { + target: RevisionTarget::PullRequest { + remote: "origin".into(), + number: index as u64, + }, + label: format!("#{index} title"), + description: format!("metadata{index}"), + category: Category::Open, + }, + (3 - index).to_string(), + state, + ) + }) + .collect(), + }), + }), + ); + for (requested, approved, with_editor) in [ + ("๐Ÿ™", "๐Ÿ‘", false), + ("๐Ÿ™", "๐Ÿ‘", true), + ("๐Ÿ‘ฉโ€๐Ÿš€ R", "็•Œ", true), + ("ask", "ok", true), + ("", "๐Ÿ‘", true), + ("๐Ÿ™", "", true), + ("", "", true), + ] { + let mut editor = Editor::open(None, vec![]).unwrap(); + editor.pull_request_review_requested_symbol = requested.into(); + editor.pull_request_approved_symbol = approved.into(); + let slot = requested.width().max(approved.width()); + let slot = slot + usize::from(slot > 0); + for width in [100, 44] { + let mut terminal = + Terminal::new(ratatui::backend::TestBackend::new(width, 24)).unwrap(); + let mut panels = WheelPanels::default(); + terminal + .draw(|frame| { + panels.begin_frame(); + draw_diff( + frame, + &diff, + frame.area(), + "โ†‘/โ†“", + "h/j/k/l", + &mut panels, + with_editor.then_some(&editor), + ); + }) + .unwrap(); + let buffer = terminal.backend().buffer(); + let mut number_column = None; + for (index, symbol) in [requested, approved, ""].into_iter().enumerate() { + let (x, y) = (0..24) + .find_map(|y| { + (0..width).find_map(|x| { + (buffer[(x, y)].symbol() == "#" + && buffer[(x + 1, y)].symbol() == index.to_string()) + .then_some((x, y)) + }) + }) + .expect("all review states render"); + assert_eq!(*number_column.get_or_insert(x), x); + let arrow_x = x - (2 + slot) as u16; + assert_eq!( + buffer[(arrow_x, y)].symbol(), + if index == 0 { ">" } else { " " } + ); + if !symbol.is_empty() { + assert_eq!( + buffer[(arrow_x + 2, y)].symbol(), + symbol.graphemes(true).next().unwrap() + ); + } else { + for blank in arrow_x + 2..x { + assert_eq!(buffer[(blank, y)].symbol(), " "); + } + } + if width == 100 { + assert_eq!( + buffer[(x, y + 1)].symbol(), + "m", + "metadata aligns with #number" + ); + } + } + assert!(!screen_text(&terminal).contains('โœ“')); + } + } +} #[test] fn diff_excerpts_render_original_lines_and_expand_without_losing_changes() {