Skip to content

MR Diffs | Load Diff Types Lazily

Thomas Randolph requested to merge 33183-request-inline-or-parallel-diffs-instead-fetching-all-data-from-backend-in-one-request into master

What does this MR do?

This MR uses the 'single_mr_diff_view' feature flag added in !21366 (merged) and updates the Diffs Vue app to "incrementally" load diffs depending on which type (inline vs parallel) is being requested in the UI.

For #33183 (closed)
Exposed and fixes #39494 (closed)
Impacted by #39104 (closed)
Depends on !21366 (merged), !21573 (merged)

There are three broad categories of changes:

  1. Load single diffs
    1. In app.vue, a bit of logic is added to determine when the next diff should be loaded. This basically boils down to "when the state contains only half of the possible diffs."
    2. Also in app.vue, we force the batch file loading to re-assign the discussions, because if the discussions load before any file, they're never re-assigned (which is true for the majority of the files, usually everything except the first one).
    3. In actions.js, the request for the data is updated to use the newly available query parameter for the correct view type. Toggling line discussions also toggles the relevant discussion on both the inline and parallel lines, since we can't depend on one or the other being the authoritative source of truth.
    4. In utils.js, a significant amount of work is added to getDiffPositionByLineCode. It was depending on highlighted_diff_lines always being available, and that's not always the case now. A complementary section for parallel_diff_lines is added.
    5. Also in utils.js, prepareDiffData is significantly refactored. While it's behavior is largely the same, a few notable changes include:
      1. The counted lines now increment for either diff type, where it used to only increment if( highlighted_diff_lines ), but used parallel_diff_lines.length in that if block for some reason
      2. trimFirstCharOfLineContent is no longer used internally, but is exported for use, so it's deprecated. It's been replaced with a simpler function that doesn't have side effects (trimFirstCharOfLineContent deletes an unmentioned object property!)
      3. All diffs are forced to include both highlighted_diff_lines and parallel_diff_lines, since the split diffs only send one. The missing property will be filled with an empty array.
    6. The remainder of the changes are dealing with guaranteed arrays (so checks for the property now check for the property length) and various checks for and passing of the feature flag.
  2. Merge diffs
    1. These changes are mostly in utils.js, and merge any existing diff files with the same unique ID as a newly loaded diff file with that new file. The new file takes precedence. This way, loading the second style of the diff view "completes" the diff file.
    2. The remainder of the changes are passing the existing diff files into prepareDiffData when it's called so that it merges files if possible (it assumes an empty array if nothing is passed)
    3. Without this merge, the MR UI will re-request each split diff over and over (every time the diff style is toggled in the UI), because each split diff overwrites the last one, which means the diff files in state only ever have half of the diffs loaded.
  3. Everything else is tests: Removing the feature flag stub and an intentional failure state from a large number of RSpec tests, updating the existing tests, adding new tests, etc.

Gosh, this MR is large!

From a number-of-files-touched perspective, it's unavoidable.

The entirety of the logic is in just 6 files: app.vue, diff_file.vue, diffs_helpers.js, actions.js, mutations.js, and utils.js.
However, once these six files are included (even if the feature is specifically disabled at the app.vue level), every one of the RSpec tests begins failing. This was implemented intentionally. Once you pull in those files, update the JS tests, and add a changelog file, you arrive at the total files changed.

From a lines-of-code-modified perspective, it's tough for me to justify fewer lines.

A lot of the changes could be fewer lines of code, but often by sacrificing what I consider to be "good readability."
There are a bunch of places where a bunch of code is refactored, but I think adhering to the "boy-scout" rule here is a good thing. There were a non-trivial amount of times when I encountered code that I thought could be markedly improved by moving it to a function, switching to a built-in iterator (like .forEach, .filter, or .reduce), or some other refactor. I prefer doing these when I'm working in the code rather than deferring to cleaning up tech debt later (because later is never 😉).

As a final note, the Lines of Code that have changed here are pretty much indicative of how complex and interwoven the diff files loading logic is.

I am sorry about the amount of review work inherent here, and I'm happy to work alongside a reviewer to walk through the changes to eliminate some of the overhead of simply understanding what's been changed.

There are comments in the source!

...and that means this doesn't pass the Code Review Guidelines!

Well, technically it says to avoid comments in the source 😉.

In the case of the comments I've left in the source here, I'm using two reasons to justify:

  1. This code is quite complex. For some of the more basic iterators, the function name and the behavior of the body is enough to describe what something is doing or - importantly - why. In other places, it's sometimes a little unclear why something is happening. Here is one example. Without an explanation like this, it might be unclear why we need to specifically set the discussions at this particular point.
  2. I've encountered quite a few places where it took me a while to understand why a decision was made. I'd like the next person to not need to spend the same time the next time this code needs an update. A good example was this comment. It helped me understand very quickly what this block of code was doing, and how I needed to change it. I expanded on that comment when I added my extra logic to help explain what I added.

Screenshots

There aren't really any UI changes, because this is just changing the way inline versus parallel diffs are fetched and computed from the API.

However, we're dealing with the behavior of the Inline/Side-by-side buttons here:

Screenshot_from_2019-12-19_16-01-57

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • ~~Label as security and @ mention @gitlab-com/gl-security/appsec~~
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by 🤖 GitLab Bot 🤖

Merge request reports