Page MenuHomePhabricator

Notifications menu has a couple of minor rendering issues
Open, LowPublic


When Aphlict isn't enabled, the little "" icon renders under the text here:

Screen Shot 2018-04-19 at 10.20.47 AM.png (68×403 px, 7 KB)

Unread notifications also currently show a grey dot but this feels inconsistent to me since they're colored with a blue wash and we use grey to indicate "disabled" in other interfaces.

Event Timeline

epriestley created this task.

The "Notification server not enabled" text should also link to the Aphlict documentation or something.

D19384 cleans this up a little bit and mostly fixes the "phantom unread notification" issue in T8953.

The way this menu is built is still a little bit funky. In particular:

  • Because of how the query works, we fetch the first 10 results, then filter them. If some of them are filtered, you get fewer than 10 results.
  • It would maybe be nice to always put unread notifications on top.
  • The "purge phantoms" code in D19384 may need to do a lot of work if you have thousands of unread notifications.

A cleaner approach here would likely be:

  • Query first, then filter later, like a normal policy query does.
  • Order by hasViewed first, then chronologicalKey.
  • Let the caller retrieve the selected-but-discarded rows from the query and purge them more narrowly, or purge them during filtering.

This would give us better behavior overall: it would always show 10 results, would make the purge cost proportional to the number of items which need to be purged, and would show unread notifications first (which is probably a more useful behavior than showing notifications chronologically). A downside is that it would put this query at risk of overheating, although that shouldn't be too difficult to handle.