feat(husky): check deletions and broken fragments in URLs

[OnkarRuikar]

The PR adds following features:
a. Detects file deletions and checks if corresponding slugs are being used in rest of the content. Logs the files, line, column, and message.
b. Detects markdown header updates and reports any URL fragments in content that refers the headers.

Above features are enforced in following way:

1. Husky - executes both the a. and b. features mentioned above during precommit and fails if any cases found.
2. GitHub PR check - executes feature b.. Tags broken places in content and fails the workflow. Demo PR for this can be found here.

ping @teoli2003

[github-actions]

Preview URLs

• /en-US/docs/MDN/Writing_guidelines/Page_structures/Syntax_sections
• /en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/setUninstallURL

External URLs (1)

URL: /en-US/docs/MDN/Writing_guidelines/Page_structures/Syntax_sections
Title: Syntax sections

• https://en.wikipedia.org/wiki/Backus%E2%80%93Naur_form (1 time) (Note! This may be a new URL 👀)

(comment last updated: 2024-01-09 04:17:00)

[Josh-Cena]

How fast is this?

Anything run in Husky shouldn't take more than a few milliseconds.

[OnkarRuikar]

Anything run in Husky shouldn't take more than a few milliseconds.

In my setup it takes less than 3 seconds.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[Josh-Cena]

"less than 3 seconds" still sounds like a lot, though, compared to Prettier. Does it depend on the input files? The key thing about lint-staged is that it's only supposed to care about those particular files, but if the performance is irrelevant of the input size then I think it should be CI-only.

[OnkarRuikar]

"less than 3 seconds" still sounds like a lot, though, compared to Prettier. Does it depend on the input files?

It is worse case time taken only if you've deleted a file or modified a markdown header, which we don't do that often. :O Yes it does depend on input files (taken from git status and git diff commands), so in normal case it completes in no time.

The key thing about lint-staged is that it's only supposed to care about those particular files, but if the performance is irrelevant of the input size then I think it should be CI-only.

The mdn/yari precommit takes 1m34s in the same setup. Because it does complete yarn install in the hook.

It is a small prices to pay to eliminate the broken fragments and broken links. At the moment there are thousands of such issues in content. Trust me you don't want to fix these later manually one by one.

---

Should we also add yarn install in mdn/content precommit hook?

[Josh-Cena]

Again, a precommit hook should not take more than a few milliseconds for one or two modified files. I have no comments for Yari—it is already a mess in terms of architecture in my eyes. But let's not make development worse for everyone working on content, which is a 10x larger body. People come from different environments and have different git habits. Many people are accustomed to "commit small and often" and we need to make the process smooth for them without forcing them to turn off precommit hooks, which defeats the purpose. Again, unless the problem is either immediately detectable or autofixable, my opinion is to defer it to the CI. CI exists so one doesn't have to duplicate the check on their own machine.

[rebloor]

r+ for the text changes

[hamishwillee]

FWIW, just before I remove myself as a reviewer ...

• Docs changes good

• If reviewer is expected to fix only the changes in docs they touched this looks good.

• Agree we need this or something like it - so easy to break stuff by changing a heading name, or just omitting the anchor #.

• I prefer to know about broken things earlier rather than later. While I have generally found it painful in BCD to not be able to submit incorrect changes when still working on stuff, most of the time it is better.

• In the linked PR the problem links is

  [coerced to a boolean](/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean#boolean_coercion)

  Messages are:
  Check failure on line 24 in files/en-us/web/javascript/reference/global_objects/symbol/hasinstance/index.md

  GitHub Actions
   / check_url_issues
   URL fragment 'web/javascript/reference/global_objects/boolean#boolean_coercion' is broken
  

  Will users know that it really is just the boolean_coercion they need to worry about (not the whole URL) and that this is likely a missing heading? And can/should we spell it out?

  Anchor #boolean_coercion missing in linked URL fragment 'web/javascript/reference/global_objects/boolean#boolean_coercion'.
  (Does heading "Boolean coercion" exist?)

[rebloor]

@OnkarRuikar I'm not the right person to review technical changes, so have requested a review from mdn/yari-content-mozilla-add-ons

[OnkarRuikar]

URL fragment 'web/javascript/reference/global_objects/boolean#boolean_coercion' is broken

Will users know that it really is just the boolean_coercion they need to worry about (not the whole URL) and that this is likely a missing heading?

Yes, it's about just the fragments.

And can/should we spell it out?

I've changed it to
URL fragment in URL 'web/javascript/reference/global_objects/boolean#boolean_coercion' is broken

[Rob--W]

If it is going to take a while before this PR is merged, then I recommend splitting the content fixes from the CI automation introduced here.

@OnkarRuikar I'm not the right person to review technical changes, so have requested a review from mdn/yari-content-mozilla-add-ons

I usually add/review content in WebExtensions documentation, and there is only one-character trivial fix for that in this PR. While I have the skills to evaluate the PR code, I am not sure if I have the authority to sign off on the addition of CI scripts. Therefore I'll remove myself a reviewer. Feel free to add me back if needed.

[teoli2003]

Anybody against merging this? If not, I'll move forward later this week.

[Rob--W]

Anybody against merging this? If not, I'll move forward later this week.

So far the only approvals are in the changed documentation. The PR has not received any review of the actual code, so I don't think that all of the PR is ready to merge.

I recommend looking for someone who has the skills and authority to sign off on code changes. If nobody is available for that, I am willing to offer a code review in this case to unblock the PR.

[bsmth]

Anything run in Husky shouldn't take more than a few milliseconds.

In my setup it takes less than 3 seconds.

What's the best way to test this? Check out the branch, stage some files with broken URL fragments in xrefs and try to commit?

[OnkarRuikar]

What's the best way to test this? Check out the branch, stage some files with broken URL fragments in xrefs and try to commit?

You'll have to do something like following:

gh pr checkout 31265
# find some popular URL
yarn content delete <slug> --redirect <slug>
git commit -am "test delete"
# the commit should fail and husky would have listed all the files with issues
# after done testing to revert deletion
git checkout .

[teoli2003]

It worked locally on my side. Let's merge this as we are at the beginning of the week.

[teoli2003]

👍