Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider what needs to happen on annotation editing regarding mentions. #9351

Open
acelaya opened this issue Feb 14, 2025 · 2 comments
Open
Assignees

Comments

@acelaya
Copy link
Contributor

acelaya commented Feb 14, 2025

EDIT from @acelaya: We originally decided to not support mention editions, but it has already brought up that this is more confusing than it seemed. See this discussion hypothesis/client#6818 (comment)


EDIT 2 from @acelaya:

We have discussed that what makes more sense is to always keep the entries in the mentions table in sync with the mention tags in the annotation text.

  • If a mention is removed from the text, we remove the corresponding row.
  • If a mention is added, we add a new row.

So basically, every time an annotation is edited, we need to "delete" all mentions and re-process the mention tags from the annotation text.

Separately, we need to track which of the mentioned users have been notified, as every one should be notified only once per annotation.

  1. When the annotation is created, we notify all users that have been mentioned.
  2. When the annotation is edited, if the mentions have changed, we notify only users that were not mentioned yet for this particular annotation.

Independently to individual mentions, the annotation could be edited in a way that is no longer available for mentioned users (it's made non-shared, it's moved to a different private group, etc). In that case we should only notify the newly mentioned users that are still "eligible".

In this case there's of course a scenario in which some users may have been already notified when they could still access the annotation. GitHub handles this by deleting the notification so that you no longer see it in your notifications tray.

Open questions:

  1. How do we handle the mentions limit per annotation when edits happen?
  2. If a user is mentioned, then the annotation is edited and the mention removed, and then edited again and mentioned back, should we notify them a second time? I think we shouldn't.
  3. In the last scenario, should we also delete the notification on our side as GitHub does, or we simply not care about it and let users run into a 404/error message?
@acelaya
Copy link
Contributor Author

acelaya commented Feb 17, 2025

@mtomilov
Copy link
Contributor

mtomilov commented Feb 17, 2025

Open questions:

  1. How do we handle the mentions limit per annotation when edits happen?

Probably having two separate limits. One for number of mentions on an annotation N which is considered on create / edit. And a separate limit for mention notifications M which we consider at send time by consulting the notification table. So even if a user creates N mentions, deletes them and creates another N mentions again we can cap total number of notifications at M.
Also this approach makes it easier to develop the supporting functionality and reason about the interplay logic between mentions and notifications.

  1. If a user is mentioned, then the annotation is edited and the mention removed, and then edited again and mentioned back, should we notify them a second time? I think we shouldn't.

I definitely concur. And again by consulting the notification table at send time we can skip notifying the user if they have been already notified in the scope of this particular annotation. Probably it doesn't even matter if it was a mention notification or a reply one.

  1. In the last scenario, should we also delete the notification on our side as GitHub does, or we simply not care about it and let users run into a 404/error message?

To be honest it feels like an optimization and a bit questionable one at that since it introduces duality between email notifications and in-app notifications since we can't unsend an email in any case. A compromise mentioned in slack was to render invalid in-app notifications in a different way compared to the valid ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants