matrix-org / matrix-org/matrix-hookshot
What is the purpose of the `merge_request.review.comments` GitLab hook config option?
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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