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

Support notifying on gitlab review comments #314

Merged
merged 3 commits into from
Apr 21, 2022

Conversation

Half-Shot
Copy link
Contributor

This one was a bit tricky. GitLab doesn't have a notion of "review comments" so we've had to play a bit of a game to make it work like we have on GitHub.

Specifically, we determine a GitLab review by the following:

  • Did a note come in for a Merge Request
  • Was the note not from the original MR author

We also batch together several notes for the same MR into one matrix message if several of them arrive in a short period of time (5s), since GitLab sends the review comments at the same time.

I'm not particularly happy with this approach, but I think it's probably better than not having them.

Co-authored-by: Christian Paul <christianp@matrix.org>
Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible to me.

@Half-Shot Half-Shot merged commit aab1c14 into main Apr 21, 2022
@Half-Shot Half-Shot deleted the hs/gitlab-review-comments branch April 21, 2022 16:11
const existing = this.debounceMRComments.get(uniqueId);
if (existing) {
clearTimeout(existing.timeout);
existing.comments = existing.comments + 1;
Copy link
Contributor

@jaller94 jaller94 Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know +=? :D

Suggested change
existing.comments = existing.comments + 1;
existing.comments += 1;

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

Successfully merging this pull request may close these issues.

2 participants