fix: keep pull request filters session-local

This commit is contained in:
Valdemar Lorenzen
2026-10-21 13:56:24 +02:00
parent fbcb519e13
commit ea7c240eb6
9 changed files with 86 additions and 53 deletions
-10
View File
@@ -1,13 +1,3 @@
- [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.
- [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.
- [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:
- personal review buckets with multi-select checkboxes
- ranked sorting by review request, update time, approval, and creation time
- persisted settings with explicit config.rb section overrides
- [ ] 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.
+2 -2
View File
@@ -109,7 +109,7 @@ Indicators compare the current editor text (including unsaved edits) against Git
`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.pull_request_sort` and `config.pull_request_filters` optionally override saved pull-request picker settings, independently. `nil` or an omitted option uses saved preferences, then built-in defaults. A configured section replaces that entire saved section; omitted sort criteria use built-in defaults, not previously saved rules.
`config.pull_request_sort` optionally overrides saved sorting: `nil` or an omitted option uses saved sorting, then built-in defaults. A configured sort section replaces that entire saved section; omitted criteria use built-in defaults. `config.pull_request_filters` sets the session's initial filter buckets; `nil` or omission includes all buckets. UI filter changes are shared across tabs and picker reopenings for this vamp session only. Restart restores the configured filters or includes all buckets; previously saved filters are ignored.
```ruby
config.pull_request_sort = {
@@ -121,7 +121,7 @@ config.pull_request_filters = [:requested, :approved]
Sort fields are `requested`, `updated_at`, `approved`, and `created_at`, in that default precedence order. Default direction is `desc`; timestamp criteria support `asc` for oldest first. `precedence` is `nil` (unnumbered default position) or a unique integer in `1..4`. Filters are any subset of `requested` (Requested from me), `approved` (Approved by me without an outstanding personal request), and `other` (Other / unavailable). `[]` excludes all requests; `{}` resets sorting to built-in defaults. Unknown fields/options, duplicate ranks/filters, and invalid types or directions reject configuration.
Enter Apply in the picker saves global preferences to `$XDG_STATE_HOME/vamp/pull_requests.json`, or `~/.local/state/vamp/pull_requests.json` if XDG state is unset or relative. This also works with `--no-config`. Configuration remains a startup override, not a lock: UI edits work and persist, but explicitly configured sections win again next startup. Save failures report session-only changes. Corrupt, incompatible, or non-regular storage is reported and preserved rather than reset or overwritten.
Enter Apply in the sort menu saves changed sorting to `$XDG_STATE_HOME/vamp/pull_requests.json`, or `~/.local/state/vamp/pull_requests.json` if XDG state is unset or relative. Applying filters never writes this file, and subsequent sort saves exclude filters. This also works with `--no-config`. Configuration remains a startup override, not a lock: UI edits work for the session, but explicitly configured settings win again next startup. Sort save failures report session-only changes. Corrupt, incompatible, or non-regular storage is reported and preserved rather than reset or overwritten.
`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.
+2
View File
@@ -90,6 +90,8 @@ Diff checks cover staged category/filter navigation, stale request cancellation,
Pull-request checks cover ranked sort menus, rank collisions/default slots, timestamp directions, independent personal review flags, checkbox unions, Apply/Cancel, category/query composition, streamed updates, and selection identity. Storage and session checks cover restart persistence, config section overrides, corrupt/incompatible-file preservation, and failed writes with isolated paths. Live PTY smoke at 100×24 and 44×16 exercises checkbox Apply/Cancel, restart restoration, creation-rank movement and ascending direction, and a config filter override. All forge responses come from a disposable curl fixture; the editor and descendants run with networking denied.
Session-filter regressions verify no file creation on filter Apply, retention across picker reopenings, restart reset, and saved sorting without filter data. Previously saved filters, including invalid legacy values, are ignored without altering valid sorting or changing the file on load. Live network-denied PTY smoke checks these transitions plus configured filter restoration and the active-filter indicator.
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.
+1 -1
View File
@@ -416,7 +416,7 @@ A later request for changes or dismissal removes 👍; comment-only and pending
**Ctrl+f** opens review filter checkboxes. **Space** toggles the highlighted bucket: **Requested from me**, **Approved by me**, or **Other / unavailable**. Checked buckets combine with OR, then intersect with the category and text query. All checked includes every review state; none checked produces no matches. These are personal states, not the PR's team approval threshold: teammates' outstanding requests do not cancel your approval. A new request for your own review takes precedence over your earlier approval. Unavailable metadata remains Other, not a claim that the PR has never been reviewed.
**Enter** applies either settings view; **Escape** discards its draft and returns to the PR list. Applied settings preserve the selected request where possible and carry across tabs and sessions. Category counts remain loaded-category totals, and partial-search notices still apply. Settings use already-loaded metadata without additional API requests. Explicit [configuration overrides](configuration.md#preferences) win again at startup; the settings view indicates when this applies.
**Enter** applies either settings view; **Escape** discards its draft and returns to the PR list. Applied settings preserve the selected request where possible and carry across tabs and picker reopenings. Sorting persists across sessions; filters are session-local and restart from `config.pull_request_filters` or all buckets when unset. Previously saved filters are ignored. The picker displays its active review buckets whenever any are excluded. Category counts remain loaded-category totals, and partial-search notices still apply. Settings use already-loaded metadata without additional API requests. Explicit [configuration overrides](configuration.md#preferences) win again at startup; the settings view indicates when this applies.
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.
+2 -1
View File
@@ -5346,8 +5346,9 @@ impl Editor {
diff.handle(input);
if std::mem::take(&mut diff.pull_request_preferences_changed) {
let preferences = diff.pull_request_preferences.clone();
self.pull_request_preferences_changed |=
self.pull_request_preferences.sort != preferences.sort;
self.pull_request_preferences = preferences.clone();
self.pull_request_preferences_changed = true;
for diff in self
.diff
.iter_mut()
+1 -1
View File
@@ -110,7 +110,7 @@ Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/Drafts/A
🙏 marks a personal review request; 👍 marks your active approval (API account)
Ctrl+s sort menu: 1-4 precedence, Backspace clear, Left/Right date direction
Ctrl+f filter menu: Space includes/excludes personal review states; Enter applies, Esc cancels
Applied choices persist; config.pull_request_sort / config.pull_request_filters override at startup
Sorting persists; filters last for this session; config.pull_request_sort / config.pull_request_filters set startup choices
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
+22 -5
View File
@@ -1,4 +1,4 @@
use crate::pull_requests::Preferences;
use crate::pull_requests::{Preferences, SortSettings};
use serde::{Deserialize, Serialize};
use std::{
ffi::OsStr,
@@ -17,6 +17,16 @@ struct StoredPreferences<T> {
preferences: T,
}
#[derive(Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
struct StoredSort<T> {
#[serde(default)]
sort: T,
// Previously saved filters are deliberately ignored, including invalid values.
#[serde(default, rename = "filters", skip_serializing)]
_filters: serde::de::IgnoredAny,
}
pub(crate) fn default_path() -> Result<PathBuf, String> {
path_from(
std::env::var_os("XDG_STATE_HOME").as_deref(),
@@ -46,7 +56,7 @@ fn read_preferences(path: &Path) -> Result<Option<Preferences>, String> {
Ok(_) => {}
}
let bytes = fs::read(path).map_err(|error| error.to_string())?;
let stored: StoredPreferences<Preferences> =
let stored: StoredPreferences<StoredSort<SortSettings>> =
serde_json::from_slice(&bytes).map_err(|error| error.to_string())?;
if stored.version != 1 {
return Err(format!(
@@ -54,8 +64,12 @@ fn read_preferences(path: &Path) -> Result<Option<Preferences>, String> {
stored.version
));
}
stored.preferences.validate()?;
Ok(Some(stored.preferences))
let preferences = Preferences {
sort: stored.preferences.sort,
..Preferences::default()
};
preferences.validate()?;
Ok(Some(preferences))
}
pub(crate) struct Store {
@@ -113,7 +127,10 @@ impl Store {
read_preferences(path)?;
let bytes = serde_json::to_vec_pretty(&StoredPreferences {
version: 1,
preferences,
preferences: StoredSort {
sort: &preferences.sort,
_filters: serde::de::IgnoredAny,
},
})
.map_err(|error| error.to_string())?;
let parent = path.parent().ok_or("preferences path has no parent")?;
+27 -4
View File
@@ -59,18 +59,43 @@ fn loading_is_lazy_and_applied_preferences_round_trip_atomically() {
);
store.save(&preferences).unwrap();
let (_, loaded, notice) = Store::load(Ok(path.clone()));
assert_eq!(loaded, preferences);
assert_eq!(loaded.sort, preferences.sort);
assert_eq!(loaded.filters, Preferences::default().filters);
assert!(!fs::read_to_string(&path).unwrap().contains("filters"));
assert!(notice.is_none());
preferences.filters.clear();
preferences.sort.clear();
store.save(&preferences).unwrap();
assert_eq!(Store::load(Ok(path)).1, preferences);
assert_eq!(Store::load(Ok(path)).1, Preferences::default());
assert_eq!(
fs::read_dir(fixture.0.join("state/vamp")).unwrap().count(),
1
);
}
#[test]
fn previously_saved_filters_are_ignored_without_changing_sort_or_file() {
let fixture = Fixture::new();
let path = fixture.path();
fs::create_dir_all(path.parent().unwrap()).unwrap();
for filters in [
r#"[]"#,
r#"["approved"]"#,
r#"["unknown"]"#,
r#"{"invalid":true}"#,
] {
let bytes = format!(
r#"{{"version":1,"preferences":{{"sort":{{"created_at":{{"precedence":1,"direction":"asc"}}}},"filters":{filters}}}}}"#
);
fs::write(&path, &bytes).unwrap();
let (_, loaded, notice) = Store::load(Ok(path.clone()));
assert!(notice.is_none());
assert_eq!(loaded.filters, Preferences::default().filters);
assert_eq!(loaded.sort[&SortField::CreatedAt].direction, Direction::Asc);
assert_eq!(fs::read_to_string(&path).unwrap(), bytes);
}
}
#[test]
fn corrupt_incompatible_and_invalid_data_is_preserved_even_after_apply() {
let fixture = Fixture::new();
@@ -83,8 +108,6 @@ fn corrupt_incompatible_and_invalid_data_is_preserved_even_after_apply() {
r#"{"version":1,"preferences":{"sort":{"updated_at":{"direction":"up"}}}}"#,
r#"{"version":1,"preferences":{"sort":{"updated_at":{"precedence":0}}}}"#,
r#"{"version":1,"preferences":{"sort":{"updated_at":{"precedence":1},"created_at":{"precedence":1}}}}"#,
r#"{"version":1,"preferences":{"filters":["unknown"]}}"#,
r#"{"version":1,"preferences":{"filters":["requested","requested"]}}"#,
] {
fs::write(&path, bytes).unwrap();
let (store, loaded, notice) = Store::load(Ok(path.clone()));
+29 -29
View File
@@ -3838,7 +3838,7 @@ fn async_responses_edit_only_current_bound_working_after_and_refresh_projection(
}
#[test]
fn applied_pull_request_preferences_survive_startup_without_config() {
fn applied_pull_request_filters_survive_picker_reopen_but_not_restart() {
use crate::pull_requests::ReviewFilter;
let fixture = Fixture::new();
let path = fixture.0.join("state/pull_requests.json");
@@ -3864,13 +3864,31 @@ fn applied_pull_request_preferences_survive_startup_without_config() {
session.handle(Input::Char(' '));
assert!(!path.exists());
session.handle(Input::Enter);
assert!(path.exists());
assert!(!path.exists());
assert!(!session.editor.pull_request_preferences_changed);
assert_eq!(
session.editor.pull_request_preferences.filters,
[ReviewFilter::Other]
);
session.handle(Input::Escape);
session.editor.execute("pull_requests");
assert!(session.editor.take_diff_request().is_some());
session.handle(Input::Ctrl('f'));
session.handle(Input::Enter);
assert_eq!(
session.editor.pull_request_preferences.filters,
[ReviewFilter::Other]
);
// A sort edit still persists, without carrying the active filter to disk.
session.handle(Input::Ctrl('s'));
session.handle(Input::Char('1'));
session.handle(Input::Enter);
assert!(path.exists());
let reopened =
EditorSession::open_with_preferences_path(None, vec![], None, Some(Ok(path))).unwrap();
assert_eq!(
reopened.editor.pull_request_preferences.filters,
[ReviewFilter::Other]
crate::pull_requests::Preferences::default().filters
);
assert_eq!(reopened.editor.pull_request_configured, [false, false]);
}
@@ -3897,7 +3915,7 @@ fn configured_pull_request_sections_replace_only_their_persisted_section() {
"config.pull_request_sort = {}",
Preferences {
sort: Default::default(),
filters: preferences.filters.clone(),
filters: Preferences::default().filters,
},
[true, false],
),
@@ -3920,7 +3938,7 @@ fn configured_pull_request_sections_replace_only_their_persisted_section() {
},
)]
.into(),
filters: preferences.filters.clone(),
filters: Preferences::default().filters,
},
[true, false],
),
@@ -3952,19 +3970,10 @@ fn corrupt_pull_request_storage_warns_and_apply_remains_session_only() {
assert!(session.editor.message().contains("session-only"));
session.editor.execute("pull_requests");
assert!(session.editor.take_diff_request().is_some());
session.handle(Input::Ctrl('f'));
for input in [
Input::Char(' '),
Input::Down,
Input::Char(' '),
Input::Down,
Input::Char(' '),
Input::Enter,
] {
session.handle(input);
}
session.handle(Input::Ctrl('s'));
session.handle(Input::Char('1'));
session.handle(Input::Enter);
assert!(session.editor.message().contains("preserved"));
assert!(session.editor.pull_request_preferences.filters.is_empty());
assert_eq!(fs::read_to_string(path).unwrap(), "preserve invalid bytes");
}
@@ -3978,19 +3987,10 @@ fn failed_pull_request_save_keeps_applied_session_preferences_and_reports_failur
fs::write(&parent, "do not replace").unwrap();
session.editor.execute("pull_requests");
assert!(session.editor.take_diff_request().is_some());
session.handle(Input::Ctrl('f'));
for input in [
Input::Char(' '),
Input::Down,
Input::Char(' '),
Input::Down,
Input::Char(' '),
Input::Enter,
] {
session.handle(input);
}
session.handle(Input::Ctrl('s'));
session.handle(Input::Char('1'));
session.handle(Input::Enter);
assert!(session.editor.message().contains("session-only"));
assert!(session.editor.pull_request_preferences.filters.is_empty());
assert!(!session.editor.pull_request_preferences_changed);
assert_eq!(fs::read_to_string(parent).unwrap(), "do not replace");
}