Page MenuHomePhabricator

Highlight all new comments since a specific timestamp when opening a new comment notification
Open, Needs TriagePublic

Description

When you are notified about a new comment, or new comments, we generate a link which has all the comment IDs in it to highlight them.

There are two problems with this:

  • That URL hash can get very long. After ~20 comments IDs it would probably break in most browsers
  • Any new comments added between you opening your notifications and you clicking on the link will not be highlighted

Both these problems can be solved by using only the ID of the oldest new comment. We can then highlight all comments in the thread containing this comment and with a timestamp greater or equal to this comment's timestamp.

In T301602 it is proposed to warn the users if the number of comments highlighted doesn't match the number in the notification. If we implement the above, I think we only need to warn users if there are no comments on the page to highlight.

Event Timeline

Change 763961 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] Highlight all comments since the oldest in a thread bundle

https://gerrit.wikimedia.org/r/763961

There are also some edge cases where this will result in a slightly weird behavior:

  • If there are multiple comments posted within one minute, we'll always highlight all of them
  • If the user marks some of the bundled comments as read, we'll still highlight them when following the bundled link
  • If the user replied on the page without marking their notifications for that page as read, we will highlight their own comment

But overall, it seems better than before.

Change 763961 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Highlight all comments since the oldest in a thread bundle

https://gerrit.wikimedia.org/r/763961

matmarex moved this task from QA to Code Review on the Editing-team (FY2021-22 Kanban Board) board.
matmarex removed a project: Editing QA.

Moving the comment and follow-up patch from the other task:

On live I clicked on a bundle of 15 notifications, and only the last two were highlighted. @matmarex questioned in code review if the 0th comment in a bundle was the oldest, and while there is code in place to sort the messages by timestamp, it appears this isn't always the case...

Change 768827 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] Fix logic for finding the oldest comment in a bundle

https://gerrit.wikimedia.org/r/768827

Change 768827 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Fix logic for finding the oldest comment in a bundle

https://gerrit.wikimedia.org/r/768827

Change 768796 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.24] Fix logic for finding the oldest comment in a bundle

https://gerrit.wikimedia.org/r/768796

Change 768797 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.25] Fix logic for finding the oldest comment in a bundle

https://gerrit.wikimedia.org/r/768797

Change 768797 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.25] Fix logic for finding the oldest comment in a bundle

https://gerrit.wikimedia.org/r/768797

Change 768796 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.24] Fix logic for finding the oldest comment in a bundle

https://gerrit.wikimedia.org/r/768796

Mentioned in SAL (#wikimedia-operations) [2022-03-08T14:32:04Z] <urbanecm@deploy1002> Synchronized php-1.38.0-wmf.24/extensions/DiscussionTools/includes/Notifications/DiscussionToolsEventTrait.php: 23939c7: Fix logic for finding the oldest comment in a bundle (T302014) (duration: 00m 50s)

There are also some edge cases where this will result in a slightly weird behavior:

Another funny edge case noted by @Ryasmeen:

  • If the oldest message in the bundle has disappeared, we'll show a warning; but if any other message has disappeared, then we won't show a warning and we'll just highlight the other ones.