View comments and system notes real-time edits (but not deletes)

Resources

FE @MadLittleMods

• Comment updates are currently not updated in real-time. They should be.
• Their transitions should follow the GitLab UX guide standard of quick update: https://docs.gitlab.com/ee/development/ux_guide/animation.html#quick-update
• Follow up to https://gitlab.com/gitlab-org/gitlab-ce/issues/29977.
• Real-time deletes are out of scope for this issue.

added Deliverable ~874211 frontend ~19492 labels

added ~897282 to schedule and removed Deliverable labels

changed milestone to %Backlog

changed milestone to %9.2

added Deliverable label

removed ~897282 to schedule labels

@victorwu I don't think any active UX work is required here. I'll follow along to see if there are any questions.

@MadLittleMods : Assigning it to you since I see it's in the description and no BE is required for this one I believe.

assigned to @MadLittleMods

@MadLittleMods : Is everything clear in the description? Let me know if anything is confusing or doesn't make sense.

@victorwu

@jschatz1 mentioned BE was needed and we are currently waiting on @smcgivern to reply back next week to see who is going to tackle.

Thanks @MadLittleMods for the update. @smcgivern : Let us know. Thanks!

Our current model for notes polling is to always add notes. I think it is possible to invalidate the cache when any note is edited or deleted, but we would need to fetch all notes instead. This would also only be on issues initially.

@adamniedzielski is that right?

removed assignee

@adamniedzielski Friendly ping ^, https://gitlab.com/gitlab-org/gitlab-ce/issues/30458#note_27587326

@smcgivern Who will be tackling this?

@smcgivern @MadLittleMods

Edited notes

It looks like NotesFinder already returns edited notes, because it queries by updated_at. We also expire ETag cache when a note is edited (or deleted). After reading notes.js I believe that it simply ignores these edited notes. Modifying the JavaScript should be all that we need.

Deleted notes

This is harder, because we don't have these deleted notes in the database, so we can't query for them. I can think of two options:

1. Implement "soft deleting" for notes - keep them in the database so we can return "recently deleted" ones.
2. Remove our mechanism for incremental updates and just return all notes

I believe that option 2 is better in general (at least from the backend perspective). Returning all the data is easier to reason about than incremental updates and we will remove a mechanism that is only used for notes (different than other places where we do polling).

However there are a few caveats:

1. Incremental updates are a performance optimization and we can only remove this optimization because of ETag caching. However, ETag caching is implemented only for issues. https://gitlab.com/gitlab-org/gitlab-ce/issues/29939 is tough and depends on https://gitlab.com/gitlab-org/gitlab-ce/issues/30224 which is quite a big deal.
2. We share the code between issue notes polling, merge request polling and so on. We would have to support two parallel flows both in notes.js and NotesFinder if we want to do this only for issues.
3. On a cache hit, the backend returns 304, but the response is intercepted by the browser and the previous response is served from cache, so it's transparent from the point of view of the JavaScript code. This requires some smart diffing mechanism if we want to avoid re-rendering all the notes every few seconds.

To sum up, I'm in favour of doing the easy thing - edited notes and postponing deleted notes.

Thanks @adamniedzielski - @victorwu can we do the easy option, and not show deletions for now? They are relatively uncommon AFAIK.

They are, but so are edits:

• Deletions: https://performance.gitlab.net/dashboard/db/rails-controllers?from=now-12h&to=now&orgId=1&var-action=Projects::NotesController%23destroy&var-database=Production
• Edits: https://performance.gitlab.net/dashboard/db/rails-controllers?from=now-12h&to=now&orgId=1&var-action=Projects::NotesController%23update.json&var-database=Production

@smcgivern I couldn't believe that edits are deletions are so rare and I thought that our monitoring is broken. But it's not, I tested it out, It tracked my requests, so they are really that uncommon.

mentioned in issue #31213 (moved)

changed title from View comments and system notes real-time edits/deletes to View comments and system notes real-time edits

changed title from View comments and system notes real-time edits to View comments and system notes real-time edits (but not deletes)

mentioned in issue #31216 (closed)

@adamniedzielski @smcgivern: Updated description here to clarify deletes are not in scope. Real-time deletes are tracked in this issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/31216.

Edit: Here instead: https://gitlab.com/gitlab-org/gitlab-ce/issues/31213

mentioned in issue #31227 (moved)

mentioned in issue #26308 (closed)

mentioned in commit 0be556eb

mentioned in merge request !10837 (merged)

mentioned in commit 93587a27

mentioned in commit 545032e1

assigned to @MadLittleMods

mentioned in commit d8cd4fd8

mentioned in commit 6c69314e

mentioned in commit e2f4ea4b

mentioned in commit 665d1919

added real-time label

mentioned in commit affa5bba

mentioned in commit 8a8cd8ba

mentioned in commit 986600b0

closed via merge request !10837 (merged)

assigned to @victorwu