Skip to content

In file-by-file MR review mode, only load the single file under review

Thomas Randolph requested to merge tor/feature/file-by-file-load-one-file into master

What does this MR do and why?

This MR changes the diff loading process so that only a single file is loaded in file-by-file mode.

Review recommendations:

  1. Review without showing whitespace changes - there are a few places where code is indented and wrapped in an if and it's more useful to see the differences without the indent.
  2. Review Version 34 first for the feature changes, and then review the commit that enables the feature flag.

Why

Context

For #337263 (closed).

We are making File-by-file view mode much more useful by only fetching the single file that you are trying to view at any time.

In extreme cases, if the file you are viewing is the last in a long list (say 100s or 1000s of files), batch-loading the diffs could introduce an extremely noticeable delay before your single file is loaded and displays while the app loads tons of files you have no interest in.

The above issue - and this chain of MRs - will resolve that problem by loading only the file needed to display.

Specifics

This MR changes the diff data load process so that - when in file-by-file mode - only a single file (the one being viewed) is loaded on demand.

Most of the modifications and new behaviors were implemented in the roadmap MRs listed below, while the - light touch - work to enable the feature is done here.

In roughly source order (scrolling through the Changes diffs of Version 34):

  • When moving from File-by-file back to full MR mode, we need to re-trigger a fetch of the full data, so we guard a refetchDiffData behind "not File-by-file" and "the diffs aren't fully loaded."
  • When the list of files changes, we re-assign discussions so that they appear on the newly loaded file UI
  • When the list of notes changes, we re-read the hash in the URL to go through this process => URL note ID ↦ A discussion (potentially just loaded!) ↦ A file (potentially not loaded, triggering a dynamic load)
  • Only reload the full metadata when requested (for example, we only need it once in file-by-file mode, and every load after would be redundant)
  • Only do the full diffs batch data load when not in file-by-file mode
  • Instead of assuming a file is present to scroll to, use an async scroll (goToFile) that could load the file if necessary
  • If we're in file-by-file mode, a diff file doesn't need to request its own diff, we'll do that at the app level.
  • As with the app, if you click a file in the tree view, don't assume it's loaded and use the async goToFile to possibly also load it before scrolling to it.
  • When we navigate to a diff file, we'll need to fetch it when we're in file-by-file mode. (If a file is already loaded, fetch doesn't trigger new network requests!)
  • When the HAML first loads, if we're in file-by-file mode, it's silly to start doing batch loads, that could potentially waste a lot of network and CPU bandwidth
  • Updating and adding tests for the new bits of behavior (the filePaths new values have been added by a previous MR, but are now required!)

That's pretty much the full set of changes, line by line!

Roadmap

This MR is one of a set of changes that will eventually implement this feature.

Phase 1 - Preparation

Status MR Purpose
🚀 Merged !112515 (merged) Adds the API access we need for the more surgical data
🚀 Merged !112516 (merged) Switches to using our initial metadata representation of the MR for many operations
🚀 Merged !113941 (merged) Marks metadata files as loaded so future uses don't try to load them again

Phase 2 - New Abilities

Status MR Purpose
🚀 Merged !113940 (merged) Adds helpers to re-retrieve the hash from the URL since it only needed to be fetched once before
🚀 Merged !113937 (merged) Adds an action to fetch and store a single file versus the existing full diff batch loading
🚀 Merged !114009 (merged) Add a getter to check the files marked in !113941 (merged) to simplify "already-loaded" guard clauses
🚀 Merged !114024 (merged) Add an action to scroll to a file async (and load it if necessary)
🚀 Merged !114170 (merged) Adds an action to re-check the note hash from the URL and potentially fetch the appropriate file

Phase 3 - Implementation

Status MR Purpose
We are here! 👉🏻 !111895 (merged) Adds the feature that will only load the proper file when necessary to display it

Screenshots or screen recordings

The changes here are basically just to load files faster (more efficiently), so it's hard to directly show the changes.

The below videos, however, do demonstrate the difference between master and this branch.

Please note that in the Before video, there is one final network request to load the diff_for_path for the file being requested.
This is existing behavior that happens on large MRs where files don't start having been expanded.
This happens when a single file is being shown and it is marked as currently collapsed (as it is in my very large MR, whose first file is more than 6000 lines), and it needs to load the diff for that file specifically, because even batch loading it doesn't send the full diff content.

Before (master) After (this branch)
fbf-last-file-batch fbf-last-file-single

How to set up and validate locally

While this will work on any MR, it is easier to see the changes on very large MRs (large count of files + large set of changed lines of code):

  1. Enable the feature flag in the rails console: Feature.enable(:single_file_file_by_file)
  2. Create an MR
  3. Push changes that amount to 100+ files with 200+ lines of changes per file
  4. Navigate to the last file in your diff (in my case, it was file 100.txt)
  5. Hard-refresh (clear cache) the page on this branch.
  6. Compare to the network tab on master / with the feature flag off. (Likewise, compare the overall speed/feel of the page).

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Thomas Randolph

Merge request reports