CodeApprove loses comments if new commits arrive

[mtlynch]

I realized CodeApprove seems to lose comments if they're not submitted before the underlying PR changes.

To reproduce

1. Developer submits PR with commit A that adds new file foo.txt
2. Reviewer comments on line 5 of foo.txt but does not send out draft
3. While Reviewer is still reviewing, Developer pushes commit B to the same PR, which updates line 5 of foo.txt

Current behavior

CodeApprove hides the draft comment that was on line 5 of foo.txt

Expected behavior

CodeApprove preserves the view of the PR that the Reviewer started with so that they can finish their review.

Screenshots

I created this PR and then made some draft comments

Before submitting my draft comments, I submitted another commit to the PR to change the lines where there were pending comments. This caused my "This is good" comment on line 7 to disappear:

There's a note that says "Warning: some comments in this file are on non-loaded lines" but clicking "Reveal All" has no effect. I've tried adjusting the base and head commits, but I still can't see the comments.

If I try to send the draft, it reports 4 pending comments, but only 3 are visible:

After I send it, the comment becomes visible again, but I can't find any way of viewing it in the full code context:

[samatcodeapprove]

@mtlynch thank you for the super clear feedback! I actually am not sure what the correct behavior is here. "Outdated" is correct as you commented on a line/diff which is no longer there.

But I could detect the change while you're editing and make the user do something ... hard to say if that's more painful.

[mtlynch]

The major pain point of this bug is that it puts the comments into a state where the reviewer can't see, edit, or delete them until they publish the full set of comments.

In my case, I had comments I hadn't finished writing that I didn't want to share, but I had to share them anyway because CodeApprove has no option to fix them.

A quick 'n dirty fix might be to show the outdated comments in draft state in the bottom section of all comments even if they're not visible inline.

Beyond that, I'd prefer that if the reviewer starts a review comparing commit A to commit B, the reviewer should be able to finish that review, no matter what commits hit the PR in the meantime. The reviewee should be able to see the review as the reviewer saw it after they publish. I think this is what Gerrit and Reviewable do.

[mtlynch]

@samatcodeapprove - Any updates on this one? This is a pretty bad bug, and it just bit me again. I wrote a long comment, switched tabs, then came back to find it gone. I thought I might have closed the tab and forgotten to save, so rewrote the comment from memory. When I submitted, I saw that it submitted both versions of the same comment. But until the reviewer releases all their notes, some comments are stuck in a hidden state where the reviewer can't see or edit their own note until they reveal it to the author.

[samatcodeapprove]

@mtlynch I am not sure why this happens to you if you switch tabs, it should only happen if you refresh the page with draft comments after a new commit has come in.

Still, losing comments while drafting is really bad no matter when it happens! So I am making two changes:

• (major change) showing draft comments in the Comments section at the bottom of the review
• (minor change) showing a warning when you get into this situation

Here's what the warning would look like:

And here's an outdated draft:

This should be live in ~30 minutes.

[samatcodeapprove]

@mtlynch note that when I have a PR page open when a new push comes in, I see this warning:

Do you not see this?

[mtlynch]

I am not sure why this happens to you if you switch tabs, it should only happen if you refresh the page with draft comments after a new commit has come in.

Oh, sorry, to clarify, I meant that I switched tabs, lost track of where the tab was, then had to reopen the PR in a new tab. I didn't switch tabs, then switch back to find my CodeApprove view had changed.

I have a PR page open when a new push comes in, I see this warning: Do you not see this?

Yep, that shows up for me, but it doesn't do much to prevent the disappearing comments.

To me, the warning isn't communicating, "You're about to lose data, so preserve your comments somewhere else before reloading." I always interpret it as, "You're not seeing the latest commits, so the obvious thing to do is reload."

Is that popup trying to communicate danger to the user?

So I am making two changes:

Great, those sound good to me!