Fix LinkPreview dropping links by truncating before deduplication#2032
Fix LinkPreview dropping links by truncating before deduplication#2032jichaowang02-lang wants to merge 1 commit into
Conversation
LinkPreview._filter_links applied the max_links limit before removing duplicate URLs. Link extraction routinely yields the same href many times (repeated nav / footer / CTA links), so the [:max_links] slice spent the budget on duplicate copies and the subsequent dedup then collapsed them, returning far fewer than max_links unique URLs. With three duplicate copies of one URL at the head of the list and max_links=3, the result was a single URL instead of three distinct ones. Deduplicate first, then apply max_links so the limit counts distinct URLs. Adds a TestFilterLinksDeduplication regression: the dedup-before-limit case fails on the old code and passes with the fix.
chuenchen309
left a comment
There was a problem hiding this comment.
The ordering issue is real — I reproduced it directly against _filter_links:
links = Links(internal=[Link(href=h) for h in ["a","a","a","b","c","d"]], external=[])
cfg = LinkPreviewConfig(include_internal=True, include_external=False, max_links=3)
lp._filter_links(links, cfg)
# main → ['a'] (slice takes the three "a"s, dedup collapses them)
# this PR → ['a','b','c']So the fix is correct and the direction is right.
One thing worth flagging for whoever merges this, because it changes how the PR should be framed: I don't think the stated cause is reachable through the library's own scraping path. LXMLWebScrapingStrategy dedupes at insert, keyed by the normalized href, and stores that same value as href:
link_data = {"href": normalized_href, ...}
...
if normalized_href not in internal_links_dict:
internal_links_dict[normalized_href] = link_dataand both Links construction sites (content_scraping_strategy.py:166 and :917) build from list(internal_links_dict.values()). So links.internal already has unique href values by construction.
Checking that against the PR's stated trigger — repeated nav/footer links:
html = """<nav><a href="/a">A</a><a href="/b">B</a><a href="/c">C</a><a href="/d">D</a></nav>
<main><a href="/a">A again</a><a href="/a">A third</a></main>
<footer><a href="/a">A footer</a><a href="/b">B footer</a></footer>"""
LXMLWebScrapingStrategy().scrap("https://example.com/", html)
# internal hrefs → ['https://example.com/a', '.../b', '.../c', '.../d']
# 4 links, 4 distinct — /a appears 4x in the HTML and /b 2x, and neither duplicatesSo a normal crawl won't lose links to this, and the dedup loop inside _filter_links is effectively already redundant for that path.
That doesn't make the change useless — Links is a public model, anyone constructing it directly (or a future scraping strategy that doesn't dedupe) would hit exactly the behavior above, and dedup-then-limit is the ordering the existing dedup loop clearly intends. I'd just suggest not describing it as links being dropped in real crawls, since that part doesn't hold today.
Not a maintainer here, just a fellow contributor who spends time in this area — take or leave as useful. (Disclosure: I use an AI coding assistant; the two snippets above are ones I ran myself against main.)
Summary
LinkPreview._filter_linksapplies themax_linkslimit beforededuplicating, so the limit is spent on duplicate copies of the same URL
instead of distinct URLs.
Link extraction routinely produces the same
hrefmany times (repeatednav / footer / CTA links), so the
[:max_links]slice keeps duplicate copiesand the later dedup collapses them — yielding far fewer than
max_linksunique URLs.
Example
Internal hrefs
["a", "a", "a", "b", "c", "d"]withmax_links=3:["a", "b", "c"](3 distinct)["a"](slice takes the 3as, dedup → 1)Fix
Deduplicate first, then apply
max_links, so the limit counts distinctURLs. Order is still preserved, and cases where the unique count is already
below
max_linksare unchanged.Testing
Adds
TestFilterLinksDeduplicationtotests/test_merge_head_data_scoring.py:The dedup-before-limit case fails on the current code and passes with the
fix; the "fewer uniques than the limit" and "cap distinct URLs" cases are
preserved.