fix(cli-utils): map cross-component doc URLs to qualified xrefs in urlToXref - #222
fix(cli-utils): map cross-component doc URLs to qualified xrefs in urlToXref#222JakeSCahill wants to merge 4 commits into
Conversation
…lToXref urlToXref treated the first path segment of every docs.redpanda.com URL as a module in the current Antora component. Cross-component URLs such as https://docs.redpanda.com/redpanda-connect/configuration/secrets/ produced the broken xref:redpanda-connect:configuration/secrets.adoc in generated CRD and Helm docs. Add a verified URL-slug-to-component map so cross-component URLs emit fully qualified xref:component:module:page.adoc resource IDs, with correct colon separation between module and page. Legacy /docs, /vX.Y, and /current prefixed URLs keep the existing same-component behavior. Prevents regression of the link hand-fixed in redpanda-data/docs#1830.
✅ Deploy Preview for docs-extensions-and-macros ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
__tests__/cli-utils/convert-doc-links.test.js (1)
56-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid cross-component coverage; a few map entries and the "beta" version segment go untested.
Traced every assertion here against the new logic — all pass as expected. Coverage is missing for
VERSION_SEGMENT_RE'sbetaalternative and for the untestedCOMPONENT_SLUG_MAPentries (agentic-data-plane,home,data-platform,self-managed). Since a wrong map entry silently produces a broken xref (the exact class of bug this PR fixes), a couple more cheap assertions would close the gap.✅ Suggested additional cases
+ it('strips a beta version segment after the slug', () => { + expect( + urlToXref('https://docs.redpanda.com/streaming/beta/manage/kubernetes/manage-resources/') + ).toBe('xref:streaming:manage:kubernetes/manage-resources.adoc'); + }); + + it('maps the home umbrella component', () => { + expect(urlToXref('https://docs.redpanda.com/home/')).toBe('xref:home::index.adoc'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/cli-utils/convert-doc-links.test.js` around lines 56 - 120, Extend the cross-component URL tests around urlToXref to cover VERSION_SEGMENT_RE’s beta version form and add assertions for the unmapped COMPONENT_SLUG_MAP entries agentic-data-plane, home, data-platform, and self-managed. Verify each URL produces the expected component-qualified xref, preserving the existing test style.cli-utils/convert-doc-links.js (1)
84-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrectness verified; consider extracting the cross-component branch for readability.
Manually traced this against every new test case (component-only, module-only, deep paths, streaming version-stripping, legacy-prefix precedence) — all outputs match expectations. This block is the fix for the docs#1830 class of bug, so keeping it easy to reason about pays off for future maintenance. Consider extracting the "resolve component" and "build cross-component xref" steps into small named helpers.
♻️ Proposed extraction
- // Build module + path + .adoc - let xref; - const component = !hadLegacyPrefix && segments.length > 0 - ? COMPONENT_SLUG_MAP[segments[0]] - : undefined; - if (segments.length === 0) { - xref = 'xref:index.adoc'; - } else if (component) { - // Cross-component URL: emit a fully qualified resource ID. - segments.shift(); - // Drop a version segment that may follow the slug (for example - // /streaming/current/manage/...) - if (segments.length > 0 && VERSION_SEGMENT_RE.test(segments[0])) { - segments.shift(); - } - if (segments.length === 0) { - xref = `xref:${component}::index.adoc`; - } else { - const moduleName = segments.shift(); - const fileName = (segments.length > 0 ? segments.join('/') : 'index') + '.adoc'; - xref = `xref:${component}:${moduleName}:${fileName}`; - } + // Build module + path + .adoc + let xref; + const component = !hadLegacyPrefix ? COMPONENT_SLUG_MAP[segments[0]] : undefined; + if (segments.length === 0) { + xref = 'xref:index.adoc'; + } else if (component) { + xref = buildCrossComponentXref(component, segments); } else { const moduleName = segments.shift(); const pagePath = segments.join('/'); const fileName = (pagePath || moduleName) + '.adoc'; xref = `xref:${moduleName}:${fileName}`; }// Cross-component URL: emit a fully qualified resource ID, dropping the // slug and an optional trailing version segment (e.g. /streaming/current/...). function buildCrossComponentXref(component, segments) { segments.shift(); if (segments.length > 0 && VERSION_SEGMENT_RE.test(segments[0])) { segments.shift(); } if (segments.length === 0) { return `xref:${component}::index.adoc`; } const moduleName = segments.shift(); const fileName = (segments.length > 0 ? segments.join('/') : 'index') + '.adoc'; return `xref:${component}:${moduleName}:${fileName}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli-utils/convert-doc-links.js` around lines 84 - 112, Extract the cross-component xref construction from the main conversion flow into a named helper such as buildCrossComponentXref, preserving slug removal, optional version-segment stripping, and component-only, module-only, and deep-path outputs. Keep the existing hadLegacyPrefix/component resolution and ordinary index handling in the surrounding logic, and call the helper only when a mapped component is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/cli-utils/convert-doc-links.test.js`:
- Around line 56-120: Extend the cross-component URL tests around urlToXref to
cover VERSION_SEGMENT_RE’s beta version form and add assertions for the unmapped
COMPONENT_SLUG_MAP entries agentic-data-plane, home, data-platform, and
self-managed. Verify each URL produces the expected component-qualified xref,
preserving the existing test style.
In `@cli-utils/convert-doc-links.js`:
- Around line 84-112: Extract the cross-component xref construction from the
main conversion flow into a named helper such as buildCrossComponentXref,
preserving slug removal, optional version-segment stripping, and component-only,
module-only, and deep-path outputs. Keep the existing hadLegacyPrefix/component
resolution and ordinary index handling in the surrounding logic, and call the
helper only when a mapped component is found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52e7a6e8-4315-473f-843e-667bb872b3dd
📒 Files selected for processing (2)
__tests__/cli-utils/convert-doc-links.test.jscli-utils/convert-doc-links.js
Deep links like .../cluster-properties/#kafka_batch_max_bytes lost their anchor: urlToXref built the xref from url.pathname only and silently dropped url.hash, so converted links landed at the top of the page instead of the referenced property or section. Found during end-to-end testing against the real connect 4.103.0 data dump, which carries fragment-bearing docs URLs. The fragment is appended to the xref target before the label, matching AsciiDoc's xref:module:page.adoc#fragment[label] form. Pre-existing on main, fixed here because this branch already reworks urlToXref.
|
Pushed a follow-up commit (499b073) fixing a defect found during the end-to-end test of this PR against the real connect 4.103.0 data: Fragments now append to the xref target before the label ( |
…p entries CodeRabbit review nitpick: VERSION_SEGMENT_RE's beta alternative and the agentic-data-plane, home, data-platform, and self-managed map entries had no assertions. A wrong map entry silently produces a broken xref, which is the class of bug this branch fixes, so each entry now has one.
|
CodeRabbit's two nitpicks addressed in d6b446e: added assertions for the |
|
Reviewed the logic against the actual Fix before merge1. // redpanda-labs (docs/antora.yml name: labs)
'redpanda-labs': 'labs', // legacy slug
'labs': 'labs',The only $ find . -name antora.yml -not -path './node_modules/*' | while read f; do printf "%-24s %s\n" "$f" "$(grep -E '^name:' "$f")"; done
./docs/antora.yml name: redpanda-labsSo Both slugs should map to 2. Component-only URLs assume a ROOT-module
Of the mapped components, only SuggestionThe "first segment after the slug is a module" heuristic misfires on ROOT-module pages. docs-site sets
What works well
|
Review finding: xref:<comp>::index.adoc assumes a ROOT index.adoc. connect, cloud-data-platform, streaming, and agentic-data-plane have none — their antora.yml start_page is home:index.adoc — so a component-only URL emitted a broken xref. Those components now resolve to their start page; labs and the docs-site umbrella components keep ::index.adoc, which is correct because they have a ROOT index page. On the labs naming question: verified redpanda-labs' docs/antora.yml says 'name: labs' on main (the branch the site playbook builds), and live, /labs/ serves 200 while /redpanda-labs/ 301s to it, so the map keeps labs as the component with redpanda-labs as the legacy slug.
|
Both findings addressed:
|
micheleRP
left a comment
micheleRP
left a comment
There was a problem hiding this comment.
Solid diagnosis and the reproduction table makes the bug easy to confirm. I independently verified every entry in COMPONENT_SLUG_MAP against each repo's antora.yml on main — all correct:
| Slug | Component | Verified |
|---|---|---|
redpanda-connect, connect |
connect |
rp-connect-docs antora.yml |
redpanda-cloud, cloud-data-platform |
cloud-data-platform |
cloud-docs antora.yml |
redpanda-labs, labs |
labs |
redpanda-labs docs/antora.yml @ main |
streaming |
streaming |
redpanda-data/docs antora.yml |
agentic-data-plane |
agentic-data-plane |
adp-docs antora.yml |
COMPONENT_START_PAGE checks out too — streaming, connect, cloud-data-platform, and agentic-data-plane all declare start_page: home:index.adoc, and labs plus the docs-site umbrellas have a real ROOT index.adoc. (My own local redpanda-labs checkout still says name: redpanda-labs; it's six months stale, so your labs reading is the right one.)
Requesting changes on one correctness issue that the tests currently lock in, plus one design question.
1. The three docs-site umbrella components are ROOT-only, so their mappings emit broken xrefs
docs-site/home, docs-site/self-managed, and docs-site/data-platform each contain only modules/ROOT/. The "first path segment after the slug is a module" rule doesn't hold for a single-module component:
| URL | This PR emits | Correct |
|---|---|---|
/home/how-to-use-these-docs/ |
xref:home:how-to-use-these-docs:index.adoc |
xref:home::how-to-use-these-docs.adoc |
/self-managed/get-started/intro/ |
xref:self-managed:get-started:intro.adoc |
xref:self-managed::get-started/intro.adoc |
/home/how-to-use-these-docs/ is a real published page (home/modules/ROOT/pages/how-to-use-these-docs.adoc), so this is reachable, not theoretical. The maps the docs-site umbrella components test asserts the broken forms, which means the bug ships with a green test blessing it.
Two ways out: add a ROOT-only set that emits xref:<component>::<path>.adoc for these three, or drop them from the map entirely. Dropping them isn't a regression — the pre-PR behavior was equally broken — but either way the test expectations need to change. labs, connect, cloud-data-platform, agentic-data-plane, and streaming are all genuinely multi-module (verified: labs has docker-compose, clients, kubernetes, ...; adp-docs has get-started, cli, connect, ...), so those entries are correct as written.
2. Self-qualifying streaming gives up version awareness
The converter's output lands in the streaming component, which is versioned (26.2 on main, plus v/* branches). A component-qualified resource ID with no version resolves to the component's latest version, so xref:streaming:manage:kubernetes/manage-resources.adoc emitted into a v/* branch during a CRD or Helm regen links across versions to latest.
Emitting a same-component xref for streaming URLs (xref:manage:kubernetes/manage-resources.adoc) keeps the current page's version and is strictly safer. You'd still want streaming in the map so the version segment gets stripped — just skip the component prefix when the mapped component is the one being generated into.
3. Fragment preservation is an undeclared behavior change (minor, scope)
The url.hash block changes output for every previously-anchored URL — they were silently dropped before. It's a good fix, but it isn't in the title or the "Fix" section, and it's the kind of change worth naming: anchors on the live site are rendered heading IDs, and they don't always survive as valid AsciiDoc anchors. 262abc6 in this repo was a fix for exactly that mismatch. An unresolvable anchor degrades to the top of the page rather than failing the build, so the risk is low — it just shouldn't arrive unannounced.
Nits
VERSION_SEGMENT_REacceptscurrent|beta|v?\d+\.\d+but misses three-part versions (v25.1.2) andlatest. Fine for the URL shapes in use; worth a comment saying so.- The map is a hardcoded snapshot of five repos'
antora.yml. A comment pointing at the source of truth would help whoever hits the next component rename. - The test suite is genuinely thorough (26 cases) and the legacy-prefix precedence test is a nice touch — the
hadLegacyPrefixguard is the subtle part and it's both correct and well documented.
Version bump needed
This changes cli-utils/convert-doc-links.js with no package.json bump, and npm is at 5.3.5. Per the repo's publish rule the fix won't reach consumers until a later PR bumps. #244 claims 5.3.6 and #245 claims 5.3.7, so this needs a version assigned before merge.
Downgrading this to non-blocking. The findings in the review above still stand — I'm just not gating the PR on them, so you can land on your own judgement.
|
Closing in favor of #247, which implements the redesign suggested here: the conversion now runs inside the Antora runtime as a Two things from this thread carried over directly: the fragment-preservation behavior and the docs#1830 reproduction case (now asserted both in unit tests and in an end-to-end Antora build). The dynamic-playbook question dissolved entirely — running in the real build means all content sources are already there. Thanks @micheleRP for the review findings that motivated the pivot; the ROOT-only umbrella components and start_page cases were exactly the class of guesswork the catalog-backed approach eliminates. |
… time Successor to #222. Instead of guessing URL structure with a hardcoded slug map, run the conversion inside the Antora runtime where the content catalog is the source of truth: - extensions/url-to-xref.js rewrites docs.redpanda.com URLs in page and partial content to xrefs at contentClassified, but only when the URL maps to a page published in the current build. Unmapped internal URLs stay raw and are logged as warnings, making the extension a broken-internal-link detector. Legacy URL shapes (/docs/, /current/, /vX.Y/, old component slugs) resolve through verified candidate rewrites; fragments and labels are preserved; latest-version targets emit unversioned xrefs, older targets emit version@ xrefs. - extensions/external-link-checker.js verifies every external URL responds (HEAD with GET fallback, one retry, concurrency-capped) and reports dead links in the build log; fail_on_broken escalates to error level for CI. - extensions/util/scan-content-urls.js is the shared scanner; it skips listing/literal/fenced/passthrough blocks, inline code spans, and attribute entries. - doc-tools generate crd-spec/helm-spec no longer convert URLs at generation time (cli-utils/convert-doc-links.js removed): raw docs URLs are valid links everywhere, and the build now upgrades them, which fixes the broken cross-component xrefs from docs#1830 without a maintained slug map. - Tests: 54 unit tests over the scanner and both extensions, plus an end-to-end suite that runs a real Antora build over a fixture site (local git sources, minimal UI bundle, local HTTP server for external links) and asserts on the published HTML and structured build log. Co-Authored-By: Claude Fable 5 <[email protected]>
2 participants
Bug
urlToXrefincli-utils/convert-doc-links.jsconverts docs.redpanda.com URLs found in operator godoc (used bydoc-tools generate crd-docsandgenerate helm-docs) into Antora xrefs by unconditionally treating the first URL path segment as a module in the current component. For URLs that point at other Antora components on docs.redpanda.com, this produces broken xrefs.Exact reproduction:
https://docs.redpanda.com/redpanda-connect/configuration/secrets/xref:redpanda-connect:configuration/secrets.adocxref:connect:configuration:secrets.adocThe broken xref shipped in the generated
modules/reference/pages/k-crd.adocvia redpanda-data/docs#1830 and was hand-fixed there (redpanda-data/docs#1830 (comment)). Every CRD regen reintroduces the breakage until the generator is fixed.Fix
Add a verified URL-slug → Antora-component map. When a URL has no legacy
/docs,/vX.Y, or/currentprefix and its first path segment is in the map, emit a fully qualifiedxref:component:module:page.adocresource ID with correct colon separation between module and page. A version segment directly after the slug (for example/streaming/current/...) is stripped. All other URLs keep the existing same-component behavior, and the map is deliberately not consulted after a legacy prefix is stripped (so/current/home/...still resolves to the current component'shomemodule).Shipped mapping (every entry verified against the target repo's
antora.ymlname:key; legacy slugs additionally verified via the live site's 301 redirects):redpanda-connect(legacy),connectconnectantora.yml; live 301/redpanda-connect/...→/connect/...redpanda-cloud(legacy),cloud-data-platformcloud-data-platformantora.yml; live 301/redpanda-cloud/...→/cloud-data-platform/...redpanda-labs(legacy),labslabsdocs/antora.yml; live 301/redpanda-labs/→/labs/streamingstreamingantora.yml; live 301/current/...→/streaming/current/...agentic-data-planeagentic-data-planeantora.ymlhome,data-platform,self-managedantora.ymlfilesTests
New
__tests__/cli-utils/convert-doc-links.test.js(18 cases), including:/current/,/docs/, and/vX.Y/URLs unchanged/connect/components/inputs/kafka/→xref:connect:components:inputs/kafka.adoc)streamingURLs, module-only and component-only URLsFull suite green: 34 suites, 768 tests passed. Also validated the converter against every docs.redpanda.com URL currently present in redpanda-data/redpanda-operator source: all legacy URLs unchanged, and the one cross-component URL now emits the same xref as the manual fix in docs#1830.
🤖 Generated with Claude Code