Skip to content

Harden root-served localization deploy routing#230

Open
PrzemyslawKlys wants to merge 2 commits intomainfrom
codex/root-served-localization-deploy
Open

Harden root-served localization deploy routing#230
PrzemyslawKlys wants to merge 2 commits intomainfrom
codex/root-served-localization-deploy

Conversation

@PrzemyslawKlys
Copy link
Member

Summary

  • add first-class renderAtRoot support for root-served language deployments
  • rebase localized menus, rendered HTML links, hreflang, and sitemap alternates for languageAsRoot builds
  • preserve absolute external and special URLs while covering the split-domain behavior with localization tests

Testing

  • dotnet test PowerForge.Tests/PowerForge.Tests.csproj -c Release --filter "FullyQualifiedNameBuild_LocalizedPages_SupportSingleLanguageDomainStyleBuild|FullyQualifiedNameBuild_LocalizedPages_RebaseInternalLinks_ForRootLanguageBuild|FullyQualifiedNameBuild_LocalizedPages_EmitAlternateHeadLinks_ForRootServedLanguage|FullyQualifiedNameSitemap_Generate_EmitsLocalizedAlternates_WithRootServedLanguageBaseUrls"

@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude
Copy link

claude bot commented Mar 26, 2026

PR Review: Harden root-served localization deploy routing. Overall this is a well-structured feature addition with good test coverage of the main scenarios. A few issues worth addressing before merge. Bug: Regex in RewriteQuotedHtmlAttribute - the character class excludes both double-quote and single-quote from the value match regardless of which quote type opened the attribute; an href containing an apostrophe would be truncated. Dead code in IsLocalizedFallbackCopy - the final null check is unreachable. Performance: ResolveLocalizationConfig called per-attribute per-page - consider resolving once per render pass. Redundant null check in ResolvePublicRouteForLanguage. RootPath in ThemeRenderContext appears unused in this diff. Unrelated changes bundled in the PR: BuildCrawlMetaHtml refactor and the EmitXDefault test. Missing test coverage for collection-level filtering, RenderAtRoot=false regression, and single-quoted HTML attributes. Minor nit: duplicate StartsWith guard in RebaseRouteForSelectedLanguageRootBuild.

@PrzemyslawKlys
Copy link
Member Author

Follow-up pushed in

@claude
Copy link

claude bot commented Mar 26, 2026

PR Review: Harden root-served localization deploy routing

Good overall direction. The renderAtRoot feature fills a real gap for split-domain i18n deployments, and the test coverage is solid (4 new integration tests covering menus, hreflang links, sitemap alternates, and x-default fallback). A few things worth addressing:


Issues

1. Regex compiled on every call (WebSiteBuilder.OutputRendering.cs)

RewriteQuotedHtmlAttribute constructs a pattern string and calls Regex.Replace(html, pattern, ...) on every invocation. The .NET internal regex cache holds only 21 entries, and with 5 attributes x every rendered page that cache will be evicted frequently. Consider caching compiled Regex instances in a ConcurrentDictionary so the pattern is only compiled once per attribute name.


2. Redundant double-lookup in IsLocalizedFallbackCopy (WebSiteBuilder.CrawlPolicy.cs)

The method calls item.Meta.TryGetValue and then, only if that fails, calls TryGetMetaValue on the same dictionary with the same key. If TryGetMetaValue handles case-insensitive or nested lookups that the direct call cannot, the outer check is redundant for the exact-key case. Either call TryGetMetaValue directly, or add a comment explaining what the first check guards that the second cannot handle.


3. alternates.Count <= 1 changed to == 0 (WebSiteBuilder.RenderAssetsAndRouting.cs)

Previously a single resolved alternate suppressed all hreflang output. Now it emits both a language tag and x-default. The new test Build_LocalizedPages_EmitXDefault_WhenOnlyCurrentLanguageAlternateExists validates the new behavior, but search engines treat a page with one hreflang + x-default differently from no hreflang at all. Worth noting this behavioral change explicitly in the PR description.


4. LocalizedLanguages silently falls back to ExpectedTranslationLanguages (Navigation.LocalizationAndVersioning.cs)

ExpectedTranslationLanguages was a validation/warning field, not a routing control. Silently reusing it to restrict hreflang output could surprise users who set it only for completeness checks. Consider documenting this fallback in the XML doc comment, or requiring LocalizedLanguages to be set explicitly.


5. AutoImageDimensions added to schema without a visible model property

The schema diff adds AutoImageDimensions to the image hints config object, but no matching C# model property appears in this PR. If the property already exists and this is just a schema sync, a brief note would help reviewers. If it is a stray change from a different branch, it should be removed.


6. crawlMeta moved into HeadHtml -- verify no double-emit (WebSiteBuilder.OutputRendering.cs)

The new code joins crawlMeta, headHtml, and alternateHeadLinksHtml together. If headHtml already contains a robots meta tag from a theme template or earlier pipeline step, this could produce two robots meta tags in the same <head>. Worth verifying there is no double-emit in the forceFallbackNoIndex path.


Minor / Nit

  • RootPath in ThemeRenderContext: Added and populated, but no usage appears in the diff. Fine if consumed by Scriban templates; if unused, it can be removed.
  • Schema duplication of PascalCase + camelCase for the new fields (RenderAtRoot/renderAtRoot, etc.) is consistent with the rest of the file.
  • The IsNonRouteReference guard covering javascript:, data:, and blob: schemes is a good defensive addition.

Summary

The core routing logic is sound and the new tests cover the main scenarios well. The primary items to address: regex performance (matters for throughput on large sites), the IsLocalizedFallbackCopy double-lookup, and the untracked AutoImageDimensions schema change. The hreflang single-alternate behavior change and the ExpectedTranslationLanguages fallback both deserve clearer documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant