Fix localized API docs SEO URLs#348
Conversation
|
Code Review Overview This PR adds localized BaseUrl propagation into generated project API docs and suite landing pages, and fixes a bug where file/asset URLs were having trailing slashes incorrectly appended during language-root rebasing. The scope is focused and the changes are cohesive. Positive Observations
Issues and Suggestions 1. False-positive risk in LooksLikeFileRoute (potential bug) Path.GetExtension on URL segments can produce false positives for version-like path components. For example, Path.GetExtension for /api/v1.0 returns .0, so that URL would be treated as a file route and skip trailing-slash normalization even though it is a regular navigable page. A safer heuristic is to check against an explicit allowlist of known file extensions (.css, .js, .png, .woff2, .xml, etc.) rather than treating any non-empty extension as a match. 2. Repeated heap allocation in LooksLikeFileRoute (minor) new[] containing the query/hash delimiters allocates a new char array on every call. Use path.AsSpan().IndexOfAny or a static readonly field instead. 3. Feed URL coverage gap The PR description says preserve file asset/feed URLs when rebasing, and FeedSpec is enabled in the new test. However, the test does not assert that feed URLs like /feed.xml are preserved without trailing slashes, leaving that half of the stated fix without a regression test. 4. Inconsistent indentation in test markdown content (cosmetic) In Build_LanguageRootRebase_PreservesFileAssetUrls, the default-language index.md raw string has the heading and img tag flush-left while the YAML frontmatter is indented. The Polish version is consistent. Won't affect test correctness, but looks like a copy-paste oversight. Summary
The core change is solid. The main item worth addressing before merge is the Path.GetExtension false-positive risk for non-file URL segments that contain dots (e.g. version numbers like /api/v1.0). |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2addad36b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ["title"] = $"{name} API Reference", | ||
| ["out"] = outputPath, | ||
| ["baseUrl"] = apiBaseUrl, | ||
| ["siteBaseUrl"] = siteBaseUrl, |
There was a problem hiding this comment.
Setting siteBaseUrl on every generated input causes a regression when apiSiteBaseUrl is null: the per-project input now overwrites the parent project-apidocs step value during CreateMergedApiDocsStepDocument merge, so ExecuteApiDocs receives an empty siteBaseUrl and skips the configured/derived base URL fallback. This breaks canonical/OpenGraph absolute URL generation for pipelines that set siteBaseUrl explicitly (or rely on parent-level defaults) when site config cannot provide a language base URL.
Useful? React with 👍 / 👎.
| ["title"] = $"{name} API Reference", | ||
| ["out"] = outputPath, | ||
| ["baseUrl"] = apiBaseUrl, | ||
| ["siteBaseUrl"] = siteBaseUrl, |
There was a problem hiding this comment.
project-apidocs now writes siteBaseUrl into each generated child apidocs step even when apiSiteBaseUrl is null, and CreateMergedApiDocsStepDocument lets child properties override parent properties. In pipelines that set siteBaseUrl on the parent step (or rely on parent-level defaults), this clears that value and causes canonical/OpenGraph URLs to fall back to relative paths instead of absolute URLs.
Useful? React with 👍 / 👎.
| ["title"] = $"{name} API Reference", | ||
| ["out"] = outputPath, | ||
| ["baseUrl"] = apiBaseUrl, | ||
| ["siteBaseUrl"] = siteBaseUrl, |
There was a problem hiding this comment.
Setting siteBaseUrl unconditionally here also overrides any explicit siteBaseUrl configured on the parent project-apidocs step, because merged child inputs win over parent properties. That is a behavior regression: callers can no longer force a deployment-specific canonical host (for example prod/staging host overrides) once a site.json is present, since this line always replaces the parent value.
Useful? React with 👍 / 👎.
| if (LooksLikeFileRoute(publicRoute)) | ||
| return publicRoute; |
There was a problem hiding this comment.
Using Path.GetExtension to classify file routes treats dotted directory-like routes (for example /docs/v1.0) as files, so trailing-slash normalization is skipped in localization rebasing. This conflicts with existing dotted-route directory behavior in the repo and can emit inconsistent public URLs/canonicals for versioned docs paths when inputs are provided without a trailing slash.
Useful? React with 👍 / 👎.
| ["title"] = $"{name} API Reference", | ||
| ["out"] = outputPath, | ||
| ["baseUrl"] = apiBaseUrl, | ||
| ["siteBaseUrl"] = siteBaseUrl, |
There was a problem hiding this comment.
This always writes a siteBaseUrl property into each generated child apidocs input, so when apiSiteBaseUrl resolves to null/empty the merged child step overwrites the parent project-apidocs setting and clears the value that ExecuteApiDocs would otherwise use. In pipelines that provide siteBaseUrl at the parent step (or rely on parent-level defaults), canonical/OpenGraph URLs regress from absolute URLs to relative ones.
Useful? React with 👍 / 👎.
| ["title"] = $"{name} API Reference", | ||
| ["out"] = outputPath, | ||
| ["baseUrl"] = apiBaseUrl, | ||
| ["siteBaseUrl"] = siteBaseUrl, |
There was a problem hiding this comment.
Because child inputs now unconditionally set siteBaseUrl, any explicit siteBaseUrl configured on the parent project-apidocs step is silently replaced by the language-derived value from site.json. That breaks environment-specific host overrides (for example staging/prod canonical host differences) that previously flowed through parent defaults into each generated apidocs run.
Useful? React with 👍 / 👎.
| return NormalizeOptionalString(ResolveProjectApiLanguageSpec(siteSpec, targetLanguage)?.BaseUrl) ?? | ||
| NormalizeOptionalString(siteSpec.BaseUrl); |
There was a problem hiding this comment.
The new helper always prefers the target language BaseUrl without checking whether that language is actually supported for the routed collection, while other project link generation paths explicitly fall back to default-language URLs when a collection is unsupported. This can produce mixed-domain metadata (canonical/social host on one language domain while related project tabs intentionally fall back to default-domain URLs) in partially localized configurations.
Useful? React with 👍 / 👎.
| if (queryIndex >= 0) | ||
| path = path[..queryIndex]; | ||
|
|
||
| return !string.IsNullOrWhiteSpace(Path.GetExtension(path)); |
There was a problem hiding this comment.
Using Path.GetExtension as the file/route discriminator classifies dotted directory routes like /docs/v1.0 as files, so trailing-slash normalization is skipped. That conflicts with the repository’s existing dotted-route directory behavior and can generate inconsistent localized/public URLs and canonicals for versioned docs paths when inputs are provided without a trailing slash.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Build_LanguageRootRebase_PreservesFileAssetUrls|FullyQualifiedNameBuild_WritesRoot404HtmlForNotFoundSlug|FullyQualifiedName~RunPipeline_ProjectApiDocs_LocalizesProjectTabsAndFallsBackToDefaultLanguageForUnsupportedCollections" --no-restore