Fix: add redirects for markdown docs - #1817
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
patrickholcomb9
left a comment
patrickholcomb9
left a comment
There was a problem hiding this comment.
nice flag & fix, Dan - sorry for missing these md redirects! lgtm!
The route and middleware.ts each set a rel="canonical" Link header, so responses carried two. They disagreed whenever a redirect destination carried a fragment (/docs/frameworks/express resolves to .../serving-inngest-functions#framework-express, which the route dropped), and crawlers ignore conflicting canonicals outright. Middleware owns it now: it resolves the same redirects and always names the production origin, which is what a canonical needs to say from a preview deploy.
/docs-markdown returned 400 "Missing 'path' query parameter" — there is no such param; the empty slug was rejected before the lookup could reach pages/docs/index.mdx. /blog-markdown 404'd because only a [slug] route existed, so walking up from a post URL landed on nothing. Both are entry points agents reach for first. The blog index body is the listing /blog.txt already built, so that logic moves to a shared blog-index module rather than being duplicated. /blog.txt output is byte-identical.
|
Automated re-review could not be completed: the sandbox environment failed to check out the repository (repeated |
![gester-bot[bot]](https://avatars.githubusercontent.com/in/4369997?s=60&v=4){kind=small}
There was a problem hiding this comment.
Unable to complete this re-review: the sandbox tooling (checkout_pr and run_command) failed repeatedly with no usable output, so I could not fetch the diff or verify any changes since the last review. No findings are reported here to avoid guessing; please re-run the review once the environment is functional.
Re-review (2026-07-28): Unable to perform this re-review: the sandbox failed to check out the repository across many retries (git lock errors / unresponsive shell), so no diff could be inspected and no new findings could be verified. Posted a status comment on the PR; please re-run the review once the environment is available.
Re-review (2026-07-28): Unable to complete this review: repeated sandbox/tooling failures prevented checking out the PR or running any commands (every call returned exit=null), so the diff could not be inspected. No findings can be reported without verifying against the actual code — please re-trigger the review.
Re-review (2026-07-29): Reviewed the full diff (redirects.mjs extraction, docs-markdown/blog-markdown routing, middleware canonical logic, and the bulk link updates). The logic checks out against manual testing (chained redirects, versionless TypeScript path resolution, empty-slug docs index) — the one gap worth calling out is test coverage for the new redirect-resolution logic.
Re-review (2026-08-03): Re-reviewed the two new commits since last pass: the mechanical /docs link updates across content/pages files, and the new redirects.test.mjs. The test file directly addresses the previously-open finding (missing coverage for resolveRedirect/resolveDocsPath), with solid behavior tests plus useful table invariants (no dupes, no self-refs, chains terminate, destinations exist, no stale destinations). No new issues found.
Re-review (2026-08-03): No material changes since the last review — the new commit is just a merge from main bringing in unrelated changes (pricing/events pages, Cloudflare docs, etc.); none of the PR's core files (redirects.mjs, docs-markdown route, middleware, next.config) changed. Nothing new to flag.
|
Unable to complete this re-review: the sandbox environment is currently failing to clone/checkout the repository (git lock errors, "not a git repository", and unresponsive shell commands). No code could be inspected, so no review findings can be verified at this time. Please re-trigger the review once the environment is stable. |
|
Automated review could not be completed: the sandbox tooling failed to check out the repository ( |
347 links across the site pointed at URLs that only resolve via a 308 in
the redirect table. They worked, which is why they accumulated, but each
one costs a redirect hop and they were the source of the 404s in the
markdown mirror, where no redirect exists to paper over them.
Rewritten to the destination the redirect table already resolves to,
following chains where present (/docs/functions/retries took two hops).
Fragments and ?ref= tags are preserved verbatim.
207 pages/docs
100 content/blog, content/changelog, shared/Patterns/_patterns
38 link values in components/shared/app/pages
2 content/customers
Two files are deliberately untouched: shared/Docs/LanguageStore.ts uses
these paths as prefix-matching logic rather than links, and rewriting
them would break SDK language detection; and a comment in the
docs-markdown route that cites a redirect as an example.
| const at = pathname.search(/[?#]/); | ||
| return at === -1 | ||
| ? [pathname, ""] | ||
| : [pathname.slice(0, at), pathname.slice(at)]; |
There was a problem hiding this comment.
New, non-trivial logic (resolveRedirect's chain-following/cycle-guard and resolveDocsPath's versionless-TypeScript-path handling) ships with no unit tests, despite being the core of this PR's fix and being consumed by three different call sites (next.config.mjs, middleware.ts, docs-markdown route). A regression here (e.g. in the cycle guard or suffix handling) would silently break canonical tags or 404 the markdown mirror. Consider adding a small test file (the repo already has a precedent in mdx/utils/docs.test.ts using node:test) covering multi-hop chains, fragment preservation, and the versionless-typescript rewrite.
Covers the resolver logic and, more usefully, invariants over the redirect table itself: no duplicate or self-referencing sources, every chain terminates, every /docs destination has a page behind it, and no destination is itself a redirect source. That last set is what catches the rot that produced the markdown-mirror 404s, since a stale entry breaks the table long before anyone notices a bad link. Behaviour cases are pinned to real entries, including the ordering bug (reference/typescript/functions/errors must resolve via the redirect, not get pinned to a version directory with no file) and fragment inheritance across a chain (frameworks/express). Verified by mutation: breaking chain following, fragment inheritance, or the redirect step, and planting a rotted or duplicate entry, each fail the suite. pnpm test now runs this alongside the existing docs-markdown test.
2 participants
Our redirects do not also handle the markdown docs like
docs-markdownso some of those URLs dead end. This takes all the redirects and also copies them relative to thedocs-markdownURLs as well so LLMs/scrapers aren't getting 404s.Source: I pulled log data over a 24h period and saw lots of these.