Fix algorithmic sort order of unresolved discussions
For #344133 (closed)
Context
We have issues knowing exactly what file is "next" (or "previous", for that matter) when processing through Diff files.
In a typical scenario, we'd be able to rely on the implicit sorting of the diff files OR on a manual sort (e.g. by sorting them based on filename).
There are multiple confounding factors, however:
- The Diff files (as rendered) are not the ultimate source of truth for files. One example is when moving through Discussions: each discussion has its own copy of a Diff file attached, rather than pointing to a canonical UUID of a file. This removes the Discussion (and its file) from the implicit sort order.
- Manually sorting the files requires substantial effort. The way the API returns the files does not appear to be based solely on - for example - file name. It's likely that the front end could implement the same sort, but that would require maintenance to stay in sync.
Changes
This merge request switches the existing sort of unresolved discussions to rely on the authoritative order as returned by the back end when retrieving the list of files in the merge request being viewed.
Notable Caveat
As you can see if you review the new match function provided for Diff Files, there are scenarios where we have to fall back to using file_identifier_hash to match files.
This - where an incomplete diff file is included in the discussion response - is one such scenario. We can't reliably match it to an authoritative diff file, so we have to use file_identifier_hash.
This will work for now. As it stands, the Code Review app really doesn't have the concept of multiple merge requests. Each merge request is loaded up in a new "world" as a new page request. In the future, we may want to cache merge requests, their versions, and the files in those versions for either an offline experience or simply to dramatically improve perceived load time. In these cases, file_identifier_hash is insufficient uniqueness to disambiguate files, and these types of sorting mechanisms will break down.
Roadmap
This is part of a multi-stage effort to standardize the sort order of Diff files.
| MR | Description |
|---|---|
| !85284 (merged) | This prior change stored the order as received from the API on each Diff file before it's processed and - eventually - placed in the local store. |
| We're here! |
This change swaps the processing of files when progressing through unresolved discussions to use the API-provided sort order, since that's the order files are displayed in the UI. |
| ?? | This future change will add more nuance to the progression through unresolved discussions by handling certain cases that aren't currently handled, like when a file is collapsed because it's too big (but has had a comment attached to it anyway!), or if a file is manually collapsed (and so the comment is not visible). |
Testing
This one is quite tricky to test, so please follow the reproduction steps in the original issue.
Once you have confirmed that incorrect behavior, you can follow the same steps in this MR to see that the order is now resolved.
Videos
It will be hard to see the problem and its resolution in these videos, but if you also perform these steps yourself, it should be clear that the "After" video shows the proper jump order.
| Before | After |
|---|---|
| simplescreenrecorder-2022-04-20_12.19.08 | simplescreenrecorder-2022-04-20_12.21.01 |