Flatten MR Diffs app state: Assess impact of removing discussions from diff lines

Summary

In the context of the maintainability discussion (&2852 (closed)) it has been repeatedly identified that the way we're currently attaching discussions to diffs has resulted in cumbersome state handling and heavy objects being passed around through getters and such.

This effort will focus on making the state lighter but also having a more resilient way to always get the association made in simple and predictable ways.

Another aspect is to make discussions more DRY in the state, ie, deduplicate where they live in the state.

What will this focus on?

This issue will attempt at moving Discussions (full object) out of diff lines and assess the repercussions of such a change. If anything, we'll only set the discussion IDs in the diff lines.

We'll monitor the performance impact, especially whether this will incur in additional getters/watchers being created to keep discussions reactive.

Details

We've discussed the following scenario:

  • Bringing discussions into the component using mapState
  • Lookup the ID directly
  • ...

Discussions could also be stored in an object, keyed by linecode.

If we need to transform what's coming from the server, we should leverage a web worker to prevent the UI from locking up.

Improvements

Discussed possibility (to be confirmed):

  • Instead of attaching the full discussion in the diff line (which needs to happen for inline and parallel data structures), do so via unique IDs. Then fetch the discussion when necessary from the appropriate part of the state.

Risks

This is a central part of the Diffs rendering.

We need to make sure all parts of the discussion lifecycle is working properly. Maybe adding some test coverage to be sure.

Involved components

  • app/assets/javascripts/diffs/store

Optional: Intended side effects

  • Lighter state (memory usage)
  • Easier to update state of any said discussion
  • Quicker process of attaching discussions to lines (smaller objects being set/unset)

Optional: Missing test coverage

TBD

Edited by André Luís