Match breadcrumb parents on same host and whole path segments - #373
Open
pxul wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #372
The "section breadcrumb" rule in
removeByContentPatterndecides whether a link points at a parent section of the current page by testingurlPath.startsWith(linkPath).That check has two faults. It compares pathnames and never the host, so a link to another site is eligible. It also lets the match stop part-way through a segment, so
/acmecounts as a parent of/acmelabs/posts/12345. Either fault deletes a link along with its text, and nothing in the output indicates the removal.The changes here are in two commits, one per missing guard, so either can be dropped.
1. Compare the host
A link to any site was eligible for deletion, because
linkPathis only a pathname. A page athttps://pages.example.com/acmelabs/posts/12345would delete a link tohttps://social.example.net/acme.pageHostwas already computed and went unused until the trailing external-link rule further down, so the new guard reuses it.The rule's block comment listed the matching patterns without ever saying the link must be same-site, so a line has been added for that.
2. Require a whole-segment match
startsWithcan match partial segments, so/acmewas treated as a parent of/acmelabs/posts/12345.The page path is now tested against the link path with a trailing
/, so a match has to end on a segment boundary:/acmelabs/posts/12345starts with/acmelabs/, but not with/acme/. A link path already ending in/is used as it stands, so the slash is never doubled.Pattern 1 in the block comment now says "whole-segment path prefix". The counter-example sits in the inline comment beside the check rather than being repeated in the block.
Behaviour
/acmelabs/posts/12345/acmelabs/acmelabs/posts/12345/acmelabs//acmelabs/posts/12345/acmelabs/posts/acmelabs/posts/12345/acmelabs/posts//acmelabs/posts/12345/acme/acmelabs/posts/12345/ac/acmelabs/posts/12345https://social.example.net/acmeTests
tests/breadcrumb-removal.test.ts, three cases reporting as six tests:The third case guards the other two: a boundary check tightened too far would keep every link, and only this case would notice. It also checks the surrounding article is still there, because all its assertions are about links being gone, and those would pass even if extraction returned nothing at all.
The first two cases were each confirmed failing before their fix. The third passes on both sides by design, since it asserts behaviour the rule already had.
The full suite passes, including the two fixtures that cover what this rule is meant to do:
content-patterns--leading-breadcrumb.htmlandgeneral--back-nav-link.html. Both link within the site (/archive,../index.html), so neither is affected.Not included
Trailing-slash equivalence. The
linkPath !== urlPathguard exempts a link to the page itself, but only on an exact string match, so a page at/blog/still loses a link to/blog. Whether/aand/a/should count as one resource reads as a policy question rather than a bug, and it may bear on other rules, so it is left alone here.A helper for
/^www\./. The new host guard stripswww.inline, matching the two existing uses in this file and four more acrosssrc/metadata.tsand two extractors. A shared utility would spread this diff over files that have nothing to do with breadcrumbs, so the existing style is followed instead.Happy to follow up on either if you would like.