matrix-org / matrix-org/matrix-hookshot

What is the purpose of the `merge_request.review.comments` GitLab hook config option?

Open
#957 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
450
Forks
95
Avg merge
1d 2h
Merged PRs (30d)
23

Description

Currently it's not displayed in the widget, and it doesn't actually do anything, but there is some code around it.

The purpose of the current code seems to be to only send new comments if there is an approval change in the same batch as a comment. That would mean that the user can chose if they only want to see reviews with approvals or all comments. It just lacks the last step to actually skip those comments. See this line of code that sets `skip`, but `skip` is never used after:

https://github.com/matrix-org/matrix-hookshot/blob/2763801d1b677a5a66ee5f1a3b822772c50fce11/src/Connections/GitlabRepo.ts#L914

The issue I see with this is that the wording of the widget says "Completed review". However, it is possible to make a review without changing the approval of the MR. So either the wording needs to be fixed to something like "Approved", or the code needs to be changed to actually detect if a review took place.

The former is simple and fixing the code to use what's in place should be easy.

The latter can be done with the data received by the webhook. With GitLab's interface, outside of approvals, a review is only possible if there is at least one comment on the code that goes with it so we just need to check if there is one of those in the batch. They can be identified with different fields: they have a `position` or a `line_id`, or their `type` is `DiffNote`.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/Connections/GitlabRepo.ts around line 914 and trace how merge request webhook batches are processed and how the skip value is handled. Review the widget wording and the GitLab fields described in the issue for approvals and code comments. Done means the option has a defined purpose and the displayed behavior matches it.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.