Notebook Renderable Diffs - Merge Requests
PSA: This feature is blocked
Why
As reported by users (&6589 (comment 744988324)):
Can this functionality be configurable/disable-able?
Some of us don't want to review code that has been through any pre-processing - we just want to review the actual code that is being proposed to merge. So far our interaction with the feature has been unhelpful and frustrating, with frequent conversion bugs (bits of JSON bleeding into the markdown, and just plain misleading diffs) that give no confidence in the user trusting the diff they are reviewing. We've been resorting to having to review diffs for .ipynb files locally.
I feel that removing the option for users to review and see diffs of the actual raw lines of their source files is a big loss.
What
On !75500 (merged) we added the functionality to swap between raw and rendered diffs for jupyter notebooks, as per user request. We now need to add this feature on the MR pages. MR's are already using the latest version of the transformed notebooks, but there are some large blockers in order to display both raw and semantic like we do on commit:
Reason for being blocked: Breaking code assumption
The vue front end code relies on a basic assumption: each diff file has only one view, and that view is referenced by file hash. All changes, events and callbacks rely on the state variable this.diffFiles, and on each action this list is iterated in order to find the correct diffFile that generated the event.
This also brings some heavy performance issues: when performing a mutation, instead of changing just the just the component that is related to that diffFile, that change will percolate across the entire app, since the list itself changes. Additionally, on every mutation, we will always iterate to find the correct diffFile (https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/javascripts/diffs/store/mutations.js#L108).
To be able to implement the swappable diffs, we would need to create a self contained text viewer, where every action calls a diffFile passed as parameter to it, instead of relying on the local singleton list. This has proved to be a far larger challenge than anticipated: when trying to design the dependency graph there we stopped at 255 connections that would need to somehow rewrite, each one with potential to cause a bug (internal https://docs.google.com/spreadsheets/d/1j07BSJbO7T_PaARfpnvuBTtuB0t-UWoBeToWa6TS05A/edit?usp=sharing).
Proposed Evolution
Rewrite the DiffView components in such a way that it only references its own local state and not the DiffApp state. Wrap the new DiffView in a feature flag, enable to 50% of the users and compare compare the performance and number of errors.