Move Diff File Discussion Re-Assignment Triggers to Diffs Store
Summary
diffs/components/app.vue
has a couple of places where it attempts to manually maintain discussions on their proper lines.
This is typically done by calling setDiscussions
and then letting the action take over.
The action basically iterates over each discussion, then iterates over each file, then iterates over each line in the file.
This is pretty rough on the browser.
More importantly, it's unlikely the component - Diffs' app.vue
, in this case - should be responsible for managing discussions manually like it is now.
Improvements
There are two big draws to moving the way discussions are managed into the store itself.
-
The store already knows about both discussions and diffs, and when each changes. It will have a much better grasp on when one or the other changes, and be able to trigger the discussion assignment when necessary.
-
The store has a much better idea of what is changing, not just when. As it stands now,
app.vue
is re-assigning the discussions any time any array it cares about changes length, or after any data fetch. In some cases (like when the split diffs load the second view style), re-assigning to every line in every file is incredibly wasteful: the store will know that one view style already has all of the discussions assigned (in the case of split diffs, in this example), and can focus on just assigning the discussions to the newly loaded lines.
Risks
The biggest risk with this refactor is just touching this very sensitive area of the codebase.
Diffs and diff discussions are complex and complicated, and it's easy to accidentally break something.
Involved components
app/assets/javascripts/diffs/components/app.vue
app/assets/javascripts/diffs/store/actions.js
app/assets/javascripts/diffs/store/mutations.js
app/assets/javascripts/diffs/store/utils.js
Intended side effects
- Triggering the re-assignment of discussions to diff file lines is managed exclusively by the Diffs store
- Potential for significant performance improvement by reducing amount of looping, and/or whether re-assignment needs to happen at all