Changing how and what pull request companion adds to pull requests

[schalkneethling]

A discussion started on Matrix about the general noisiness of the PR review companion on pull requests on mdn/content. One option offered was using a GitHub Action to collapse comments from the github_actions bot. Perhaps this is more of a band-aid on the underlying problem.

Instead of "muting" the companion, perhaps we must address what it reports on pull requests.

@wbamberg mentioned:

Or not show flaws on PRs, since I at least never look at them. Or only show flaws introduced by this PR, and not previously existing ones.

@SphinxKnight seconded that:

Same here, I look at them locally, not on the PR itself

@OnkarRuikar added that:

The "External URLs" section has redundancy. It logs, "No new external URLs", for every file involved. The single message adds 8 lines.

This discussion then intends to conclude how the PR companion can be most effective and file an issue for consideration by engineering.

[OnkarRuikar]

My only complaint about the PR review companion is that it creates a huge comment(mdn/content#15334 (comment)). Getting to the relevant discussion involves a lot of mouse wheeling. Then when you realize that the comment is really huge you need to grab the scrollbar.

Let us keep all the features as it is, one may find them useful in certain situations.

I would love to see the comment as small as possible. This is how it will look if we use the hide-comment-action:

Not all readers refer to the info. Also, when we revisit the PR we are interested in review comments and discussion more. Readers can always expand the comment and see the info.

On top of it following improvements can also be done:

1. Get rid of the "No new external URLs" logs. Show only if there are URL changes to the file. This will tremendously reduce the comment size.
   

2. Also, the "on GitHub" links are always broken. Either fix them or remove them.

3. We can wrap "Flaws" and "External URLs" sections in <details> tag so it'll further reduce the size:

Video1.mov

---

A temporary solution till this gets implemented

I've created a Tampermonkey (Firefox, Chrome) script that searches given usernames and minimizes their comments and adds a Minimize button.
The script can be found here: https://gist.github.com/OnkarRuikar/c323f320b3c858bfac40e577e6b73802

[caugner]

Get rid of the "No new external URLs" logs. Show only if there are URL changes to the file. This will tremendously reduce the comment size.

@OnkarRuikar Would you like to open a PR for this?

We can wrap "Flaws" and "External URLs" sections in

tag so it'll further reduce the size:

Let's not wrap for now, as it makes it harder for new contributors to discover.

[OnkarRuikar]

Get rid of the "No new external URLs" logs. Show only if there are URL changes to the file. This will tremendously reduce the comment size.

• We've fixed this fix(pr-companion): remove redundant entries yari#7105

[SphinxKnight]

Thanks for starting this @schalkneethling 🙏

I don't want to fall into scope creep here but from a localizer's point of view, I'd say:

• it may help to have a link to the corresponding English page (for each localized page touched by the current PR). Most PRs should (in theory) be maintenance content PRs with propagation of English updates into localized docs and reviewing those includes checking the localized doc vs the en-US doc. (Technically speaking, this would somehow be a difference of implementation between mdn/content and mdn/translated-content :x )
• in addition to @OnkarRuikar comment's, I think flaws have… flaws :) (cf. Localization differences - Address EmbedLiveSample case with its "localized" argument yari#4971 for instance) so I'd advocate for minimizing them at least or removing them.

[Graywolf9]

Thanks for start the discoussion @schalkneethling

only show flaws introduced by this PR, and not previously existing ones.

I think this could be so helpful, if it's possible

Or third option could be to update the bot code to wrap everything in

tag. So the comment content would be minimizable.

Also this could be helpful for those who said that is a large comment, by sections

I consider that the flaws section is helpful for make a quick revision without have to fork the patch.

[Rumyra]

We want to start looking at improving the pr review companion.

There's some great ideas already in this discussion, easily a details element would help with minimising the size of these comments.

But I would really like to know before we start planning: What would you like to see in the pr review?

Here's my initial list:

• Minimize sections
• I think the preview links are incredibly helpful
• Linked things such as linked issues, prs, whether something depends on another pr (like a warning) - I realise these are in thread, but can be skipped over the initial comment, so it'd be nice to have a section for it
• Whether the files touched are greater than say 5, in which case a big review is needed, or it's part of a project which includes mass updates
• This pr has been assigned to ... for review, if you're working in a team and need to modify that please do it now
• Feedback for the contributor:
  • New page? Was the content suggestion process followed.
  • Does it touch a page where the learn repositories should be updated
  • Does it touch a page where demos, interactive examples et al should be updated
  • Timescale and expectations for reviewing and merging

The latter here could be a separate bot, would it make sense to have a 'thank you for the pr, here's some info for you' message, then the review companion with info helpful to reviewers?

[teoli2003]

* Minimize sections 
* I think the preview links are incredibly helpful

👍

I think we have to keep in mind that:

• we shouldn't redo the GitHub UI inside the PR Review Companion. For example, any info easily available in the regular interface shouldn't be duplicated. From your list:

  • Linked things such as linked issues, prs, whether something depends on another pr (like a warning)

    👎 Linked issues are often added after the PR companion run, even years after, throughout the life of the PR. So it won't be up-to-date and is already available prominently in the interface.

  • Whether the files touched are greater than say 5, in which case a big review is needed, or it's part of a project which includes mass updates

    👎 The amount of files is in the tab list at the top, and the amount of touched files is mostly irrelevant for a big review or not. I merged a PR with 300 comments, 6 approvals, and … 1 file. In contrast, many very large PRs with 300 files touched are reviewed quickly by 1 reviewer.

  • This pr has been assigned to ... for review, if you're working in a team and need to modify that, please do it now

    👎 Useless for reviewers; it is on the sidebar.

• Feedback for the contributors doesn't belong in the PR review companion

  • New page? Was the content suggestion process followed?

    👎 I think this is useless for the reviewer and any regular contributors. If the content suggestion process has not been followed, it is too late. Most new pages are submitted by seasoned contributors, seldom by new contributors anyway.

  • Timescale and expectations for reviewing and merging

    👍 👎 In a welcome message for new contributors, before the PR companion kick in (because the PR companion is not launched automatically for first-time contributors)

  • The latter here could be a separate bot. Would it make sense to have a 'thank you for the pr, here's some info for you' message, then the review companion with info helpful to reviewers?

    👍 But only for first-time contributors. Else it is just too much noise.

• Help for the reviewer (checklist)

  • Does it touch a page where the learn repositories should be updated

  • Does it touch a page where demos, interactive examples et al should be updated

    👍 These two should be checkboxes so that a reviewer can think about them and check or uncheck them

• We should shorten the output

  • Don't list files with no flaws in them.

Globally the PR review companion is too long, and people don't use it that much. We should shorten it as much as possible

[OnkarRuikar]

In the PR companion's first comment:

• ✔️ Minimize sections
• ✔️ Preview links (mostly required for testing live sample outputs, macros, definition list indentations)
• ✔️ Flaws
  • ✔️ URL
  • ✖️ Title
  • ✔️ on Github (we need to fix these edit links)
  • ✖️ Flaw count
• ✖️ External URLs - I have no idea what this could be used for. We fixed all the broken links in separate PRs.
• ✖️ Timescale and expectations for reviewing and merging
  This may lead to hurt feelings if things won't happen as promised. Especially new contributors don't have the idea about the pace at which things happen here. This may also put burden on reviewers.

---

On PR creation greeting message:
If it's the first PR by the contributor: "Welcome to the MDN docs! Thank you for the Pull Request!"
Seeing the message the reviewer will congratulate them upon successful merge: "Congratulations on first commit! Welcome aboard..."

After PR merge/close message:

• ✔️ Feedback for the contributor:
  • ✔️ Helpful links
    Like Community guidelines, Writing guidelines, code formatting etc. But show this only for first 5-10 PRs.
  • ✔️ Achievements
    Give virtual trophies (:1st_place_medal: :trophy:) or achievement badges upon completion of 20, 50, 100 commits. The gamification, like stackoverflow, may motivate young contributors more. :thinking:

This can be implemented easily on PR create/close/merge events.

---

Whether the files touched are greater than say 5, in which case a big review is needed, or it's part of a project which includes mass updates

This can also be achieved by using a 'big change 🐘' label. We can update current github workflow to add this label if there are more files.

---

The latter here could be a separate bot, would it make sense to have a 'thank you for the pr, here's some info for you' message, then the review companion with info helpful to reviewers?

Involvement of multiple bots may confuse/distract users. We can use the same bot for more personalized experience. If possible, a friendlier name would be nice like mdn companion 🦖 instead of github-actions.

[teoli2003]

* ✔️ Greeting message
  If it's the first PR by the contributor: "Welcome to the MDN docs! Thank you for the Pull Request!"
  Seeing the message the reviewer will congratulate them upon successful merge: "Congratulations on first commit! Welcome aboard..."

I would not add the Greeting message in the PR companion:

• They have not done a commit, we need to review and modify it.
• They won't see the message: the PR companion doesn't run for first-time contributors. That's the reviewer that manually triggers it, as well as the other tests. It can be several days later. (To protect us from spam)

[bsmth]

I would throw in a screenshot from Netlify's GitHub bot for a reference:

The QR code cell is collapsible, so it can be expanded showing content that is large (this might work well if there are many links to changed pages). I like that it uses a table with name and link header cells, it's neat and quick to read through to find what you want.

Docusaurus also have a similar way of reporting PR impact with their lighthouse bot

[caugner]

One important take away from these screenshots is:

• Tables can help.
• <details> can help, and can be put inside a table.
• Color-coded emojis green/orange/yellow/red can help to spot what needs to be looked at.
• Links don't need to appear fully qualified (with host and everything).

[teoli2003]

I agree with you and just want to point out:

* Color-coded emojis green/orange/yellow/red can help to spot what needs to be looked at.

The color-coded icons of Docusaurus' lighthouse bot convey some information only through colors, so it is not accessible. I'm all for colored icons, but the information should also be conveyed through shape. (Shape & colors). Like we did everywhere else on yari.

[caugner]

My hope would be that :red_circle: (🔴) is read as "red circle" in screen readers.

But looking at the HTML it creates, I'm not so sure anymore:

<g-emoji class="g-emoji" alias="red_circle" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f534.png">🔴</g-emoji>

edit: I didn't have color-blindness in mind, but something like ❌ and ⚠️ would of course be preferable to the same shape in multiple colors.

[bsmth]

I think these ones are good status indicators that are accessible:

❎ Cross Mark Button
✅ Check Mark Button
☑️ Check Box with Check
✔️ Check Mark
❌ Cross Mark
❗ Red Exclamation Mark
⚠️ Warning
🚫 Prohibited

[OnkarRuikar]

Looks like Github converts both Unicode emojis (❗) and GitHub emojis ( :exclamation: ❗ ) to the same HTML:

<g-emoji class="g-emoji" alias="exclamation" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/2757.png">
    <img class="emoji" alt="exclamation" height="20" width="20" src="https://github.githubassets.com/images/icons/emoji/unicode/2757.png">
</g-emoji>

If required we'll have to use our own icons:

<span role="img" aria-label="Description of the overall image">
  <img title="exclamation" alt="exclamation" height="40" width="40" src="https://github.githubassets.com/images/icons/emoji/unicode/2757.png">
</span>

[wbamberg]

For me the starting point is: there is too much in the PR companion's output that is (almost never) useful and is just a source of noise. So I think the bar to adding new things should be quite high and should meet a definite identified need. For instance, are there lots of cases where reviewers would have used the Lighthouse report when reviewing content PRs?

Another reason to be careful here is performance: the PR companion is already slow to run. This is made worse by the fact that many of our PRs are from first-time contributors, so workflows aren't run by default. So a reviewer has to push the button, then wait for the previews to be built (or more likely context-switch into something else and forget about the PR).

One thing that would help, that I'm not even sure is possible. In the preview pages, internal links in pages (like "/en-US/docs/Web/CSS/margin") get resolved to the preview base URL, like "https://pr20182.content.dev.mdn.mozit.cloud". This means all links from a preview page that aren't to another page edited in the same PR are broken, which makes it hard to check that a PR hasn't broken links. It would be a lot better for reviewers if links were resolved to https://developer.mozilla.org.

Give virtual trophies (🥇 🏆) or achievement badges upon completion of 20, 50, 100 commits. The gamification, like stackoverflow, may motivate young contributors more. 🤔

Really unsure about this. I think it would encourage people to file lots of low-value PRs.

[teoli2003]

💯

• Yep, don't slow down the PR Companion. The slower it gets, the less I'll use it.
• Lighthouse: I'm not sure the results are relevant for reference pages.
• Context-switch: For first-time contributors, I launch the tests before looking at the PR; that way, I prevent content switching. But sometimes it still happens.
• Virtual trophies: no way! We want quality, not quantity. Leave this to GitHub.
• 👍 about the "pr20182" links. I even thought of writing a web extension. Having the link to pages in the pr stay as "pr20182" and the other link to the live site would save me time.

[OnkarRuikar]

that I'm not even sure is possible. ... if links were resolved to developer.mozilla.org.

I think it's possible. We need to do simple search and replace before the deployment in pr-review-companion.yml:

    - name: Unzip what was downloaded
      run: |
        unzip build.zip -d build
        # update the links
        sed -i 's|/en-US/docs|https://developer.mozilla.org/en-US/docs|g' build/**/*.md

Note: for macros generated links i think there is an environment variable for base urls in yari. If not then the provision needs to be made.

Other option could be to deploy all the linked documents as well.

---

Another reason to be careful here is performance: the PR companion is already slow to run.

Recently we've added markdownlint to the checks and code lint may be the next.
Not sure if that'll will slow down the companion deployment.

---

I think it would encourage people to file lots of low-value PRs.

But we do accept even typos.

[teoli2003]

Another reason to be careful here is performance: the PR companion is already slow to run.

Recently we've added markdownlint to the checks and code lint may be the next. Not sure if that'll will slow down the companion deployment.

We are speaking about adding/removing information into the PR Review Companion messages, not other tests. Markdonwlint tests are in a different workflow.

I think it would encourage people to file lots of low-value PRs.

But we do accept even typos.

Yes, but reviewing 5-10 short typo/grammar PRs per day is not the same as 50-60… Having a full-time equivalent reviewer for commas (even if we reject them) is not a good use of reviewer time.

[Graywolf9]

Hello to everyone!

Personally I use the PR preview links more than other thing, I'm not sure about add or remove things, but could help making collapsible throught <details>

One thing that would help, that I'm not even sure is possible. In the preview pages, internal links in pages (like "/en-US/docs/Web/CSS/margin") get resolved to the preview base URL, like "https://pr20182.content.dev.mdn.mozit.cloud/". This means all links from a preview page that aren't to another page edited in the same PR are broken, which makes it hard to check that a PR hasn't broken links. It would be a lot better for reviewers if links were resolved to https://developer.mozilla.org/.

Also this could help to have a better flaws criteria

[OnkarRuikar]

Following work has been done recently to reduce the companion's comment size:

• fix(pr-companion): remove broken 'on GitHub' links yari#7089
• fix(pr-companion): remove redundant entries yari#7105
• fix(pr-companion): fold sections with count yari#7110

Now the comment won't be bigger than 9 lines:
Example 1: mdn/content#21006 (comment)

Example 2: mdn/content#20871 (comment)

---

GitHub's markdown is highly sanitized; inline attributes like style are removed. So, the options for styling are limited. We need to keep this in mind for further discussion.

[bsmth]

That's a really great improvement, well done 👍🏻

[Graywolf9]

It look awesome and much better, thank you so much!