Skip to content

Resolve Markdown document links from source paths - #137

Closed
guanzhousongmicrosoft wants to merge 3 commits into
documentdb:mainfrom
guanzhousongmicrosoft:fix-markdown-document-links
Closed

Resolve Markdown document links from source paths#137
guanzhousongmicrosoft wants to merge 3 commits into
documentdb:mainfrom
guanzhousongmicrosoft:fix-markdown-document-links

Conversation

@guanzhousongmicrosoft

Copy link
Copy Markdown
Contributor

Summary

  • resolve relative .md links against the Markdown source file instead of the rendered page URL
  • publish sibling, parent-directory, and index.md targets with trailing-slash website routes while preserving queries and fragments
  • provide source paths for article, reference, and reference-metadata rendering
  • cover the resolver with focused unit tests

This removes the need to rewrite every cross-document Markdown link as an absolute documentdb.io URL and fixes the long-term rendering gap tracked in the docs repository.

Fixesdocumentdb/docs#38

Verification

  • npm test (112 tests)
  • npm run lint
  • tsc --noEmit
  • npm run build:next

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.
@guanzhousongmicrosoft

Copy link
Copy Markdown
ContributorAuthor

Both review findings confirmed and fixed in 91594e0.

1. Cross-section api-reference links. Confirmed. The source paths callers pass are already in website space — /docs/reference/... — while an author writes the link against the folder they can actually see in the docs repository, ../api-reference/operators/aggregation/%24limit.md. That resolved to /docs/api-reference/..., which does not exist.

Worth noting why this is the only affected mapping: content.config.json renames exactly one folder on publish (api-referencereference); the others keep their names, and the articles/ prefix is internal and never reaches a URL. Path depth also matches between the two spaces, so ../-climbing links were already correct. The fix is therefore a single mapping entry rather than a mirror of the config, and the comment says so, so nobody has to re-derive it.

2. NEXT_BASE_PATH. Confirmed. These render as plain anchors, not next/link, so nothing applies the base path for us. Navbar.tsx already calls withBasePath from a client component, so this follows the established pattern rather than inventing one. Worth stating that production deliberately leaves NEXT_BASE_PATH unset — the deploy workflow comments on that at length — so this was latent rather than user-visible, which is what makes it easy to miss.

Coverage — eight new cases: the mapping from an article page and from inside the reference section, a section that is not remapped (guarding against over-eager rewriting), 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 NEXT_BASE_PATH once at module scope.

On verification, honestly:npm ci fails on this machine — the registry proxy 404s on vite@8.2.0 — so the vitest suite is left to CI, and the test wiring above is unproven locally. What I could verify, I did: the resolver was exercised directly under node with type stripping across all thirteen cases, with and without a base path. The two new behaviours:

../api-reference/operators/aggregation/%24limit.md -> /docs/reference/operators/aggregation/%24limit/
../../../api-reference/commands/query-and-write/find.md -> /docs/reference/commands/query-and-write/find/
[base=preview] functions.md -> /preview/docs/postgres-api/functions/
[base=preview] python-setup.md?view=full#connect -> /preview/docs/getting-started/python-setup/?view=full#connect
[base=preview] #examples -> #examples

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.
@guanzhousongmicrosoft

Copy link
Copy Markdown
ContributorAuthor

You are right, and my previous fix was wrong in a way its own tests could not detect. Corrected in 4ab8888.

The mechanism.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/...; the same code after hydration emitted /docs/.... The href changed under the reader, with a hydration mismatch alongside it.

Two things made this easy to miss, and both are worth recording:

  • The exported HTML is correct in the broken version. The export comes from the server-side render that reads the variable fine, so nothing on disk looks wrong. A build-level assertion would have passed — and would also pass on a future regression, which is why I did not add one.
  • vitest runs in node, so my base-path tests exercised the one environment where the bug does not exist. They asserted the prefix was applied and were green.

I also cited Navbar as precedent for calling withBasePath from a client component. That was the wrong reasoning: it is the same latent defect, not a sanction. More on that below.

The fix. The resolver is pure again and returns a base-path-relative route. The anchor renderer uses next/link when the href was rewritten — which by definition makes it an internal document route — and next/link applies the basePath from next.config identically on the server and in the browser. Hrefs the resolver leaves alone still render as plain anchors, so external links, fragments and hand-written absolute paths behave exactly as before. No new environment variable, and nothing for a deployment to keep in sync.

Coverage, following the same reasoning rather than the original request:

  • The base-path tests now assert the route is unchanged whether or not NEXT_BASE_PATH is set, including a case that compares both directly.
  • A source guard asserts neither client-reachable module mentions NEXT_BASE_PATH, sitePath or withBasePath, with comments stripped first.

On "browser/build-level coverage" specifically: build-level cannot catch this, for the reason above. Browser-level would need hydration testing, and the project has no harness for it — jsdom and testing-library are not dependencies, and adding them is more than this branch should carry, especially as npm ci fails against the registry proxy on my machine so I could not validate the setup. The guard therefore sits at the source, where the mistake is actually made. If you would rather have the real thing, adding a hydration harness is worth its own PR.

Related, not fixed here.Navbar.tsx calls withBasePath at module scope in a client component — for the blogs anchor and the logo image — and has the same defect. It is a separate surface, so I left it alone rather than growing this branch. Happy to take it next.

Verified locally as far as the environment allows: the resolver returns identical routes with and without NEXT_BASE_PATH set, and both modules pass the source guard. The suite itself is left to CI.

@guanzhousongmicrosoft

Copy link
Copy Markdown
ContributorAuthor

Reopened as #141 from a personal fork, so the branch and the pull request are owned by the same account that authors the commits. The tree is identical — the same three commits, unchanged.

The review discussion above stays here rather than moving: the api-reference mapping and NEXT_BASE_PATH findings and the correction on why the base path could not be read in the browser are both worth reading alongside #141, and #141's description summarizes both.

Closing in favour of #141.

guanzhousongmicrosoft pushed a commit that referenced this pull request Aug 4, 2026
withBasePath reads NEXT_BASE_PATH, which is a private build variable, so
Next strips it from the browser bundle. Navbar is a client component and
calls it at module scope for the blogs anchor and the logo image: under a
configured base path the static render produced /preview/blogs/, and the
same code after hydration produced /blogs/, changing the href under the
reader and leaving React a mismatch to reconcile.
Nothing on disk shows this. The export is written by the server-side
render, which reads the variable correctly, so the emitted markup is
right in both the broken and the fixed build - only a real browser
disagrees. That is what made it survive review twice.
next.config now republishes the normalized value as
NEXT_PUBLIC_BASE_PATH, which Next inlines into both bundles, and
sitePath prefers it while keeping the private variable as a fallback for
server-only callers and standalone scripts that never see the
republished one. Deployments still set the single variable they already
set, and every existing caller is fixed without being touched, including
any added later.
Covered with the environment stubbed both ways: the public variable, the
private fallback, agreement between them, slash normalization, an empty
republished value meaning no base path, and relative and absolute URLs
left alone.
Exercised directly under node across all four combinations of the two
variables. The vitest suite is left to CI, since npm install fails
against the registry proxy on this machine.
Found while fixing the same defect in the Markdown link resolver
(#137), which solves it differently - that path is an internal route, so
it can hand the prefixing to next/link and read no environment at all.
Sign up for freeto 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.

Fix broken links in the website (https://documentdb.io) for the quick start links that link to .md files

2 participants

@guanzhousongmicrosoft@GuanzhouSong