a73x

51e7d5ae

Say how much feedback is still unanswered, in the lists a reviewer scans

a73x   2026-08-13 07:02

Commit message
Say how much feedback is still unanswered, in the lists a reviewer scans

`28fc6751` made one thread's answered-ness visible on `patch show`. That
answers "is this thread done" one patch at a time, which is the wrong shape
for the question a reviewer starts with: *which* patches have unanswered
feedback. Answering it meant opening every patch.

`PatchState::unresolved_comments` is the one definition behind the number,
and `patch list`, its `--json`, the dashboard and the web list all call it.
Putting it on the folded state is what stops a fourth divergence of the kind
`10bb2d84` argued about ids and `952390ad` is arguing about timestamps: there
is one predicate, and changing it changes every view at once. It is derived
rather than serialized, so it never reaches the on-disk fold cache — the same
argument that keeps `looks_merged` off `PatchState` — and it costs a pass over
a `Vec` the list already folded, not a second walk of the DAG.

A count cannot carry the caveat `patch show` prints beside a thread, so two
edge cases had to be decided outright.

A stale claim counts as answered. A resolution keeps the revision it was
pinned to, and one older than the tip is shown as `resolved by Alice at r1 —
2 revisions landed since`: a hint, deliberately not a state change. Counting
it as unanswered would make that state change in every list view. It would
also be a number nobody could act on — staleness is a property of the pair
(claim revision, current tip), not of the thread, so a revision touching an
unrelated file would re-raise feedback nobody reopened, and the only way back
to zero would be to re-resolve every thread on every revision. That is the
noisier question "not re-confirmed against the tip", not the one a review
queue asks. The caveat stays on the surface with room to spell it out.

A withdrawn comment is not outstanding. A tombstone has no words left to
answer, and counting it would leave a row clearable only by "resolving" a
comment that says nothing. A `non_blocking` comment *is* counted: excluding it
would let the count be silenced by a flag rather than by an answer.

What each surface renders:

* `patch list` appends ` (N unresolved)`, omitted at zero like `(N new)`
  beside it. `--json` takes the opposite rule and always carries
  `unresolved_comments`, because a caller cannot tell an absent key from a
  zero — the rule `resolved` and `non_blocking` already follow. Full ids are
  unchanged.
* The dashboard puts `!N` between the status and the title. The list pane is
  ~28 columns, so a suffix would be truncated away on every real repository.
* The web list gains an Unresolved column, blank at zero.

Each also grew a filter, since counting without filtering only half solves it.
The CLI spells it `--unresolved`, a flag that composes with the status axis
(`--unresolved -a` widens to closed patches); the dashboard binds `u` on the
patch pane, a second axis rather than a fourth value of the status cycle; the
web adds `?filter=unresolved`, a fifth value of a single-select bar that
implies open. The shapes differ because a flag composes and a link bar does
not — the number means the same thing on all three, which is what has to
agree. An empty result names the filter instead of claiming the repository is
empty.

Reading stays reading: a test snapshots every collab ref across `patch list`,
`--unresolved`, `--json` and both web pages and asserts nothing moved.

Fixes: 76c6cb58
src/cli.rs
Old New
@@ -735,6 +735,13 @@ pub enum PatchCmd {
735 /// Filter by label (repeatable; matches patches carrying any of the given labels) 735 /// Filter by label (repeatable; matches patches carrying any of the given labels)
736 #[arg(long)] 736 #[arg(long)]
737 label: Vec<String>, 737 label: Vec<String>,
738 /// Show only patches with unanswered inline feedback
739 ///
740 /// A flag rather than a fourth value of the status filter, so the two
741 /// axes compose: `--unresolved` alone still means open patches, and
742 /// `--unresolved -a` widens to closed ones.
743 #[arg(long)]
744 unresolved: bool,
738 }, 745 },
739 /// Show patch details 746 /// Show patch details
740 #[command(alias = "view", alias = "info")] 747 #[command(alias = "view", alias = "info")]
src/lib.rs
Old New
@@ -656,9 +656,13 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
656 json, 656 json,
657 sort, 657 sort,
658 label, 658 label,
659 unresolved,
659 } => { 660 } => {
661 let opts = patch::ListOpts {
662 unresolved_only: unresolved,
663 };
660 if json { 664 if json {
661 let output = patch::list_json(repo, all, archived, sort, &label)?; 665 let output = patch::list_json(repo, all, archived, sort, &label, opts)?;
662 println!("{}", output); 666 println!("{}", output);
663 return Ok(()); 667 return Ok(());
664 } 668 }
@@ -673,6 +677,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
673 offset, 677 offset,
674 sort, 678 sort,
675 &label, 679 &label,
680 opts,
676 &mut std::io::stdout(), 681 &mut std::io::stdout(),
677 )?; 682 )?;
678 Ok(()) 683 Ok(())
src/patch.rs
Old New
@@ -160,6 +160,27 @@ fn count_unread(repo: &git2::Repository, id: &str) -> Option<usize> {
160 Some(revwalk.count()) 160 Some(revwalk.count())
161 } 161 }
162 162
163 /// Listing filters that are not the status/label axis
164 /// [`cli::filter_sort_paginate`] already applies.
165 ///
166 /// A struct rather than another positional `bool` beside `show_closed` and
167 /// `show_archived`: three adjacent bare bools at a call site transpose
168 /// silently, and this one decides which patches a reviewer is shown.
169 #[derive(Debug, Clone, Copy, Default)]
170 pub struct ListOpts {
171 /// Keep only patches with unanswered inline feedback — see
172 /// [`PatchState::unresolved_comments`] for what that counts.
173 ///
174 /// Composes with the status axis rather than replacing it: on its own it
175 /// still means open patches, and with `show_closed` it widens to closed
176 /// ones. That is why it is not a fourth value of the status filter.
177 pub unresolved_only: bool,
178 }
179
180 // Same exemption `list_to_writer` below already carries, and for the same
181 // reason: these are the list's filter/sort/paginate knobs, and bundling them
182 // into one struct would only move the same eight names behind a builder.
183 #[allow(clippy::too_many_arguments)]
163 pub fn list( 184 pub fn list(
164 repo: &Repository, 185 repo: &Repository,
165 show_closed: bool, 186 show_closed: bool,
@@ -168,12 +189,19 @@ pub fn list(
168 offset: Option<usize>, 189 offset: Option<usize>,
169 sort: SortMode, 190 sort: SortMode,
170 labels: &[String], 191 labels: &[String],
192 opts: ListOpts,
171 ) -> Result<Vec<ListEntry>, crate::error::Error> { 193 ) -> Result<Vec<ListEntry>, crate::error::Error> {
172 let patches = if show_archived { 194 let patches = if show_archived {
173 state::list_patches_with_archived(repo)? 195 state::list_patches_with_archived(repo)?
174 } else { 196 } else {
175 state::list_patches(repo)? 197 state::list_patches(repo)?
176 }; 198 };
199 // Before pagination, so `-n 10 --unresolved` means "ten patches needing
200 // attention" rather than "however many of the first ten happened to".
201 let patches: Vec<PatchState> = patches
202 .into_iter()
203 .filter(|p| !opts.unresolved_only || p.unresolved_comments() > 0)
204 .collect();
177 let filtered = cli::filter_sort_paginate(patches, show_closed, labels, sort, offset, limit); 205 let filtered = cli::filter_sort_paginate(patches, show_closed, labels, sort, offset, limit);
178 let entries = filtered 206 let entries = filtered
179 .into_iter() 207 .into_iter()
@@ -194,11 +222,29 @@ pub fn list_to_writer(
194 offset: Option<usize>, 222 offset: Option<usize>,
195 sort: SortMode, 223 sort: SortMode,
196 labels: &[String], 224 labels: &[String],
225 opts: ListOpts,
197 writer: &mut dyn std::io::Write, 226 writer: &mut dyn std::io::Write,
198 ) -> Result<(), crate::error::Error> { 227 ) -> Result<(), crate::error::Error> {
199 let entries = list(repo, show_closed, show_archived, limit, offset, sort, labels)?; 228 let entries = list(
229 repo,
230 show_closed,
231 show_archived,
232 limit,
233 offset,
234 sort,
235 labels,
236 opts,
237 )?;
200 if entries.is_empty() { 238 if entries.is_empty() {
201 writeln!(writer, "No patches found.").ok(); 239 // An empty *filtered* list is not an empty repository, and saying "No
240 // patches found." under `--unresolved` reads as "there is no work
241 // here" when it means "nothing is waiting on you". Same rule the web
242 // list already follows for its filter; see issue c43c459d.
243 if opts.unresolved_only {
244 writeln!(writer, "No patches with unanswered feedback.").ok();
245 } else {
246 writeln!(writer, "No patches found.").ok();
247 }
202 return Ok(()); 248 return Ok(());
203 } 249 }
204 // Over every patch in the repository; see `issue::list_to_writer`. 250 // Over every patch in the repository; see `issue::list_to_writer`.
@@ -215,6 +261,14 @@ pub fn list_to_writer(
215 Some(n) if n > 0 => format!(" ({} new)", n), 261 Some(n) if n > 0 => format!(" ({} new)", n),
216 _ => String::new(), 262 _ => String::new(),
217 }; 263 };
264 // Omitted at zero, like `(N new)` beside it. A list is scanned, and a
265 // column of `(0 unresolved)` on every row trains the eye to skip the
266 // very thing the column exists to show. `--json` has the opposite
267 // requirement and always carries the number; see `patch_json_value`.
268 let unresolved = match p.unresolved_comments() {
269 0 => String::new(),
270 n => format!(" ({} unresolved)", n),
271 };
218 // `merged?` — not `merged` — for a patch that only looks merged. 272 // `merged?` — not `merged` — for a patch that only looks merged.
219 // Reachability is a hint here, never the recorded status. 273 // Reachability is a hint here, never the recorded status.
220 // 274 //
@@ -226,7 +280,7 @@ pub fn list_to_writer(
226 // has usually been deleted. `patch show` still prints it. See 850f3b8e. 280 // has usually been deleted. `patch show` still prints it. See 850f3b8e.
227 writeln!( 281 writeln!(
228 writer, 282 writer,
229 "{} {:7} {}{} → {} (by {}){}{}", 283 "{} {:7} {}{} → {} (by {}){}{}{}",
230 abbrev.of(&p.id), 284 abbrev.of(&p.id),
231 p.status_display(repo), 285 p.status_display(repo),
232 p.title, 286 p.title,
@@ -234,7 +288,8 @@ pub fn list_to_writer(
234 p.base_ref, 288 p.base_ref,
235 p.author.name, 289 p.author.name,
236 stale, 290 stale,
237 unread 291 unread,
292 unresolved
238 ) 293 )
239 .ok(); 294 .ok();
240 } 295 }
@@ -260,8 +315,21 @@ pub fn list_to_writer(
260 /// Skipped entirely when false, so it never reads as a positive assertion that 315 /// Skipped entirely when false, so it never reads as a positive assertion that
261 /// a patch is unmerged — the hint cannot see a squash, so its absence means 316 /// a patch is unmerged — the hint cannot see a squash, so its absence means
262 /// nothing. 317 /// nothing.
318 /// `unresolved_comments` is added here for the same reason and *not* skipped
319 /// at zero, which is where it parts company with `looks_merged` above. That
320 /// hint's absence means nothing — it cannot see a squash — so leaving it out
321 /// is honest. A count's absence is ambiguous: a caller reading `--json` could
322 /// not tell "no feedback outstanding" from "this build does not report it",
323 /// which is the same argument that keeps `resolved` and `non_blocking` always
324 /// serialized on `InlineComment`. So the key is always present.
263 fn patch_json_value(repo: &Repository, patch: &PatchState) -> Result<serde_json::Value, Error> { 325 fn patch_json_value(repo: &Repository, patch: &PatchState) -> Result<serde_json::Value, Error> {
264 let mut value = serde_json::to_value(patch)?; 326 let mut value = serde_json::to_value(patch)?;
327 if let Some(obj) = value.as_object_mut() {
328 obj.insert(
329 "unresolved_comments".to_string(),
330 serde_json::Value::from(patch.unresolved_comments()),
331 );
332 }
265 if patch.looks_merged(repo) { 333 if patch.looks_merged(repo) {
266 if let Some(obj) = value.as_object_mut() { 334 if let Some(obj) = value.as_object_mut() {
267 obj.insert("looks_merged".to_string(), serde_json::Value::Bool(true)); 335 obj.insert("looks_merged".to_string(), serde_json::Value::Bool(true));
@@ -276,8 +344,18 @@ pub fn list_json(
276 show_archived: bool, 344 show_archived: bool,
277 sort: SortMode, 345 sort: SortMode,
278 labels: &[String], 346 labels: &[String],
347 opts: ListOpts,
279 ) -> Result<String, crate::error::Error> { 348 ) -> Result<String, crate::error::Error> {
280 let entries = list(repo, show_closed, show_archived, None, None, sort, labels)?; 349 let entries = list(
350 repo,
351 show_closed,
352 show_archived,
353 None,
354 None,
355 sort,
356 labels,
357 opts,
358 )?;
281 let patches: Vec<serde_json::Value> = entries 359 let patches: Vec<serde_json::Value> = entries
282 .iter() 360 .iter()
283 .map(|e| patch_json_value(repo, &e.patch)) 361 .map(|e| patch_json_value(repo, &e.patch))
src/server/http/repo/patches.rs
Old New
@@ -17,6 +17,18 @@ pub enum PatchListFilter {
17 Closed, 17 Closed,
18 Merged, 18 Merged,
19 All, 19 All,
20 /// Open patches carrying unanswered inline feedback.
21 ///
22 /// A fifth value of this one-axis bar rather than a second query parameter,
23 /// because the bar is single-select and a second axis would multiply it out
24 /// for a combination nobody asks for: "closed patches with unanswered
25 /// feedback" is not a review queue. It implies open for the same reason —
26 /// this is the "what is waiting on me" view, and a merged patch is not.
27 ///
28 /// The CLI spells the same filter as a composable `--unresolved` flag. The
29 /// shapes differ because a flag composes and a link bar does not; the
30 /// *number* means the same thing on both, which is what has to agree.
31 Unresolved,
20 } 32 }
21 33
22 impl PatchListFilter { 34 impl PatchListFilter {
@@ -25,6 +37,7 @@ impl PatchListFilter {
25 Some("closed") => PatchListFilter::Closed, 37 Some("closed") => PatchListFilter::Closed,
26 Some("merged") => PatchListFilter::Merged, 38 Some("merged") => PatchListFilter::Merged,
27 Some("all") => PatchListFilter::All, 39 Some("all") => PatchListFilter::All,
40 Some("unresolved") => PatchListFilter::Unresolved,
28 _ => PatchListFilter::Open, 41 _ => PatchListFilter::Open,
29 } 42 }
30 } 43 }
@@ -35,6 +48,7 @@ impl PatchListFilter {
35 PatchListFilter::Closed => "closed", 48 PatchListFilter::Closed => "closed",
36 PatchListFilter::Merged => "merged", 49 PatchListFilter::Merged => "merged",
37 PatchListFilter::All => "all", 50 PatchListFilter::All => "all",
51 PatchListFilter::Unresolved => "unresolved",
38 } 52 }
39 } 53 }
40 } 54 }
@@ -62,6 +76,11 @@ pub struct PatchListItem {
62 /// belongs. See issue `850f3b8e`. 76 /// belongs. See issue `850f3b8e`.
63 pub base_ref: String, 77 pub base_ref: String,
64 pub updated: Timestamp, 78 pub updated: Timestamp,
79 /// How much inline feedback on this patch is still unanswered, from
80 /// `PatchState::unresolved_comments` — the same function `patch list` and
81 /// the dashboard call, so the three surfaces cannot disagree about what
82 /// the number means.
83 pub unresolved: usize,
65 } 84 }
66 85
67 #[derive(askama::Template, askama_web::WebTemplate)] 86 #[derive(askama::Template, askama_web::WebTemplate)]
@@ -205,6 +224,16 @@ pub async fn patches(
205 PatchListFilter::All => { 224 PatchListFilter::All => {
206 git_collab::state::list_patches_with_archived(&repo).unwrap_or_default() 225 git_collab::state::list_patches_with_archived(&repo).unwrap_or_default()
207 } 226 }
227 // Served from the cheap non-archived listing like `Open`, which it
228 // narrows: the archived namespace holds closed patches, and a closed
229 // patch is not waiting on a reviewer whatever its threads say.
230 PatchListFilter::Unresolved => git_collab::state::list_patches(&repo)
231 .unwrap_or_default()
232 .into_iter()
233 .filter(|p| {
234 p.status == git_collab::state::PatchStatus::Open && p.unresolved_comments() > 0
235 })
236 .collect(),
208 }; 237 };
209 238
210 // Built from every patch id in the repository, not from the rows above — 239 // Built from every patch id in the repository, not from the rows above —
@@ -216,6 +245,7 @@ pub async fn patches(
216 .map(|p| { 245 .map(|p| {
217 let id = p.id.clone(); 246 let id = p.id.clone();
218 let short_id = abbrev.of(&id).to_string(); 247 let short_id = abbrev.of(&id).to_string();
248 let unresolved = p.unresolved_comments();
219 PatchListItem { 249 PatchListItem {
220 short_id, 250 short_id,
221 id, 251 id,
@@ -225,6 +255,7 @@ pub async fn patches(
225 labels: p.labels.join(", "), 255 labels: p.labels.join(", "),
226 base_ref: p.base_ref, 256 base_ref: p.base_ref,
227 updated: Timestamp::new(p.last_updated), 257 updated: Timestamp::new(p.last_updated),
258 unresolved,
228 } 259 }
229 }) 260 })
230 .collect(); 261 .collect();
src/server/http/templates/patches.html
Old New
@@ -10,6 +10,9 @@
10 <a href="/{{ repo_name }}/patches?filter=closed"{% if filter == "closed" %} class="active"{% endif %}>closed</a> 10 <a href="/{{ repo_name }}/patches?filter=closed"{% if filter == "closed" %} class="active"{% endif %}>closed</a>
11 <a href="/{{ repo_name }}/patches?filter=merged"{% if filter == "merged" %} class="active"{% endif %}>merged</a> 11 <a href="/{{ repo_name }}/patches?filter=merged"{% if filter == "merged" %} class="active"{% endif %}>merged</a>
12 <a href="/{{ repo_name }}/patches?filter=all"{% if filter == "all" %} class="active"{% endif %}>all</a> 12 <a href="/{{ repo_name }}/patches?filter=all"{% if filter == "all" %} class="active"{% endif %}>all</a>
13 {# The review queue: open patches with feedback nobody has answered. Last in
14 the bar because it narrows `open` rather than sitting beside it. #}
15 <a href="/{{ repo_name }}/patches?filter=unresolved"{% if filter == "unresolved" %} class="active"{% endif %}>unresolved</a>
13 </p> 16 </p>
14 {% if patches.is_empty() %} 17 {% if patches.is_empty() %}
15 {# An empty *filtered* list is not an empty repository. Saying "No patches." 18 {# An empty *filtered* list is not an empty repository. Saying "No patches."
@@ -18,6 +21,11 @@
18 issue c43c459d. #} 21 issue c43c459d. #}
19 {% if total == 0 %} 22 {% if total == 0 %}
20 <p style="color: #666;">No patches yet.</p> 23 <p style="color: #666;">No patches yet.</p>
24 {% else if filter == "unresolved" %}
25 {# "No unresolved patches" would name the filter but not what it means. An
26 empty review queue is good news and should read as such. #}
27 <p style="color: #666;">No patches with unanswered feedback. ({{ total }} total —
28 <a href="/{{ repo_name }}/patches?filter=all">show all</a>)</p>
21 {% else %} 29 {% else %}
22 <p style="color: #666;">No {{ filter }} patches. ({{ total }} total — 30 <p style="color: #666;">No {{ filter }} patches. ({{ total }} total —
23 <a href="/{{ repo_name }}/patches?filter=all">show all</a>)</p> 31 <a href="/{{ repo_name }}/patches?filter=all">show all</a>)</p>
@@ -33,6 +41,7 @@
33 <th>Author</th> 41 <th>Author</th>
34 <th>Labels</th> 42 <th>Labels</th>
35 <th>Base</th> 43 <th>Base</th>
44 <th>Unresolved</th>
36 <th>Updated</th> 45 <th>Updated</th>
37 </tr> 46 </tr>
38 </thead> 47 </thead>
@@ -45,6 +54,9 @@
45 <td style="color: #666;">{{ p.author }}</td> 54 <td style="color: #666;">{{ p.author }}</td>
46 <td style="color: #666;">{{ p.labels }}</td> 55 <td style="color: #666;">{{ p.labels }}</td>
47 <td class="mono" style="color: #666;">{{ p.base_ref }}</td> 56 <td class="mono" style="color: #666;">{{ p.base_ref }}</td>
57 {# Blank rather than 0, matching `patch list`: a column of zeroes trains
58 the eye to skip the column. The heading still says what it counts. #}
59 <td class="mono">{% if p.unresolved > 0 %}<span style="color: #b60;">{{ p.unresolved }}</span>{% endif %}</td>
48 <td class="mono timestamp" style="color: #666;" title="{{ p.updated.full }}">{{ p.updated.short }}</td> 60 <td class="mono timestamp" style="color: #666;" title="{{ p.updated.full }}">{{ p.updated.short }}</td>
49 </tr> 61 </tr>
50 {% endfor %} 62 {% endfor %}
src/state.rs
Old New
@@ -913,6 +913,53 @@ impl IssueState {
913 } 913 }
914 914
915 impl PatchState { 915 impl PatchState {
916 /// How much review feedback on this patch is still unanswered.
917 ///
918 /// The single definition behind the number `patch list`, its `--json`, the
919 /// dashboard and the web list all render. It lives here, on the folded
920 /// state, so those four surfaces cannot drift the way ids and timestamps
921 /// have: there is one predicate, and changing it changes every view at
922 /// once.
923 ///
924 /// Derived, never stored. A serialized field would be written into the
925 /// on-disk fold cache as though it were folded state — the same argument
926 /// that keeps `looks_merged` out of `PatchState`. Here it costs nothing to
927 /// recompute: the comments are already in hand from the fold the list did
928 /// anyway, so this is a pass over a `Vec` already in memory, not a second
929 /// walk of the DAG.
930 ///
931 /// Two edge cases forced a decision, because a count cannot carry the
932 /// caveat `patch show` prints beside a thread:
933 ///
934 /// * **A stale claim counts as answered.** A resolution keeps the revision
935 /// it was pinned to, and one older than the tip is shown by `patch show`
936 /// as `resolved by Alice at r1 — 2 revisions landed since`: a hint, and
937 /// deliberately not a state change. Counting it as unanswered would make
938 /// that state change in every list view. It would also be unactionable:
939 /// staleness is a property of the pair (claim revision, current tip), not
940 /// of the thread, so a revision touching an unrelated file would re-raise
941 /// feedback nobody reopened, and the only way back to zero would be to
942 /// re-resolve every thread on every revision. That is a different
943 /// question — "not re-confirmed against the tip" — and a noisier one than
944 /// a review queue asks. The count counts recorded facts; the caveat stays
945 /// on the surface with room to spell it out.
946 ///
947 /// * **A withdrawn comment is not outstanding.** A deleted inline comment
948 /// is a tombstone with no words left to answer, so counting it would send
949 /// a reviewer to read `[deleted]` and leave the row clearable only by
950 /// "resolving" a comment that says nothing.
951 ///
952 /// A `non_blocking` comment *is* counted. It is unanswered feedback that
953 /// the reviewer chose to mark as a suggestion rather than a demand;
954 /// excluding it would let a count be silenced by the flag rather than by
955 /// an answer.
956 pub fn unresolved_comments(&self) -> usize {
957 self.inline_comments
958 .iter()
959 .filter(|c| c.resolved.is_none() && !c.deleted)
960 .count()
961 }
962
916 /// Resolve a comment/review ID prefix against everything on this patch 963 /// Resolve a comment/review ID prefix against everything on this patch
917 /// that carries an editable body. 964 /// that carries an editable body.
918 /// 965 ///
src/tui/mod.rs
Old New
@@ -1290,7 +1290,11 @@ mod tests {
1290 let mut app = test_app(); 1290 let mut app = test_app();
1291 app.mode = ViewMode::PatchDetail; 1291 app.mode = ViewMode::PatchDetail;
1292 app.current_patch = Some(make_patch_with_revisions()); 1292 app.current_patch = Some(make_patch_with_revisions());
1293 app.set_diff_text(&(0..100).map(|i| format!(" line {}\n", i)).collect::<String>()); 1293 app.set_diff_text(
1294 &(0..100)
1295 .map(|i| format!(" line {}\n", i))
1296 .collect::<String>(),
1297 );
1294 app.rebuild_patch_rows(); 1298 app.rebuild_patch_rows();
1295 app.patch_viewport = 10; 1299 app.patch_viewport = 10;
1296 1300
@@ -1681,4 +1685,214 @@ mod tests {
1681 app.list_state.select(Some(1)); 1685 app.list_state.select(Some(1));
1682 assert!(app.linked_patch_for_selected().is_none()); 1686 assert!(app.linked_patch_for_selected().is_none());
1683 } 1687 }
1688
1689 // ── Unanswered feedback ──────────────────────────────────────────────
1690 //
1691 // The dashboard renders the same number `patch list` and the web list do,
1692 // out of the same `PatchState::unresolved_comments`. What differs is only
1693 // how much room it has to say it in: the list pane is ~28 columns, so the
1694 // count goes *before* the title, where truncation cannot eat it.
1695
1696 /// An inline comment on a patch, resolved or not.
1697 fn inline(resolved: bool) -> crate::state::InlineComment {
1698 crate::state::InlineComment {
1699 author: make_author(),
1700 file: "src/lib.rs".into(),
1701 line: 1,
1702 body: "look at this".into(),
1703 timestamp: "2026-01-01T00:00:00Z".into(),
1704 revision: Some(1),
1705 commit_id: Oid::zero(),
1706 edited: false,
1707 deleted: false,
1708 non_blocking: false,
1709 resolved: resolved.then(|| crate::state::Resolution {
1710 by: make_author(),
1711 revision: Some(1),
1712 timestamp: "2026-01-01T00:00:00Z".into(),
1713 commit_id: Oid::zero(),
1714 }),
1715 }
1716 }
1717
1718 /// Two open patches: one with two unanswered threads, one with none.
1719 fn app_with_unanswered_feedback() -> App {
1720 let mut noisy = make_patch("p1", "Needs attention", PatchStatus::Open);
1721 noisy.inline_comments = vec![inline(false), inline(false), inline(true)];
1722 let quiet = make_patch("p2", "Nothing outstanding", PatchStatus::Open);
1723 App::new(
1724 vec![],
1725 vec![noisy, quiet],
1726 Abbrev::minimal(),
1727 Abbrev::minimal(),
1728 )
1729 }
1730
1731 #[test]
1732 fn test_patch_list_shows_unresolved_count() {
1733 let mut app = app_with_unanswered_feedback();
1734 app.list_mode = ListMode::Patches;
1735
1736 let buf = render_app(&mut app);
1737 assert_buffer_contains(&buf, "!2");
1738 }
1739
1740 /// Before the title, not after it. The list pane is ~28 columns on an
1741 /// 80-column terminal, so a title of any real length is already being
1742 /// truncated — and a count rendered after it would be truncated with it,
1743 /// which is the same as not rendering it at all.
1744 #[test]
1745 fn test_unresolved_count_survives_a_truncated_title() {
1746 let mut noisy = make_patch(
1747 "p1",
1748 "A title long enough that the list pane has to cut it short",
1749 PatchStatus::Open,
1750 );
1751 noisy.inline_comments = vec![inline(false), inline(false)];
1752 let mut app = App::new(vec![], vec![noisy], Abbrev::minimal(), Abbrev::minimal());
1753 app.list_mode = ListMode::Patches;
1754
1755 let buf = render_app(&mut app);
1756 let text = buffer_to_string(&buf);
1757 // The row in the list pane, not the copy of the title in the detail
1758 // pane beside it.
1759 let row = text
1760 .lines()
1761 .find(|l| l.contains("> p1"))
1762 .unwrap_or_else(|| panic!("no patch row on screen:\n{}", text));
1763 assert!(
1764 !row.contains("has to cut it short"),
1765 "sanity: the title should be truncated in this pane:\n{}",
1766 row
1767 );
1768 assert!(
1769 row.contains("!2"),
1770 "the count must survive a title long enough to be truncated:\n{}",
1771 row
1772 );
1773 }
1774
1775 /// Zero renders as nothing, like `(N new)` on the CLI: a column of
1776 /// zeroes is noise the eye learns to skip.
1777 #[test]
1778 fn test_no_marker_when_nothing_is_outstanding() {
1779 let mut app = App::new(
1780 vec![],
1781 vec![make_patch("p2", "Nothing outstanding", PatchStatus::Open)],
1782 Abbrev::minimal(),
1783 Abbrev::minimal(),
1784 );
1785 app.list_mode = ListMode::Patches;
1786
1787 let buf = render_app(&mut app);
1788 let text = buffer_to_string(&buf);
1789 assert!(
1790 !text.contains('!'),
1791 "a patch with nothing outstanding carries no marker:\n{}",
1792 text
1793 );
1794 }
1795
1796 #[test]
1797 fn test_u_filters_to_patches_with_unanswered_feedback() {
1798 let mut app = app_with_unanswered_feedback();
1799 app.list_mode = ListMode::Patches;
1800 assert_eq!(app.visible_patches().len(), 2);
1801
1802 let action = app.handle_key(
1803 crossterm::event::KeyCode::Char('u'),
1804 crossterm::event::KeyModifiers::empty(),
1805 );
1806 assert_eq!(action, KeyAction::Continue);
1807
1808 let visible = app.visible_patches();
1809 assert_eq!(visible.len(), 1, "the filter must drop the quiet patch");
1810 assert_eq!(visible[0].title, "Needs attention");
1811
1812 // And it is a toggle, not a one-way door.
1813 app.handle_key(
1814 crossterm::event::KeyCode::Char('u'),
1815 crossterm::event::KeyModifiers::empty(),
1816 );
1817 assert_eq!(app.visible_patches().len(), 2);
1818 }
1819
1820 /// A second axis, not a fourth value of the status filter — so the two
1821 /// compose rather than replace one another, matching `--unresolved -a`
1822 /// on the CLI.
1823 #[test]
1824 fn test_unresolved_filter_composes_with_the_status_filter() {
1825 let mut closed = make_patch("p3", "Closed with feedback", PatchStatus::Closed);
1826 closed.inline_comments = vec![inline(false)];
1827 let mut open = make_patch("p1", "Open with feedback", PatchStatus::Open);
1828 open.inline_comments = vec![inline(false)];
1829 let mut app = App::new(
1830 vec![],
1831 vec![open, closed],
1832 Abbrev::minimal(),
1833 Abbrev::minimal(),
1834 );
1835 app.list_mode = ListMode::Patches;
1836 app.unresolved_only = true;
1837
1838 let visible = app.visible_patches();
1839 assert_eq!(visible.len(), 1);
1840 assert_eq!(visible[0].title, "Open with feedback");
1841
1842 app.status_filter = StatusFilter::All;
1843 assert_eq!(
1844 app.visible_patches().len(),
1845 2,
1846 "widening the status axis must widen the result"
1847 );
1848 }
1849
1850 /// The filter changes what the list holds, so it has to say so on the
1851 /// pane it changed — a filtered list that looks unfiltered is the bug
1852 /// this dashboard's staleness banner exists to avoid elsewhere.
1853 #[test]
1854 fn test_pane_title_admits_the_unresolved_filter() {
1855 let mut app = app_with_unanswered_feedback();
1856 app.list_mode = ListMode::Patches;
1857 app.unresolved_only = true;
1858
1859 let buf = render_app(&mut app);
1860 assert_buffer_contains(&buf, "unresolved");
1861 }
1862
1863 /// The key has to be discoverable, and the footer is where every other
1864 /// key on this pane is advertised.
1865 #[test]
1866 fn test_footer_offers_the_unresolved_filter_on_the_patch_pane() {
1867 let mut app = app_with_unanswered_feedback();
1868 app.list_mode = ListMode::Patches;
1869
1870 let buf = render_app(&mut app);
1871 assert_buffer_contains(&buf, "u:unresolved");
1872 }
1873
1874 /// `u` belongs to the patch pane. On the issue pane there is no such
1875 /// count, so it must neither act nor be advertised.
1876 #[test]
1877 fn test_u_is_inert_on_the_issue_pane() {
1878 let mut app = make_app(3, 0);
1879 app.list_mode = ListMode::Issues;
1880
1881 app.handle_key(
1882 crossterm::event::KeyCode::Char('u'),
1883 crossterm::event::KeyModifiers::empty(),
1884 );
1885 assert!(
1886 !app.unresolved_only,
1887 "`u` must not filter a list that has no such count"
1888 );
1889
1890 let buf = render_app(&mut app);
1891 let text = buffer_to_string(&buf);
1892 assert!(
1893 !text.contains("u:unresolved"),
1894 "the issue pane must not advertise a patch-only key:\n{}",
1895 text
1896 );
1897 }
1684 } 1898 }
src/tui/state.rs
Old New
@@ -106,6 +106,17 @@ pub(crate) struct App {
106 pub(crate) pane: Pane, 106 pub(crate) pane: Pane,
107 pub(crate) mode: ViewMode, 107 pub(crate) mode: ViewMode,
108 pub(crate) status_filter: StatusFilter, 108 pub(crate) status_filter: StatusFilter,
109 /// Show only patches with unanswered inline feedback.
110 ///
111 /// A second axis rather than a fourth value of `status_filter`, so the two
112 /// compose: on its own it narrows whatever the status filter admits, which
113 /// is what `--unresolved -a` does on the CLI. Folding it into the status
114 /// cycle would make "closed patches with unanswered feedback" unreachable
115 /// and put a `u` press three `a` presses away.
116 ///
117 /// Patches only. Issues carry no such count, so `u` is inert on that pane
118 /// and the footer does not advertise it there.
119 pub(crate) unresolved_only: bool,
109 pub(crate) search_query: String, 120 pub(crate) search_query: String,
110 pub(crate) input_mode: InputMode, 121 pub(crate) input_mode: InputMode,
111 pub(crate) input_buf: String, 122 pub(crate) input_buf: String,
@@ -171,6 +182,7 @@ impl App {
171 pane: Pane::ItemList, 182 pane: Pane::ItemList,
172 mode: ViewMode::Details, 183 mode: ViewMode::Details,
173 status_filter: StatusFilter::Open, 184 status_filter: StatusFilter::Open,
185 unresolved_only: false,
174 search_query: String::new(), 186 search_query: String::new(),
175 input_mode: InputMode::Normal, 187 input_mode: InputMode::Normal,
176 input_buf: String::new(), 188 input_buf: String::new(),
@@ -295,6 +307,7 @@ impl App {
295 } 307 }
296 StatusFilter::All => true, 308 StatusFilter::All => true,
297 }) 309 })
310 .filter(|p| !self.unresolved_only || p.unresolved_comments() > 0)
298 .filter(|p| self.matches_search(&p.title)) 311 .filter(|p| self.matches_search(&p.title))
299 .collect() 312 .collect()
300 } 313 }
@@ -568,6 +581,17 @@ impl App {
568 state.select(if count > 0 { Some(0) } else { None }); 581 state.select(if count > 0 { Some(0) } else { None });
569 KeyAction::Continue 582 KeyAction::Continue
570 } 583 }
584 // Patches only: issues carry no unresolved count, so on that pane
585 // this would silently do nothing to a list the user is looking at.
586 KeyCode::Char('u') if self.list_mode == ListMode::Patches => {
587 self.unresolved_only = !self.unresolved_only;
588 // The row under the cursor may have just left the list; the
589 // status filter resets the selection for the same reason.
590 let count = self.visible_count();
591 self.patch_list_state
592 .select(if count > 0 { Some(0) } else { None });
593 KeyAction::Continue
594 }
571 KeyCode::Char('r') => KeyAction::Reload, 595 KeyCode::Char('r') => KeyAction::Reload,
572 _ => KeyAction::Continue, 596 _ => KeyAction::Continue,
573 } 597 }
src/tui/widgets.rs
Old New
@@ -368,17 +368,38 @@ fn render_list(frame: &mut Frame, app: &mut App, area: Rect) {
368 PatchStatus::Closed => Style::default().fg(Color::Red), 368 PatchStatus::Closed => Style::default().fg(Color::Red),
369 PatchStatus::Merged => Style::default().fg(Color::Cyan), 369 PatchStatus::Merged => Style::default().fg(Color::Cyan),
370 }; 370 };
371 ListItem::new(format!( 371 // Between the status and the title, not after the title.
372 "{} {:6} {}", 372 // This pane is ~28 columns on an 80-column terminal, so a
373 app.patch_abbrev.of(&p.id), 373 // suffix would be truncated away on every real repository
374 status, 374 // — which is the same as not rendering it. The number is
375 p.title 375 // the one `patch list` and the web list print; only the
376 )) 376 // notation is compressed to fit.
377 .style(style) 377 let unresolved = p.unresolved_comments();
378 let mut spans = vec![Span::styled(
379 format!("{} {:6} ", app.patch_abbrev.of(&p.id), status),
380 style,
381 )];
382 if unresolved > 0 {
383 spans.push(Span::styled(
384 format!("!{} ", unresolved),
385 Style::default()
386 .fg(Color::Yellow)
387 .add_modifier(Modifier::BOLD),
388 ));
389 }
390 spans.push(Span::styled(p.title.clone(), style));
391 ListItem::new(Line::from(spans))
378 }) 392 })
379 .collect(); 393 .collect();
380 394
381 let title = format!("Patches ({})", app.status_filter.label()); 395 // The pane says what it is hiding. A filtered list that looks
396 // unfiltered is the same failure the staleness banner exists to
397 // prevent elsewhere on this screen.
398 let title = if app.unresolved_only {
399 format!("Patches ({}, unresolved)", app.status_filter.label())
400 } else {
401 format!("Patches ({})", app.status_filter.label())
402 };
382 403
383 let list = List::new(items) 404 let list = List::new(items)
384 .block( 405 .block(
@@ -1161,7 +1182,9 @@ fn render_footer(frame: &mut Frame, app: &App, area: Rect) {
1161 }; 1182 };
1162 let mode_hint = match app.list_mode { 1183 let mode_hint = match app.list_mode {
1163 ListMode::Issues => " c:events p:patch", 1184 ListMode::Issues => " c:events p:patch",
1164 ListMode::Patches => " Enter:view patch", 1185 // Advertised only where it does something: issues carry no
1186 // unresolved count, so `u` is inert on that pane.
1187 ListMode::Patches => " Enter:view patch u:unresolved",
1165 }; 1188 };
1166 format!( 1189 format!(
1167 " j/k:navigate Tab:pane {} {}{} /:search r:refresh q:quit", 1190 " j/k:navigate Tab:pane {} {}{} /:search r:refresh q:quit",
tests/collab_test.rs
Old New
@@ -1345,6 +1345,7 @@ fn capture_patch_list(
1345 offset, 1345 offset,
1346 git_collab::cli::SortMode::Recent, 1346 git_collab::cli::SortMode::Recent,
1347 &[], 1347 &[],
1348 git_collab::patch::ListOpts::default(),
1348 &mut buf, 1349 &mut buf,
1349 ) 1350 )
1350 .unwrap(); 1351 .unwrap();
@@ -1651,6 +1652,7 @@ fn test_patch_list_json_output() {
1651 false, 1652 false,
1652 git_collab::cli::SortMode::Recent, 1653 git_collab::cli::SortMode::Recent,
1653 &[], 1654 &[],
1655 git_collab::patch::ListOpts::default(),
1654 ) 1656 )
1655 .unwrap(); 1657 .unwrap();
1656 let value: serde_json::Value = serde_json::from_str(&json_str).unwrap(); 1658 let value: serde_json::Value = serde_json::from_str(&json_str).unwrap();
tests/sort_test.rs
Old New
@@ -230,8 +230,17 @@ fn test_patch_default_sort_by_recency() {
230 // Patch B: created later, never updated 230 // Patch B: created later, never updated
231 let (_, _) = create_patch_at(&repo, &alice(), "Beta patch", "2025-06-01T00:00:00Z"); 231 let (_, _) = create_patch_at(&repo, &alice(), "Beta patch", "2025-06-01T00:00:00Z");
232 232
233 let entries = 233 let entries = git_collab::patch::list(
234 git_collab::patch::list(&repo, true, false, None, None, SortMode::Recent, &[]).unwrap(); 234 &repo,
235 true,
236 false,
237 None,
238 None,
239 SortMode::Recent,
240 &[],
241 git_collab::patch::ListOpts::default(),
242 )
243 .unwrap();
235 assert_eq!(entries.len(), 2); 244 assert_eq!(entries.len(), 2);
236 assert_eq!(entries[0].patch.title, "Alpha patch"); 245 assert_eq!(entries[0].patch.title, "Alpha patch");
237 assert_eq!(entries[1].patch.title, "Beta patch"); 246 assert_eq!(entries[1].patch.title, "Beta patch");
@@ -253,8 +262,17 @@ fn test_patch_sort_by_created() {
253 262
254 let (_, _) = create_patch_at(&repo, &alice(), "Beta patch", "2025-06-01T00:00:00Z"); 263 let (_, _) = create_patch_at(&repo, &alice(), "Beta patch", "2025-06-01T00:00:00Z");
255 264
256 let entries = 265 let entries = git_collab::patch::list(
257 git_collab::patch::list(&repo, true, false, None, None, SortMode::Created, &[]).unwrap(); 266 &repo,
267 true,
268 false,
269 None,
270 None,
271 SortMode::Created,
272 &[],
273 git_collab::patch::ListOpts::default(),
274 )
275 .unwrap();
258 assert_eq!(entries.len(), 2); 276 assert_eq!(entries.len(), 2);
259 assert_eq!(entries[0].patch.title, "Beta patch"); 277 assert_eq!(entries[0].patch.title, "Beta patch");
260 assert_eq!(entries[1].patch.title, "Alpha patch"); 278 assert_eq!(entries[1].patch.title, "Alpha patch");
@@ -269,7 +287,17 @@ fn test_patch_sort_alpha() {
269 create_patch_at(&repo, &alice(), "Apple patch", "2025-06-01T00:00:00Z"); 287 create_patch_at(&repo, &alice(), "Apple patch", "2025-06-01T00:00:00Z");
270 create_patch_at(&repo, &alice(), "Mango patch", "2025-03-01T00:00:00Z"); 288 create_patch_at(&repo, &alice(), "Mango patch", "2025-03-01T00:00:00Z");
271 289
272 let entries = git_collab::patch::list(&repo, true, false, None, None, SortMode::Alpha, &[]).unwrap(); 290 let entries = git_collab::patch::list(
291 &repo,
292 true,
293 false,
294 None,
295 None,
296 SortMode::Alpha,
297 &[],
298 git_collab::patch::ListOpts::default(),
299 )
300 .unwrap();
273 assert_eq!(entries.len(), 3); 301 assert_eq!(entries.len(), 3);
274 assert_eq!(entries[0].patch.title, "Apple patch"); 302 assert_eq!(entries[0].patch.title, "Apple patch");
275 assert_eq!(entries[1].patch.title, "Mango patch"); 303 assert_eq!(entries[1].patch.title, "Mango patch");
tests/unresolved_counts_test.rs
Old New
@@ -0,0 +1,478 @@
1 //! Surfacing *how much* feedback is still unanswered, in the views a reviewer
2 //! scans rather than the one they open.
3 //!
4 //! `28fc6751` made a single thread's answered-ness visible on `patch show`.
5 //! That answers "is this thread done" one patch at a time, which is the wrong
6 //! shape for the question a reviewer actually starts with: *which* patches
7 //! have unanswered feedback. Answering that by opening every patch is the tax
8 //! this removes.
9 //!
10 //! The count is one number with one definition, computed once from the folded
11 //! `PatchState` and rendered by `patch list`, the dashboard and the web list.
12 //! Three properties are load-bearing and each is pinned here:
13 //!
14 //! * **The definition.** An unresolved comment is an inline comment carrying
15 //! no standing resolution and not withdrawn. The two edge cases that forced
16 //! a decision — a *stale* claim and a *tombstoned* comment — are tested
17 //! directly, because a count cannot carry the caveat `patch show` prints.
18 //! * **Agreement.** The CLI, the JSON and the web list report the same number
19 //! for the same repository. A number that means one thing in one view and
20 //! something else in another is worse than no number.
21 //! * **Reading is not writing.** Rendering a list must not append events or
22 //! move refs. This project has a history of exactly that bug.
23
24 mod common;
25
26 use common::{ServerHarness, TestRepo};
27
28 // ===========================================================================
29 // Helpers
30 // ===========================================================================
31
32 /// A patch over `feature.txt`, left checked out on `main`. Returns the
33 /// abbreviated id `patch create` prints, which is what a user would then type;
34 /// the full id only ever appears in `--json`.
35 fn patch_over_a_file(repo: &TestRepo, title: &str, branch: &str) -> String {
36 repo.git(&["checkout", "-b", branch]);
37 repo.commit_file("feature.txt", "v1\n", &format!("commit for {}", title));
38 let out = repo.run_ok(&["patch", "create", "-t", title, "-B", branch]);
39 repo.git(&["checkout", "main"]);
40 out.trim()
41 .strip_prefix("Created patch ")
42 .unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
43 .to_string()
44 }
45
46 /// Leave an inline comment on `feature.txt:1` and return its full id.
47 fn inline_comment(repo: &TestRepo, id: &str, body: &str) -> String {
48 repo.run_ok(&[
49 "patch",
50 "comment",
51 id,
52 "--file",
53 "feature.txt",
54 "--line",
55 "1",
56 "-b",
57 body,
58 ]);
59 let json = repo.run_ok(&["patch", "show", id, "--json"]);
60 let value: serde_json::Value = serde_json::from_str(&json).expect("valid JSON");
61 let comments = value["inline_comments"]
62 .as_array()
63 .expect("inline_comments is an array");
64 comments
65 .last()
66 .expect("at least one inline comment")
67 .get("commit_id")
68 .and_then(|v| v.as_str())
69 .expect("commit_id on the inline comment")
70 .to_string()
71 }
72
73 /// The `unresolved_comments` field `patch list --json` reports for `id`.
74 fn json_count(repo: &TestRepo, id: &str) -> u64 {
75 let json = repo.run_ok(&["patch", "list", "--json", "-a", "--archived"]);
76 let value: serde_json::Value = serde_json::from_str(&json).expect("valid JSON");
77 let entry = value
78 .as_array()
79 .expect("a JSON array")
80 .iter()
81 .find(|p| p["id"].as_str().is_some_and(|full| full.starts_with(id)))
82 .unwrap_or_else(|| panic!("patch {} missing from `patch list --json`: {}", id, json));
83 entry
84 .get("unresolved_comments")
85 .unwrap_or_else(|| {
86 panic!(
87 "`patch list --json` carries no `unresolved_comments` key: {}",
88 json
89 )
90 })
91 .as_u64()
92 .unwrap_or_else(|| panic!("`unresolved_comments` is not a number: {}", json))
93 }
94
95 /// The row `patch list` prints for `id`.
96 fn list_row(repo: &TestRepo, id: &str) -> String {
97 let listing = repo.run_ok(&["patch", "list", "-a", "--archived"]);
98 listing
99 .lines()
100 .find(|l| l.split_whitespace().next().is_some_and(|first| first == id))
101 .unwrap_or_else(|| panic!("patch {} missing from `patch list`:\n{}", id, listing))
102 .to_string()
103 }
104
105 // ===========================================================================
106 // The count, on the CLI
107 // ===========================================================================
108
109 #[test]
110 fn patch_list_says_how_much_feedback_is_still_unanswered() {
111 let repo = TestRepo::new("Alice", "alice@example.com");
112 let id = patch_over_a_file(&repo, "needs answers", "feat-count");
113 let first = inline_comment(&repo, &id, "this needs a guard");
114 inline_comment(&repo, &id, "and this name is wrong");
115
116 let row = list_row(&repo, &id);
117 assert!(
118 row.contains("2 unresolved"),
119 "`patch list` does not say how much feedback is outstanding:\n{}",
120 row
121 );
122
123 repo.run_ok(&["patch", "resolve", &id, &first[..8]]);
124 let row = list_row(&repo, &id);
125 assert!(
126 row.contains("1 unresolved"),
127 "answering a thread did not move the count:\n{}",
128 row
129 );
130 }
131
132 /// Zero is not printed, for the same reason `(N new)` is not: a list is
133 /// scanned, and a column of `(0 unresolved)` is noise that trains the eye to
134 /// skip the very thing the column exists to show. `--json` still carries the
135 /// zero; see below.
136 #[test]
137 fn a_patch_with_nothing_outstanding_says_nothing() {
138 let repo = TestRepo::new("Alice", "alice@example.com");
139 let id = patch_over_a_file(&repo, "all answered", "feat-quiet");
140 let comment = inline_comment(&repo, &id, "one thing");
141 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
142
143 let row = list_row(&repo, &id);
144 assert!(
145 !row.contains("unresolved"),
146 "a fully answered patch should not carry a count:\n{}",
147 row
148 );
149 }
150
151 /// The scripted surface has the opposite requirement to the text one. A caller
152 /// parsing JSON cannot tell an absent key from a zero, so the key is always
153 /// present — the same rule `resolved` and `non_blocking` already follow.
154 #[test]
155 fn json_always_carries_the_count_even_when_it_is_zero() {
156 let repo = TestRepo::new("Alice", "alice@example.com");
157 let id = patch_over_a_file(&repo, "scripted", "feat-json");
158 assert_eq!(
159 json_count(&repo, &id),
160 0,
161 "a patch with no feedback must report 0, not omit the key"
162 );
163
164 let comment = inline_comment(&repo, &id, "one thing");
165 assert_eq!(json_count(&repo, &id), 1);
166 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
167 assert_eq!(json_count(&repo, &id), 0);
168 }
169
170 /// `--json` grew a field; it did not lose one. The ids stay full.
171 #[test]
172 fn json_still_reports_full_ids_beside_the_new_field() {
173 let repo = TestRepo::new("Alice", "alice@example.com");
174 let id = patch_over_a_file(&repo, "ids intact", "feat-ids");
175 inline_comment(&repo, &id, "something");
176
177 let json = repo.run_ok(&["patch", "list", "--json", "-a", "--archived"]);
178 let value: serde_json::Value = serde_json::from_str(&json).expect("valid JSON");
179 let entry = &value.as_array().expect("array")[0];
180 let full = entry["id"].as_str().expect("an id");
181 assert_eq!(
182 full.len(),
183 40,
184 "`--json` must carry the full patch id, not the abbreviation the \
185 prose surface prints: {}",
186 json
187 );
188 assert!(
189 full.starts_with(&id),
190 "the id in `--json` is not the patch that was created: {}",
191 json
192 );
193 assert!(
194 entry["inline_comments"][0]["commit_id"]
195 .as_str()
196 .is_some_and(|s| s.len() == 40),
197 "inline comment ids must stay full: {}",
198 json
199 );
200 }
201
202 // ===========================================================================
203 // The two decisions a count forces
204 // ===========================================================================
205
206 /// **A stale claim counts as answered.**
207 ///
208 /// `patch show` prints a resolution older than the tip as
209 /// `resolved by Alice at r1 — 2 revisions landed since`: a hint beside the
210 /// thread, deliberately never a state change. A count cannot carry that
211 /// caveat, so it has to pick a side, and counting the claim as *unanswered*
212 /// would make in every list view precisely the state change `28fc6751`
213 /// refused to make.
214 ///
215 /// It would also be a number nobody could act on. Staleness is not a property
216 /// of the thread but of the pair (claim revision, current tip), so a revision
217 /// landing on an unrelated file would silently re-raise feedback that nobody
218 /// touched — and the only way to drive the count back down would be to
219 /// re-resolve every thread on every revision. That converts "unanswered
220 /// feedback" into "feedback not re-confirmed against the tip", which is a
221 /// different and much noisier question than the one a review queue asks.
222 ///
223 /// So the count counts recorded facts, and the caveat stays where it can be
224 /// spelled out. This test pins both halves: the count says answered, and
225 /// `patch show` still says how old the claim is.
226 #[test]
227 fn a_claim_older_than_the_tip_still_counts_as_answered() {
228 let repo = TestRepo::new("Alice", "alice@example.com");
229 repo.git(&["checkout", "-b", "feat-stale"]);
230 repo.commit_file("feature.txt", "v1\n", "feature v1");
231 let out = repo.run_ok(&["patch", "create", "-t", "moving target", "-B", "feat-stale"]);
232 let id = out
233 .trim()
234 .strip_prefix("Created patch ")
235 .unwrap()
236 .to_string();
237
238 let comment = inline_comment(&repo, &id, "fix this");
239 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
240
241 // Two more revisions land after the claim was made.
242 for v in ["v2", "v3"] {
243 repo.commit_file("feature.txt", &format!("{}\n", v), &format!("rev {}", v));
244 repo.run_ok(&["patch", "revise", &id]);
245 }
246
247 assert_eq!(
248 json_count(&repo, &id),
249 0,
250 "a revision landing elsewhere must not silently re-raise a thread \
251 nobody reopened"
252 );
253 let row = list_row(&repo, &id);
254 assert!(
255 !row.contains("unresolved"),
256 "the list disagrees with the recorded resolution:\n{}",
257 row
258 );
259
260 // And the caveat the count cannot carry is still where it can be read.
261 let show = repo.run_ok(&["patch", "show", &id]);
262 assert!(
263 show.contains("r1") && show.to_lowercase().contains("since"),
264 "the staleness hint must survive on the surface that has room for it:\n{}",
265 show
266 );
267 }
268
269 /// **A withdrawn comment is not outstanding feedback.**
270 ///
271 /// Deleting an inline comment leaves a tombstone: the words are gone and there
272 /// is nothing left to answer. Counting it would send a reviewer to a patch to
273 /// read `[deleted]`, and the only way to clear the row would be to "resolve" a
274 /// comment that no longer says anything. `patch show` still prints the
275 /// tombstone, so a reader who opens the patch sees at once why the number is
276 /// what it is.
277 #[test]
278 fn a_withdrawn_comment_is_not_outstanding_feedback() {
279 let repo = TestRepo::new("Alice", "alice@example.com");
280 let id = patch_over_a_file(&repo, "withdrawn", "feat-tombstone");
281 let comment = inline_comment(&repo, &id, "actually never mind");
282 assert_eq!(json_count(&repo, &id), 1);
283
284 repo.run_ok(&["patch", "delete-comment", &id, &comment[..8]]);
285 assert_eq!(
286 json_count(&repo, &id),
287 0,
288 "a tombstone carries no words to answer, so it is not outstanding"
289 );
290 }
291
292 /// A reopened thread is outstanding again — the count follows the fold, not
293 /// the first event it saw.
294 #[test]
295 fn reopening_a_thread_puts_it_back_in_the_count() {
296 let repo = TestRepo::new("Alice", "alice@example.com");
297 let id = patch_over_a_file(&repo, "reopened", "feat-reopen");
298 let comment = inline_comment(&repo, &id, "not actually fixed");
299 repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
300 assert_eq!(json_count(&repo, &id), 0);
301
302 repo.run_ok(&["patch", "unresolve", &id, &comment[..8]]);
303 assert_eq!(
304 json_count(&repo, &id),
305 1,
306 "withdrawing a resolution must put the thread back in the count"
307 );
308 }
309
310 // ===========================================================================
311 // Filtering
312 // ===========================================================================
313
314 #[test]
315 fn patch_list_can_narrow_to_patches_with_unanswered_feedback() {
316 let repo = TestRepo::new("Alice", "alice@example.com");
317 patch_over_a_file(&repo, "nothing outstanding", "feat-a");
318 let noisy = patch_over_a_file(&repo, "needs attention", "feat-b");
319 inline_comment(&repo, &noisy, "look at this");
320
321 let listing = repo.run_ok(&["patch", "list", "--unresolved"]);
322 assert!(
323 listing.contains("needs attention"),
324 "the filter dropped a patch with unanswered feedback:\n{}",
325 listing
326 );
327 assert!(
328 !listing.contains("nothing outstanding"),
329 "the filter kept a patch with nothing outstanding:\n{}",
330 listing
331 );
332 }
333
334 /// The filter is a flag, not a fourth value of the status filter, so it
335 /// composes with the axis that already exists rather than replacing it.
336 #[test]
337 fn the_unresolved_filter_composes_with_the_status_filter() {
338 let repo = TestRepo::new("Alice", "alice@example.com");
339 let open = patch_over_a_file(&repo, "open with feedback", "feat-open");
340 inline_comment(&repo, &open, "a thing");
341 let closed = patch_over_a_file(&repo, "closed with feedback", "feat-closed");
342 inline_comment(&repo, &closed, "another thing");
343 repo.run_ok(&["patch", "close", &closed]);
344
345 let default = repo.run_ok(&["patch", "list", "--unresolved"]);
346 assert!(default.contains("open with feedback"));
347 assert!(
348 !default.contains("closed with feedback"),
349 "`--unresolved` alone must still mean open patches only:\n{}",
350 default
351 );
352
353 let all = repo.run_ok(&["patch", "list", "--unresolved", "-a", "--archived"]);
354 assert!(
355 all.contains("closed with feedback"),
356 "`--unresolved -a` must widen to closed patches too:\n{}",
357 all
358 );
359 }
360
361 /// An empty result says which filter emptied it, rather than claiming the
362 /// repository has no patches — the same rule the web list already follows.
363 #[test]
364 fn an_empty_unresolved_list_names_the_filter() {
365 let repo = TestRepo::new("Alice", "alice@example.com");
366 patch_over_a_file(&repo, "nothing outstanding", "feat-empty");
367
368 let listing = repo.run_ok(&["patch", "list", "--unresolved"]);
369 assert!(
370 listing.to_lowercase().contains("unanswered")
371 || listing.to_lowercase().contains("unresolved"),
372 "an empty `--unresolved` list must say what it was filtering for:\n{}",
373 listing
374 );
375 }
376
377 // ===========================================================================
378 // The web list agrees
379 // ===========================================================================
380
381 #[test]
382 fn the_web_patch_list_reports_the_same_count_as_the_cli() {
383 let harness = ServerHarness::new("unresolved-web-count");
384 let repo = harness.work_repo();
385 let id = patch_over_a_file(repo, "web counted", "feat-web");
386 inline_comment(repo, &id, "first thing");
387 inline_comment(repo, &id, "second thing");
388 let third = inline_comment(repo, &id, "third thing");
389 repo.run_ok(&["patch", "resolve", &id, &third[..8]]);
390
391 assert_eq!(json_count(repo, &id), 2, "sanity: the CLI count");
392
393 harness.push_head();
394 harness.push_collab_refs();
395
396 let body = harness
397 .get_ok(&format!("/{}/patches", harness.repo_name()))
398 .body;
399 assert!(
400 body.contains("<th>Unresolved</th>"),
401 "the web patch list has no Unresolved column:\n{body}"
402 );
403 assert!(
404 body.contains(">2<"),
405 "the web patch list does not report the count the CLI reports:\n{body}"
406 );
407 }
408
409 #[test]
410 fn the_web_patch_list_can_narrow_to_patches_with_unanswered_feedback() {
411 let harness = ServerHarness::new("unresolved-web-filter");
412 let repo = harness.work_repo();
413 patch_over_a_file(repo, "Nothing outstanding here", "feat-web-quiet");
414 let noisy = patch_over_a_file(repo, "Needs attention here", "feat-web-noisy");
415 inline_comment(repo, &noisy, "look at this");
416
417 harness.push_head();
418 harness.push_collab_refs();
419
420 let body = harness
421 .get_ok(&format!(
422 "/{}/patches?filter=unresolved",
423 harness.repo_name()
424 ))
425 .body;
426 assert!(
427 body.contains("Needs attention here"),
428 "the web filter dropped a patch with unanswered feedback:\n{body}"
429 );
430 assert!(
431 !body.contains("Nothing outstanding here"),
432 "the web filter kept a patch with nothing outstanding:\n{body}"
433 );
434 assert!(
435 body.contains("patches?filter=unresolved"),
436 "the filter bar does not offer the unresolved filter:\n{body}"
437 );
438 }
439
440 // ===========================================================================
441 // Reading is not writing
442 // ===========================================================================
443
444 /// Counting is a fold over state already in hand. It must not append an event
445 /// or move a ref — not on the CLI, not through the server.
446 #[test]
447 fn counting_unanswered_feedback_writes_nothing() {
448 let harness = ServerHarness::new("unresolved-readonly");
449 let repo = harness.work_repo();
450 let id = patch_over_a_file(repo, "read only", "feat-readonly");
451 inline_comment(repo, &id, "a thing");
452 harness.push_head();
453 harness.push_collab_refs();
454
455 let snapshot = |r: &TestRepo| {
456 r.git(&[
457 "for-each-ref",
458 "--format=%(refname) %(objectname)",
459 "refs/collab",
460 ])
461 };
462
463 let before = snapshot(repo);
464 repo.run_ok(&["patch", "list"]);
465 repo.run_ok(&["patch", "list", "--unresolved"]);
466 repo.run_ok(&["patch", "list", "--json", "-a", "--archived"]);
467 harness.get_ok(&format!("/{}/patches", harness.repo_name()));
468 harness.get_ok(&format!(
469 "/{}/patches?filter=unresolved",
470 harness.repo_name()
471 ));
472 let after = snapshot(repo);
473
474 assert_eq!(
475 before, after,
476 "rendering a list must not append events or move refs"
477 );
478 }