Fix incorrect viewed files counter if reverted change was viewed

[bytedream]

If a file is marked as viewed in a PR and all changes to those file are reverted afterwards, the file is still stored as viewed in the db, which causes an incorrect viewed files counter

---

[lunny]

Could we have a test for that?

[wxiaoguang]

changedFiles needs to be a map now

[bytedream]

I've reverted to keep the old logic - I haven't thought about the performance bottleneck here. Done

[bytedream]

Could we have a test for that?

Test added

[lunny]

Could we have a test for that?

Test added

It's better to use fast-import to import the fixture git repository.

[bytedream]

I've added the test repo changes as fast-import, but I'm still using a test repo as base, as the used pull request fixture depends on it

[silverwind]

Comment authored by Claude on behalf of @silverwind

Review of #36819 — Fix incorrect viewed files counter if reverted change was viewed

The fix is correct in concept — leftover changedFiles entries after the main loop represent files that changed between the review commit and HEAD but are no longer in the diff (i.e. reverted). Marking these as Unviewed properly fixes the counter.

Two points:

1. Outdated comment should be updated. Line 1511 currently says:

   On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed.

   This PR addresses exactly that case, so the comment is now misleading and should be removed or reworded.

2. Consider a map for changedFiles instead of slice swap-and-shrink. wxiaoguang suggested this earlier. Converting changedFiles to a map[string]struct{} upfront would:

   • Reduce the inner loop from O(n*m) to O(n+m)
   • Eliminate the swap-and-shrink pattern which, while correct here (the continue outer saves it), is tricky to reason about inside a range loop
   • Make the "leftover files" logic at the end simpler — just iterate the map

   Something like:

   changedFileSet := make(map[string]struct{}, len(changedFiles))
   for _, f := range changedFiles {
       changedFileSet[f] = struct{}{}
   }
   // ... in the loop: _, changed := changedFileSet[filename]; delete(changedFileSet, filename)
   // ... after the loop: iterate changedFileSet for leftovers

The test is well-structured and validates the scenario. Overall looks good pending the above.

[bytedream]

Addressed the ai suggestions

[silverwind]

This review was written by Claude.

[silverwind]

Not sure about backport, likely won't merge cleanly and it's better to focus on the 1.26 release.

[silverwind]

Cancelled the backport PR.