Warning on merge request diff when files collapsed
Problem to solve
Collapsing large files on a merge request diff is done to enhance the performance and responsiveness of the diff section in a merge request. However, the fact that some files are collapsed may be missed when scrolling through the list of files, causing some files to go unreviewed.
We have introduced a visible warning atop the merge request diff page, clearly informing the user when a file in the section is collapsed. This will ensure all related changes in the merge request are reviewed.
Further details
Users have rightfully complained about how the current situation where we collapse some files for performance reasons might lead to missing important changes during code review.
This can lead to bugs, outages and other major problems in production for our users and customers.
Also, when reviewing code the reviewer might run a native browser "Find in page" to see the occurrences of said variable/term. If they're not aware some areas are collapsed, they might be lead to believe it doesn't occur in the rest of the changes. But it might.
Make it clearly visible when at least ONE diff file is collapsed. In the warning add a button to "expand all".
Note: we might want to add a slight note in a popover/tooltip that expanding all at once might cause some performance problems.
User experience goal
Make users easily and clearly aware when the content shown is only a partial view of the whole set of changes
Proposal
It would be beneficial to set a priority order for the alerts shown above the diff. Today, we have two alerts: when there are too many files to show and when there are merge conflicts (mockup). With this issue, we will have a third case: when some files are collapsed by default. To avoid the situation where we show 2 or 3 of those alerts at the same time, we could set a priority to show only one at a time, like so:
- Merge conflicts
- Too many files to show
- Some files are collapsed by default
As for the alert to be introduced in this issue, some files are collapsed by default:
However, as it's rightly noted in the description, it should still be visible if users follow a link to a discussion/file (i.e., make it somehow sticky). Today, those two alerts shown above the diffs are not sticky, so we should change that too.
Further details
- Shouldn't apply in File-by-file mode
- It should still be visible if users follow a link to a discussion/file. ie, make it somehow sticky.
Stretch goals
- #243572 (closed): Adapt the existing “Too many files to show” big, orange alert so it conforms with our Pajamas guidelines on alerts and is consistent with the other alerts shown above the diff. Also make the UI copy a bit easier to understand.
-
#243573 (closed): Move the
Show latest version
andExpand all
buttons to the left, after the comparison dropdowns, so they are more visible. - #243574 (closed): Change the button label to “Expand all files” so that the action is crystal clear.
Documentation
Add a section to diffs mentioning an increased awareness if any content has been "hidden" for performance reasons.
Availability & Testing
What does success look like, and how can we measure that?
Reviewers stop missing important changes. We can assess this via qualitative surveys.
Is this a cross-stage feature?
devopscreate groupsource code only.
Release notes
Collapsing large files on a merge request diff is done to enhance the performance and responsiveness of the diff section in a merge request. However, the fact that some files are collapsed may be missed when scrolling through the list of files, causing some files to go unreviewed.
We have introduced a visible warning atop the merge request diff page, clearly informing the user when a file in the section is collapsed. This will ensure all related changes in the merge request are reviewed.