search

codeapprove / feedback

Feedback for CodeApprove
0 stars 0 forks source link

Diff view appears out of chronolgical order #88

Open mtlynch opened 1 year ago

mtlynch commented 1 year ago

PR: https://github.com/tiny-pilot/ansible-role-ustreamer/pull/89

On this PR, the order of events was:

  1. jdeanwallace gave me round 1 of notes (2023-01-05 9:37 AM ET)
  2. I updated the PR and published responses to those notes (2023-01-05 3:46 PM ET)
  3. jdeanwallace gave me round 2 of notes (2023-01-06 9:54 AM ET)

Expected

CodeApprove orders the options as:

  1. "Since review by jdeanwallace 9:54 AM"
  2. "Since my last review Jan 05, 3:46 PM"
  3. "Since review by jdeanwallace Jan 05 9:37 AM"

Actual

CodeApprove orders the options out of chronological order:

  1. "Since my last review Jan 05, 3:46 PM"
  2. "Since review by jdeanwallace 9:54 AM"
  3. "Since review by jdeanwallace Jan 05 9:37 AM"

image

samatcodeapprove commented 1 year ago

@mtlynch ah thanks for filing this. This is currently intended behavior but I can see how it feels like a bug. Right now the first option is always "Since my last review" and then below that are all other reviews. I do this because I think that's the option most people are looking for and it could easily be buried under other reviewers in a multi-review situation.

I'll think about this some more and see if there's a way to make this more intuitive

mtlynch commented 1 year ago

"Since my last review" is a good first option if there have been changes since my last review.

The view I wanted to see was the diff of the last logical round (base: "review by jdeanwallace Jan 05 9:37 AM" vs head: "review by jdeanwallace 9:54 AM").

In this case, the first two options in CodeApprove's dropdown were empty diffs because I hadn't pushed any changes yet.