Streamline discussions requests and rendering
Summary
It's not a short summary, but here we go ;).
All types of noteables in GitLab have a list of notes associated with them, which can be sorted into threaded discussions. These type include:
- Issue
- MergeRequest
- Snippet
- Commit
- Epic
- Vulnerability (?)
- Design (?)
The way we actually retrieve and display the discussions for these items is a bit complex. We start by requesting a full set of discussions from the .../discussions.json
controller API endpoint. This is defined in the IssuableActions
concern, and returns an unpaginated list of all discussions, with the notes for each discussion as members of that.
The frontend requests this as a single giant JSON array, which can easily be 1MiB or more, and take tens of seconds to generate, since each note requires involved HTML modifications, to take access control into account. We've been talking about adding pagination to it, but pagination works on the basis of the discussions; a discussion can have an unbounded number of notes..
It's a bit awkward. Even if we restrict the number of discussions to 20 per request, via pagination, that could still be 10,000 notes or more, so the degree to which it helps is limited.
Once the initial discussions have loaded, the frontend updates with new notes that are added after the page is loaded. It does this by calling the .../notes
controller endpoint with an X-Last-Fetched-At
request header. At present, this is set to the time of the last fetch, or "now" for the first fetch. All notes since the last fetch are returned, so again, this endpoint is unpaginated - but the notes are returned in a flat format, then merged into existing discussions (or added as new discussions) one at a time. Also sent in the response is a new value for X-Last-Fetched-At
(Time.now
).
We've been talking about pagination for the discussions endpoint, and its performance has been a concern, but the notes endpoint has, thus far, flown under the radar - probably because it's rare for many notes to be added in a short time, but it's not difficult to make it generate a very large response, simply by sending X-Last-Fetched-At: 0
in the request.
Improvements
It seems we already have all the logic we need in the frontend to take an unstructured list of notes from a paginated endpoint, so... why don't we just do that? The flow would look like:
- Load "/show" page, including JS
- Starting state:
lastFetchedAt = 0
- We show the skeleton
- Ignore the
discussions
endpoint! - Call
notes
withX-Last-Fetched-At: {lastFetchedAt}
, which returns:- 20 oldest notes by
updated_at
- New value for
last_fetched_at
that is theupdated_at
of the last note in that set
- 20 oldest notes by
- Frontend renders these items before the skeleton
- If the number of notes in the response was 0:
- Hide the skeleton
- Sleep for a polling interval.
-
GOTO 5
!
This seems magic to me. We end up with a simple keyset-paginated (on updated_at
) endpoint that does everything we need to render the discussions on an issue or merge request (I've not looked at the other noteables, but most I don't have a question about. Commit, Vulnerability, Design are the ones I'm unsure about).
Risks
Does it not work? Have I missed something?
Previous discussions about paginating discussions have focused on how the user experience is poor, since elements start jumping around. Quite how we present this to the user so it's pleasant, I'm uncertain, but we need some kind of pagination here, so it feels like it's going to be a problem for every single approach we take, and indeed, is already an issue when someone adds a new note to a noteable you're working on. So I don't think this should block pagination of discussions. Instead, we should focus on changes we can make to the UI to make it less painful, and work on those in parallel
Two things are at the bottom of the discussions list: action buttons for the noteable (close issue, for instance), and the "add a new note" form. I think we could move the action buttons to the top - this makes them perfectly usable regardless of the state of the discussions.
For the "add a new note" form, it's a bit more difficult. We can't just move it to the top without it being confusing. Maybe a modal, or a floating element tied to the bottom of the viewport?
@danielgruesso @fguibert @gweaver @jprovaznik @pslaughter @cablett @pedroms (sorry, it's a long list, I know!) I wonder what you all think of this idea. I've been fiddling with the discussions endpoint and MR discussions frontend for a little while over the past day or two, and the more I touch it, the more I want to adopt a sort of scorched-earth strategy
How do we solve discussions pagination efficiently without doing something like this? If the frontend can already handle an unstructured list of notes, why don't we lean on that for the whole thing? I'm very interested in reasons why it wouldn't work