zorbathut
commented
1 year ago NB: if we wanted to go to reddit-style comment notifications (just the reply, no context), it would be trivial to do so
Yeah, this is probably the right choice - the current behavior is, honestly, weird. I was expecting it to be more complicated, if it's easy, go for it!
The story of this PR is that I set out to address #476, hard wrapped some lines to make it easier to reason about, changed the notification subpages to use real paths rather than query string parameters, and realized that we probably mostly fixed the issue with one of the many comment filtering / moderation state / comment_on_publish reworks.
Our notifications logic is nearly identical to upstream differing mainly in: upstream sorts comment replies by new, whereas ours was by old. This has been remedied (as suggested in the meta thread at the time).
Given that the issue doesn't exist on upstream, my theory is it's because of fork-specific comment filtering code. A reply can still get quasi-buried through the following sequence: 1) a reply is filtered, 2) a different unfiltered reply is made, 3) the first reply is then unfiltered. These will appear sorted by submission time descending, not by first time visible descending, which may be perceived as burial, though the notification bell will still light up. We don't have the data readily available in the schema to fix this with the context-included approach, though sort-by-new may ameliorate this in practice.
NB: if we wanted to go to reddit-style comment notifications (just the reply, no context), it would be trivial to do so and would truly solve the aforementioned scenario where initially-filtered comments can be buried. Am willing to amend PR if this is requested.
tl;dr— actual function differences:
/notifications?messages=true→/notifications/messages(etc), comment replies in notifs are sorted by new. Code broken up into more readable blocks.