Uh oh!
There was an error while loading. Please reload this page.
Fix two duplicate/hreflang defects in multi-domain sitemaps - #782
Fix two duplicate/hreflang defects in multi-domain sitemaps#782timujinne wants to merge 2 commits into
Conversation
Both surfaced on a live three-domain site (en primary, de, fr), where the generated per-host sitemaps disagreed with what the rendered pages emit. A single-language group carried a lone self+x-default hreflang pair. The page-level builders already treat an under-2-entry set as noise and emit nothing, so the sitemap advertised an alternate set the page never backed up. group_alternates/4 now returns [] for such a group. A host could also list the same <loc> twice. A locale-prefixed clone route (/de, /fr pointing at the home LiveView) is discovered by RouterDiscovery with no canonical_path, so it forms its own group; re-hosting strips the prefix and lands it on the home URL the static source already placed on that language's domain. The two groups cannot merge, so the file listed the home twice. Rather than add a second dedup policy, the one the flat generator already applied moves into UrlEntry.dedupe_by_loc/1 + UrlEntry.richness/1, and both callers share it. That preserves the established rule that a RouterDiscovery entry always loses a same-loc collision — a priority-only comparison would not have kept it. Also restores coverage the guard silently voided: the existing m2 assertion (no x-default when a group lacks the primary's language) ran against an empty alternates list and so passed vacuously. It now asserts the empty list it actually exercises, and a new two-language test pins m2 itself.
cbba2ed to
3ef2174Compareddon
commented
Sep 5, 2026
Still draft, not touching |
timujinne
left a comment
There was a problem hiding this comment.
Independent review — verdict: ready, with four cosmetic items
Reviewed at head 932ca122 (code commit 3ef2174a), base a9a66190. No commit on main since the base touches lib/modules/sitemap/ or test/integration/sitemap/, so there is no dead-code-base risk.
Spec: pass — and checked by running, not by reading the diff
Both promised defects exist, sit where the description says, and are fixed there. This was established by running the PR's test file against pre-fix code:
- Defect 1 —
domain_mode.ex:227-228. Base emits a lone hreflang for a single-language group:assert fr_entry.alternates == []failed withleft: [%{href: "https://site.example.fr/only-french", hreflang: "fr"}]. - Defect 2 —
domain_mode.ex:140. Base lists the same<loc>twice: one bare entry (canonical_path: nil, alternates[fr]) and one rich (canonical_path: "/", alternates[en, fr, x-default]). A single failure dump exhibits both defects at once.
Nothing missing, wrong, or extra against the stated spec.
Findings — no critical, no major
1. The description understates the scope of fix #1. The body says the lone pair was emitted "whenever that language was the primary domain's". The guard is map_size(by_lang) < 2, so it also strips the lone self-reference from single-language groups in non-primary languages — the pre-fix failure above is an fr (non-primary) group changing [{hreflang: "fr"}] → []. The behaviour is correct; the description lags it, and a maintainer reading the body would not expect non-primary URLs to change. One sentence fixes it.
2. The moduledoc still states the old two-rule alternates contract.domain_mode.ex:14-27 says the set is "each group language on its home URL, plus x-default … (omitted otherwise)". The new third rule — under two languages ⇒ no alternates at all — is documented only in a private-function comment. Verified: the diff contains no @moduledoc change. For a published library the moduledoc is where the contract is read.
3. A private customer application is named in library code.domain_mode.ex:220 introduces Decor3dprintWeb.SEO.with_x_default/1 in a comment. This is the only named private application anywhere in lib/: a survey of every external module name appearing in lib/ comments (42 after removing modules defined here and stdlib) splits cleanly into BeamLab-family siblings, third-party dependencies, and deliberate generic placeholders for the host app — YourApp, AppName, AppWeb, HostWeb. The repository's own convention for talking about a host app is AppWeb/YourApp, so this is a one-word change. Keep the rule it illustrates ("an hreflang set of fewer than two entries is not a set"), and keep the PhoenixKitEcommerce.Web.SEOHelpers half — that is a documented sibling already referenced at lib/phoenix_kit_web/users/auth_seo.ex:6,24.
Caveat: the survey covered # comments in lib/ only, not @doc/@moduledoc bodies.
4. "76 tests, 0 failures" in the body is 77. Base sitemap suite is 74; the diff adds three tests (not two — the m2 test, the primary-single-language test, and the duplicate-home test).
Nitpick.domain_entries/5 still ends |> Enum.sort_by(& &1.loc) at domain_mode.ex:213, but every consumer re-sorts after dedup at line 140 — dead work. Note also that CLAUDE_REVIEW.md states "both now sort by loc on the same key after dedup", implying the inner sort was removed; it was not.
Boundary of the chosen fix — not a regression
Dedup resolves the duplicate <loc> but does not reconcile the two hreflang sets. When two rich canonical groups collide on one URL, the loser's group keeps advertising the loc it lost:
site.example.com/b alts=[{en, …/b}, {fr, https://site.example.fr/a}, {x-default, …/b}]
site.example.fr/a alts=[{en, …/a}, {fr, https://site.example.fr/a}, {x-default, …/a}] ← never mentions /b
…com/b claims its French version is …fr/a, but that page's own set describes group /a and never points back. Reachable only from malformed content data, and the pre-fix behaviour — the same loc listed twice with conflicting sets — was no better. Worth knowing before anyone assumes dedup makes hreflang self-consistent.
Same defect class, one layer up — pre-existing, outside this PR
generator.ex:730-762 (collect_multilingual_entries/3) builds flat-mode alternates with no under-2 guard: a single-entry canonical group gets [self, x-default], and default_entry falls back to || List.first(entries) at line 746, which can label a non-default language as x-default — precisely the rule the DomainMode moduledoc says must never be violated. That is the non-domain-mode path, i.e. most installs. The PR's own rationale — the sitemap must not promise what the page's <head> does not back up — applies to it verbatim. Traced by reading, never executed.
Checked and found correct
- The guard cannot drop URLs:
locandalternatesare computed independently (domain_mode.ex:204-210).home_url/5always returns a URL, so the link count always equalsmap_size(by_lang)and the guard's threshold is exactly right. UrlEntry.dedupe_by_loc/1andrichness/1are a faithful move of the Generator originals; both call sites updated; no other callers; the ":router_discoveryalways loses" rule survives the move.- Determinism holds:
Enum.max_bykeeps the first on a tie,Enum.group_bypreserves arrival order. - Reciprocity holds by construction for well-formed input — one alternates set per group, applied identically to every member. Dedup never orphans an hreflang target: the survivor carries the same
loc. mix format --check-formattedclean on all four touched files;mix credo --stricton the three changed lib files — 89 modules/functions, no issues;MIX_ENV=test mix compile --warnings-as-errors --forceclean.
Not verified — stated plainly
- Anything on the live three-domain install. Every number in the body's Verification section that refers to the running site — 639→638, 643 unchanged, "untranslated URLs now carry no hreflang block", "the surviving home entry is the rich one" — rests on the original author's report alone. I had no access to that install and did not attempt to get it. If those numbers are meant to be trusted, they are still one person's word.
- The premise about the page-level builders. Neither
Decor3dprintWeb.SEO.with_x_default/1norPhoenixKitEcommerce.Web.SEOHelpers.dedup_or_empty/1is defined in this repository. Only the premise was checked — that a one-entry hreflang set is meaningless per the spec — not that those two functions behave as the comment claims. - The flat-path twin at
generator.ex:730-762— read, never executed. It is private and driving it needs real sources plus multi-language collection. - Dialyzer. CI runs it; this review did not. Type-level regressions from the new
@specs ondedupe_by_loc/1andrichness/1are unverified. - Whether promoting
dedupe_by_loc/1andrichness/1to documented public functions is an API-surface expansion you mind.UrlEntryalready exposes documented helpers such asparse_priority/1, so it matches local style — but that call is yours, and it is flagged so it is not made silently.
Out of scope, pre-existing
domain_mode.ex:190-192 — dialect collapse can lose a URL. Noted as ambiguous and not touched by this PR.
ddon
commented
Sep 6, 2026
@timujinne still a draft, waiting it to be finished |
Two defects in multi-domain sitemap generation, both found on a live three-domain site (en primary + de + fr) where the generated per-host sitemaps disagreed with what the rendered pages actually emit.
1. Lone self+x-default hreflang on a single-language URL
DomainMode.group_alternates/4emitted a self +x-defaultpair for a canonical-path group containing only one language, whenever that language was the primary domain's. That is a two-entry hreflang "set" for a page with no translations.The page-level builders already treat an under-2-entry set as noise and emit nothing, so the sitemap advertised an alternate set that the page's own
<head>never backed up — a crawler following the sitemap saw a promise the page did not keep.group_alternates/4now returns[]for such a group.Because the function is shared by every source via
rebuild_for_domains/2, this affected shop products and publishing posts alike, not one source.2. The same
<loc>listed twice in one host's fileA locale-prefixed clone route (e.g.
live "/de"pointing at the home LiveView, so/dedoes not 404 on a prefix install) is picked up byRouterDiscoverywith nocanonical_path, so it forms its own group. Re-hosting strips the prefix and lands it onhttps://<de-host>/— exactly where the static source has already placed that domain's home. The two groups cannot merge, so the file listed the home twice: once with the full cross-domain alternates, once bare.Per-host entries are now deduplicated by
<loc>, keeping the richest description of the URL: alternates first, then priority, then arrival order so the result is deterministic run to run.extra_for_primary/5already enforced this same "the same<loc>in one file twice" rule across sources for the domainless case — it simply never applied within a host's own set, which is where the clone-route duplicate lands.Verification
Regression tests added for both cases in
test/integration/sitemap/domain_mode_test.exs. Full sitemap integration suite: 76 tests, 0 failures.mix formatclean.Confirmed against a live three-domain install by regenerating the sitemaps:
Note
Version and CHANGELOG are intentionally left untouched for the maintainer.