Uh oh!
There was an error while loading. Please reload this page.
Resolve Markdown document links from source paths - #141
Conversation
Signed-off-by: Guanzhou Song <guanzhousong@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two gaps in the resolver, both found in review. A cross-section link naming api-reference resolved to a route that does not exist. content.config.json publishes the docs repository's api-reference/ folder at /docs/reference, and the source paths callers pass are already in website space, so a link written the only way an author can write it - ../api-reference/operators/aggregation/$limit.md, against the folder they can see - resolved to /docs/api-reference/... and 404'd. That is the one folder whose published name differs; every other section keeps its own, so the mapping is a single entry rather than a copy of the config. The resolved route also skipped NEXT_BASE_PATH. Markdown links render as plain anchors rather than next/link, so nothing applies the base path for us, and a subpath deployment would have emitted /docs/... - correct only at the domain root. withBasePath now wraps the path, leaving the query and fragment after it. Navbar already does this from a client component, so this follows the pattern the codebase settled on rather than introducing one. Covered both: the mapping from an article page and from inside the reference section, a section that is not remapped, and the base path across a plain route, a mapped route, one carrying a query and fragment, and links the resolver declines to touch. The base path tests re-import the module after stubbing the environment, since sitePath reads it once at module scope. npm install fails against the registry proxy on this machine, so the vitest suite is left to CI. The resolver itself was exercised directly under node with type stripping, over every case above, with and without a base path configured.
The previous fix was wrong in a way its tests could not see. Markdown is a client component, so markdownLinks ships in the browser bundle, and NEXT_BASE_PATH is not NEXT_PUBLIC_-prefixed: Next strips it from that bundle. Under a configured base path the static render - which runs in node, where the variable exists - emitted /preview/docs/..., and the same code after hydration emitted /docs/..., so the href changed under the reader and React had a mismatch to reconcile. Nothing in the exported HTML shows this, because the export is produced by the server-side render that reads the variable correctly. The tests passed for the same reason: vitest runs in node. The resolver is pure again and returns a base-path-relative route. The anchor renderer now uses next/link for a rewritten href, which is by definition an internal document route, and next/link applies the basePath from next.config on both the server and the client. Hrefs the resolver leaves alone still render as plain anchors, so external links, fragments and hand-written absolute paths behave exactly as before. Coverage follows the same reasoning. The base-path tests now assert the route is unchanged whether or not NEXT_BASE_PATH is set, and a source guard asserts neither client-reachable module mentions NEXT_BASE_PATH, sitePath or withBasePath. A build-level check would not have caught this and would not catch a regression: the markup on disk is correct in both the broken and the fixed version. Proving the browser behaviour would need hydration testing, which the project has no harness for today - jsdom and testing-library are not dependencies - so the guard is placed where the mistake is made instead. Note Navbar has the same latent defect: it calls withBasePath at module scope in a client component, for the blogs anchor and the logo image. Left alone here, since it is a different surface and this branch should not grow to cover it.
Uh oh!
There was an error while loading. Please reload this page.
GuanzhouSong
commented
Aug 4, 2026
All three review findings confirmed and fixed in Prototype keys in the section table. The lookup was on a plain object, so inherited keys resolved as configured mappings: Now a The guard had a hole of its own — and this is the one worth dwelling on, because the guard exists specifically to stop a Reference source paths came from the route. On the three you set aside — I agree with all three, and with the reasoning. Worth adding one thing to the Verified under node, as before — |
Supersedes #137, reopened from a personal fork; the commits and review history are unchanged. Fixesdocumentdb/docs#38.
Summary
.mdlinks against the Markdown source file instead of the rendered page URLindex.mdtargets as trailing-slash website routes, preserving queries and fragmentsThis removes the need to write every cross-document Markdown link as an absolute
documentdb.ioURL — the workaround that documentdb/docs#57 and documentdb/docs#65 applied by hand across 36 links.Two fixes from review
Cross-section
api-referencelinks. The source paths callers pass are already in website space (/docs/reference/...), while an author writes the link against the folder they can see in the docs repository (../api-reference/operators/aggregation/%24limit.md). That resolved to/docs/api-reference/..., which does not exist.content.config.jsonrenames exactly one folder on publish —api-reference→reference; the rest keep their names, and thearticles/prefix never reaches a URL. Path depth also matches between the two spaces, so../-climbing links were already correct. The fix is therefore one mapping entry rather than a mirror of the config, and the comment says so.NEXT_BASE_PATH. The first attempt at this was wrong in a way its own tests could not detect, and the correction is the interesting part.Markdownis a client component, so the resolver ships in the browser bundle, andNEXT_BASE_PATHis notNEXT_PUBLIC_-prefixed — Next strips it there. CallingwithBasePathin the resolver produced/preview/docs/...during the static render, which runs in node where the variable exists, and/docs/...after hydration. The href changed under the reader.Two things hid it. The exported HTML is correct in the broken version, because the export comes from the server render — so a build-level assertion passes either way, on the bug and on a future regression. And vitest runs in node, so tests exercised the one environment where the bug does not exist and went green while asserting the wrong thing.
The resolver is now pure and returns a base-path-relative route. A rewritten href is by definition an internal document route, so it renders with
next/link, which applies thebasePathfromnext.configidentically on the server and in the browser. Everything else still renders as a plain anchor, so external links, fragments, and hand-written absolute paths are untouched.Coverage
17 tests. Alongside the original resolution cases: the
api-referencemapping from an article page and from inside the reference section, a section that is not remapped, the route staying identical whether or notNEXT_BASE_PATHis set, and a source guard asserting neither client-reachable module mentionsNEXT_BASE_PATH,sitePath, orwithBasePath.The guard sits at the source deliberately. Build-level cannot catch this class, per the above; browser-level would need hydration testing, and the project has no harness —
jsdomandtesting-libraryare not dependencies.Verification
CI ran the suite green on the identical tree under #137 (
tests/markdownLinks.test.ts (17 tests) ✓, both jobs passing). Locally,npm cifails against the registry proxy, so the resolver was exercised directly under node with type stripping across every case, with and without a base path configured.Related
#138 fixes the same
NEXT_BASE_PATHdefect inNavbar, which cannot usenext/link— it points at/blogs/, a route Next does not own, and an imagesrc. Independent of this PR and mergeable in either order.