Support cross-repository redirects and specialized anchor scenarios #1227
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for cross‐repository redirects and enhanced anchor handling in Markdown links.
- Updated HtmxLinkInlineRenderer to differentiate between cross links and HTTP links.
- Refactored CrossLinkResolver to improve redirect resolution and error reporting.
- Enhanced URI validation in DocumentationSet and updated documentation for redirects.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Elastic.Markdown/Myst/Renderers/HtmxLinkInlineRenderer.cs | Introduces new flags (isCrossLink, isHttpLink) and refines hx attribute assignment based on link types. |
| src/Elastic.Markdown/Links/CrossLinks/CrossLinkResolver.cs | Refactors redirect resolution logic with clearer variable naming and improved handling of anchors and lookup paths. |
| src/Elastic.Markdown/IO/DocumentationSet.cs | Adds URI validation for external redirect targets. |
| docs/contribute/redirects.md | Updates documentation with examples on cross-repository redirects and complex anchor scenarios. |
| continue; | ||
| if (!linkReference.Links.TryGetValue(redirect.To, out link)) | ||
| continue; | ||
| var lookupPath = Path.Combine(targetCrossUri.Host, targetCrossUri.AbsolutePath.TrimStart('/')); |
There was a problem hiding this comment.
Using Path.Combine for URL construction might introduce platform-specific path separators. Consider using string concatenation or a URI-specific method to ensure consistent forward slash formatting in URLs.
| var lookupPath = Path.Combine(targetCrossUri.Host, targetCrossUri.AbsolutePath.TrimStart('/')); | |
| var lookupPath = $"{targetCrossUri.Host}/{targetCrossUri.AbsolutePath.TrimStart('/')}"; |
Copilot uses AI. Check for mistakes.
| 'item-a': 'yy' | ||
| - to: 'testing/redirects/third-page.md' | ||
| anchors: | ||
| 'item-b': |
There was a problem hiding this comment.
The anchor mapping for 'item-b' is empty, which could be unintentional. Confirm whether this is the desired behavior or if an explicit null/empty value should be specified for clarity.
| 'item-b': | |
| 'item-b': '!' |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Nice work!
A few nits about leniency. Can you also add more tests to https://github.com/elastic/docs-builder/blob/da90aa5d28612d4b9871462a4647cd41bb442494/tests/authoring/Inline/CrossLinksRedirects.fs ?
|
|
||
| var declaredRepositories = fetchedCrossLinks.DeclaredRepositories; | ||
| if (!declaredRepositories.Contains(crossLinkUri.Scheme)) | ||
| if (crossLinkUri.Scheme.Equals("asciidocalypse") || !fetchedCrossLinks.LinkReferences.TryGetValue(crossLinkUri.Scheme, out var sourceLinkReference)) |
There was a problem hiding this comment.
I think its time to remove this leniency. All repositories are now publishing links.json files for a long while.
docs/contribute/redirects.md
Outdated
| @@ -49,6 +49,19 @@ redirects: | |||
| 'testing/redirects/third-page.md': | |||
| anchors: | |||
| 'removed-anchor': | |||
| 'testing/redirects/cross-repo-page.md': 'other-repo://reference/section/new-cross-repo-page.md' | |||
| 'testing/redirects/8th-page.md': | |||
| to: 'other-repo://testing/redirects/5th-page.md' | |||
There was a problem hiding this comment.
With my other comment about removing leniency this should no longer be allowed. We should only link to published remote links.
Closes #670, #847
This MR introduces support for cross-repository redirects, and support for more complex anchor scenarios.