Skip to content

Add Lorenz van Herwaarden as a frontend maintainer

Lorenz van Herwaarden requested to merge lorenzvanherwaarden/become-frontend-maintainer into master

Mentor Justification

It's hard to specify hard requirements for becoming a maintainer, which is why the documentation consists of flexible guidelines. Reviewers are encouraged to think of their eligibility for maintainership in the terms of "I could be ready at any time to be a maintainer as long as it is justified".

  • The MRs reviewed by the candidate consistently make it through maintainer review without significant additionally required changes.
  • The MRs authored by the candidate consistently make it through reviewer and maintainer review without significant required changes.

From @aturinske : Lorenz is a skillful reviewer, providing thorough and insightful comments, which is evident from his many examples below. What really stuck out to me about Lorenz was how much he cares about providing a fast and detailed review; he tests locally and if he finds an issue, he provides a patch so that there is less review cycles or defaults to a non-blocking comment. With that I recommend Lorenz for the frontend maintainer role.

My application

Lorenz van Herwaarden's frontend maintainer application
Hi! 😄 I joined GitLab a bit over a year ago at the end of January 2023. I've been a Senior Frontend Engineer in the groupthreat insights team since then, with an exception of a small borrow period of about a month in groupcompliance. @aturinske has been so kind to mentor me during this traineeship, which has been very helpful.

Trainee issue

Details of about every review I've done during my traineeship can be found in my trainee issue.

Some quick stats

Review philosophy

I try to live by all our GitLab values and code review guidelines, but I think these 3 characteristics stick out.

  1. fast & non-blocking (efficiency; results for customers; iteration; diversity inclusion, and belonging)
    I try to respond quickly to review requests. I'll also normally let the author know if I can't get to the review in the current working day. I try to be pragmatic, but that doesn't mean I'll shy away from posting all the suggestions I have. It just means I'll lean more towards non-blocking feedback so it's at the author's discretion. [1, 2, 3, 4, 5]
  2. test locally (results for customers; transparency)
    I always try to test the changes locally. Even small changes could have an unexpected impact that one would not expect at first. Validation steps, especially when going outside of your own domain, can sometimes take quite a bit of time to set up. However, I try to see this as an opportunity to learn more about the product, backend, and integrations. I've modified the author's validation steps a couple times if I found that a step that was missing or wasn't correct. [1, 2, 3, 4, 5]
  3. provide patch or suggestion (collaboration; efficiency; transparency)
    I often try to provide a patch to clearly illustrate the changes I'm suggesting or how an issue I've found could be fixed. Providing code with a bit of explanation is often way more valuable than a long explanation on how the changes would look like. In case there's an actual issue/bug with the MR, it can take considerable time to debug. I'll try to timebox this, but I think it's worth spending a bit more time on the review in that case because I would already be in the context of that specific issue. [1, 2, 3, 4, 5, 6]

Non-trivial MRs I've reviewed

Not really in any specific order, they're all non-trivial in their own right. The trainee entry link summarizes my feedback per MR.

MR & trainee entry Changes Notes
Render blame info in blob viewer (gitlab-org/gitlab!133798 - merged)

trainee entry 		+379 -54 This review was non-trivial because it was pretty complex code to follow. I found a couple good suggestions in regards to code quality, testing, UX, and internationalization. I suggested (with a patch) moving a side-effect that was triggered within a computed property to a watcher method instead, I suggested rendering the blame view if the querystring contained the blame parameter, I suggested removing an object from the GraphQL query which was not used, I suggested using a translatable string for the blame button, and I suggested some valuable test coverage improvements regarding event handling.
Add ability to filter Add-ons by group (gitlab-org/gitlab!142304 - merged)

trainee entry 		+340-6 This review was non-trivial because I found some properties in a component which were not being set anywhere and hence did not have any effect. With some further digging I found they were actually only being set in the spec. After questioning why this was added, it turned out the author added it specifically to test the component because it wasn't working with the normal apollo mock. I explained that I thought this was a bad practice and I questioned why the apollo mocking behavior didn't work at first. I also provided a patch where I removed the unnecessary properties/logic and made the spec work with an apollo mock.
Work item Epic inline color widget (gitlab-org/gitlab!141639 - merged)

trainee entry 		+470-3 This review was non-trivial because the diff was big, there was a lot to improve, and I had some fundamental questions about how of a couple things were implemented. It required 3 rounds of initial frontend review and a couple of in-depth explanations and a patch to get to the maintainer round. The most interesting comment I made was: I questioned why we were refetching the whole work item in the color component while we already had this readily available in the parent component. I provided a patch which removed this, simplified the caching behavior and optimized the props being passed. One of the conclusions of this review was that I should request to split up big MRs more quickly.
Add group by new feature popup (gitlab-org/gitlab!133591 - merged)

trainee entry 		+116 -29 This review was non-trivial because at first glance it seemed like the callout popover was working as expected. However, after testing different states and scenarios in combination with some debugging I found a couple issues: the callout was dismissed after only hovering it, the callout would re-appear if it was actually dismissed and required 2 clicks to fully disappear, and there were actually 2 popovers being displayed at the same time. I provided suggestions to fix some of the bugs and my debugging steps to further explain my analysis.
Improve info on Free Public Namespaces (gitlab-org/gitlab!138802 - merged)

trainee entry 		+308-374 This review was non-trivial for a couple reasons: I found a lot of suggestions for code removal or refactoring which weren't directly obvious from the diff itself, I found quite some test coverage which had become outdated from previous MRs, the author disagreed on 2 code quality suggestions I made, and I had to take over after another initial frontend review so there was quite a large context to start the review.
Fetch counts for each group (gitlab-org/gitlab!132542 - merged)

trainee entry 		+153 -16 This review was non-trivial because even though, initially, the changes looked fine in terms of code quality, functionality, and UX, I found 2 functional bugs, a UX suggestion, and a code quality improvement.
Improve readability of vulnerability header_spec (gitlab-org/gitlab!139442 - merged)

trainee entry 		+185-191 This review was non-trivial because I didn't agree entirely that the MR would improve the readability of the spec file and I explained why this was the case. I suggested another approach and the author agreed. I also noticed that a test case being removed actually had to stay but this wasn't obvious and I explained why. I also suggested we had to improve this and the author created a follow-up.

Projects with multiple MRs I was reviewer for

Project Author(s) MRs
Kubernetes dashboard / detail views [1, 2] @anna_vovchenko 1, 2, 3, 4, 5, 6, 7, 8
Vulnerability Report Grouping [1] @svedova 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14
UX Paper Cuts [1, 2, 3] @annabeldunstone @seggenberger 1, 2, 3, 4, 5, 6, 7

Reached out to a domain expert/maintainer for advice

MR Notes
Update loading of streaming destinations (gitlab-org/gitlab!133151 - merged) I found an issue with a spec that wasn't properly testing the loading icon in the case one of the two apollo queries was already done loading. I posted a message on the #frontend slack channel on how we could advance only one graphql mock. The answers in the thread helped me come up with a patch to provide to the author.
Improve info on Free Public Namespaces (gitlab-org/gitlab!138802 - merged) A safeguard was being introduced to a widely-used text utility. I wasn't sure whether this was the way to go because many utilities do not have extra safeguards. I didn't block the review for this but instead pointed the maintainer towards my thought to have a second look.
Improved submit_review quick action (gitlab-org/gitlab!136863 - merged) I pointed out that I was unfamiliar with this part of the code and let the author know it might be best to make sure the maintainer that would be assigned has some domain knowledge.
Migrate buttons in awards_block.html.haml (gitlab-org/gitlab!141930 - merged) I found some orphaned code and explained this with a mermaid diagram because it wasn't super obvious. The orphaned code was not due to the changes in the MR so I did not block the review and opted towards asking the maintainer to double-check and if verified, create a follow-up issue. The maintainer had the same insights but also verified on the frontend slack channel to ask about the functionality around this orphaned code. The maintainer created a follow-up issue to delete the code.

Reviewed community contributions

My work I'm proud of

  • Integrating the vulnerability dismissal reasons and descriptions in the finding modal, vulnerability report, and vulnerability details pages [1, 2]
  • Removing last uses of the legacy finding modal which covered frontend, backend, and E2E test changes. This results in the removal of >5000 lines of code [1, 2, 3]
  • Migrating the Pipeline Security Listing to GraphQL by using shared components from the Vulnerability Report [1]. This included refining issues, finding a MVC/Post-MVC path, frontend and E2E test changes, and a lot of collaboration.
  • Refining/triaging issues and implementing UI/UX improvements to the audit events streaming, covering 12 MRs in a borrow period of 1 month.
  • 30 vue3 spec violation MRs
  • Found couple of significant reductions in bundle size [1, 2]
  • Migrate a couple GlDropdowns in Threat Insights domain

Before Merging (Manager Tasks)

  • Close any relevant trainee maintainer issues with a comment indicating that this merge request is being created, as (they are no longer required to become a maintainer).
  • Mention the maintainers from the given specialty and ask them to provide feedback to the manager directly.
  • Leave this merge request open for 1 week, to give the maintainers time to provide feedback.
  • Ensure we have at least 2 approvals from existing maintainers.

Once This MR is Merged

  1. Create an access request for maintainer access to gitlab-org/<project>.
  2. Join the [at]frontend-maintainers slack group
  3. Ask the maintainers in your group to invite you to any maintainer-specific meeting if one exists.
  4. Let a maintainer add you to gitlab-org/maintainers/frontend with Owner access level.
  5. Remove @lorenzvanherwaarden as trainee from mentor @aturinske in maintainer_mentorship.yml
  6. Announce it everywhere
  7. Keep reviewing, start merging 🤘 😎 🤘
Edited by Lorenz van Herwaarden

Merge request reports