Derive adapter params from routes, not local slugs - #184
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 740699f867
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const slugs = await listJoinedSlugs({ source: config.source }); | ||
| const basePath = config.basePath; | ||
| return slugs.map((slug) => joinUrlPath(basePath, slug)); |
There was a problem hiding this comment.
When an explicit basePath is combined with a mount that changes a page's local slug (for example, policies/privacy mounted at /docs/legal/privacy), this emits /guide/legal/privacy, but createLoadPageData({ source, basePath: "/guide" }) searches for that route against the canonical /docs/legal/privacy and then falls back to the nonexistent local slug legal/privacy. The generated prerender route therefore loads null; re-rooting must retain enough source-prefix information for the load helper to map the emitted slug back to the mounted page.
Useful? React with 👍 / 👎.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Deriving params from urlPath is the right call, but basePath now means two opposite things depending on the adapter, and the Nuxt basePath + mounts combination prerenders routes that 404. I confirmed the latter by running it.
Reviewed changes
- Read the full diff across all 13 files.
- Traced
resolveRouteBase/listRouteSlugs/createLoadPagethrough all five adapters. - Ran a throwaway harness against the built helpers to confirm the Nuxt round-trip failure and the cross-adapter
basePathdivergence. - Checked the i18n path helpers,
baseUrlPrefixForMountstie-breaking,listPages()caching, and the in-repo example apps — all clean, no findings there.
🚨 basePath re-roots on Nuxt but relativizes-and-throws everywhere else
createPrerenderRoutes treats basePath as a prefix to prepend (joinUrlPath(basePath, slug)). The other four adapters route through listRouteSlugs, where basePath is the base pages are made relative to — and any page outside it is now a hard throw.
Same option name, opposite direction. I ran identical arguments through all five against a source whose routePrefix is /docs, with basePath: "/guide":
next: THREW: page "index.mdx" resolves to "/docs", outside the route base "/guide"
astro: THREW: (same)
sveltekit: THREW: (same)
nuxt: ["/guide", "/guide/quickstart"]
The compatibility angle matters more than the inconsistency. Before this PR listJoinedSlugs ignored basePath outright, so on those four adapters it was a silent no-op — passing it did nothing and builds succeeded. Now the exact configuration described in docs/integrations/framework-matrix.mdx:31 ("If your docs are mounted somewhere other than /docs, pass the matching path to the adapter's basePath … option") throws at build time.
That's a breaking change for anyone who followed that sentence, but .changeset/route-aware-params.md marks it minor and calls it "additive". Worth either bumping the changeset and writing a migration note, or making the four adapters re-root like Nuxt does so basePath keeps one meaning.
Two docs pages also still describe the pre-PR behaviour and aren't touched here:
docs/integrations/framework-matrix.mdx:31— the sentence quoted above.docs/pipeline/configure-sources.mdx:78-117— documents{ pathPrefix: "changelog", urlPrefix: "/changelog" }as the canonical mounts shape. That mount is not a catch-all, soroutePrefixstays/docswhile the pages land on/changelog/**, and every stock static-params helper now throws on them. This repo's owndocs/docs.config.ts:289uses exactly that config.
⚠️ No test pins the new basePath behaviour on non-Nuxt adapters
framework-adapters.test.ts covers mount-derived routes well, but I couldn't find a case that passes basePath to Next/Astro/SvelteKit/TanStack and asserts the resulting params — which is the surface that changed from no-op to throwing. The Nuxt basePath + mounts combination in the inline comment below is likewise uncovered; a test there would have caught it.
ℹ️ Nitpicks
docs/reference/project.mdx — the new site-root snippet sets basePath: "/" on the params helper but leaves createGenerateMetadata({ manifest }) without it. Since createGenerateMetadata hardcodes its own config.basePath ?? "/docs" (packages/leadtype/src/next/index.ts:379) and has no routePrefix awareness, copying that snippet gives you canonical URLs under /docs on a site-root deployment. Adding basePath: "/" to both calls would make the example self-consistent.
Claude Opus ｜ 𝕏
| // An explicit basePath re-roots the routes: pages are enumerated relative | ||
| // to the source's own prefix, then joined onto the override. A page a | ||
| // mount moved outside the source's prefix cannot be re-rooted and throws. | ||
| const slugs = await listJoinedSlugs({ source: config.source }); |
There was a problem hiding this comment.
listJoinedSlugs({ source: config.source }) drops basePath, so slugs come back relative to the source's routePrefix and then get re-rooted under basePath. When the source has mounts, those two bases disagree and the prerendered route no longer matches what createLoadPageData can resolve.
I confirmed this against the built helpers — a source with routePrefix: "/docs" and a page whose file slug is policies/privacy but whose urlPath is /docs/legal/privacy, with basePath: "/guide", prerenders /guide/legal/privacy while the load helper returns null for it. This round-tripped correctly before the PR, since listJoinedSlugs emitted local slugs and createLoadPage resolved by local slug.
Technical details
Harness output:
prerender routes: ["/guide","/guide/quickstart","/guide/legal/privacy"]
load /guide/legal/privacy -> NULL (404)
The trace:
listJoinedSlugs({ source })uses base/docs(nobasePathpassed), so/docs/legal/privacyrelativizes tolegal/privacy.joinUrlPath("/guide", "legal/privacy")emits/guide/legal/privacy.createLoadPagedoes getbasePath, so its base is/guideand it buildsroutePath = /guide/legal/privacy.- No page matches — the real
urlPathis/docs/legal/privacy. - Fallback
source.loadPage(["legal", "privacy"])misses too, because the file slug is["policies", "privacy"]. →null.
Forwarding basePath makes both sides agree, and turns the mismatch into the same loud error the other adapters raise rather than a silent 404:
const slugs = await listJoinedSlugs({
source: config.source,
basePath: config.basePath,
});That does mean pages outside basePath throw here too — which seems like the intent, but it interacts with the basePath semantics question in the review body, so worth deciding both together.
| // for a whole multi-collection project in one call. | ||
| if (config.basePath === undefined) { | ||
| const pages = await config.source.listPages(); | ||
| return pages.map((page) => normalizeUrlPath(page.urlPath)); |
There was a problem hiding this comment.
This branch maps page.urlPath straight through with no base validation, while every other adapter now throws when a page escapes the route base. If Nuxt's catch-all is mounted at /docs and a mount sends pages to /changelog/**, those routes get prerendered and then 404 at request time — the silent misroute the throw elsewhere in this PR exists to prevent.
Not necessarily wrong (Nuxt prerender routes are absolute, so emitting urlPath verbatim is defensible), but the asymmetry is worth a comment explaining why Nuxt is exempt.
| const slug = routeSlugFromUrlPath(page.urlPath, base); | ||
| if (slug === null) { | ||
| throw new Error( | ||
| `leadtype: page "${page.relativePath}${page.extension}" resolves to "${page.urlPath}", outside the route base "${base}" — a catch-all mounted at "${base}" cannot serve it. Mount a catch-all at the prefix that owns the page and hand it that collection's source (\`project.getSource(key)\`), or pass the base your catch-all is actually mounted at via \`basePath\` ("/" for a site-root catch-all).` |
There was a problem hiding this comment.
Both suggested fixes assume shapes that a single-collection config with top-level mounts doesn't have: there's no getSource(key) for a mount, and basePath: "/" requires actually moving the catch-all to the site root.
That's the config in docs/pipeline/configure-sources.mdx:78-117 — and in this repo's own docs/docs.config.ts:289 — so the person most likely to hit this error gets pointed at two remedies neither of which applies. Mentioning the third option (make the mount a catch-all, i.e. empty pathPrefix, so routePrefix picks it up) would close the gap.
| const match = pages.find( | ||
| (page: DocsPageMeta) => normalizeUrlPath(page.urlPath) === routePath | ||
| ); | ||
| if (match) { | ||
| // A project meta carries its collection; load by route path, which the | ||
| // project resolves uniquely — a collection-local slug can be ambiguous | ||
| // across collections. A plain source loads by its exact slug. | ||
| return await config.source.loadPage( | ||
| "collection" in match ? routePath : match.slug | ||
| ); |
There was a problem hiding this comment.
When the source is a DocsProject, this urlPath pre-check runs before project.loadPage's own resolution. A page whose route path matches here is returned directly, bypassing the project's local-slug lookup and the "ambiguous slug" throw it raises for cross-collection collisions.
Only reachable with a nested collection routePrefix (e.g. /docs/policies) where a route path is also a valid local slug elsewhere, so fairly contrived — but if the ambiguity error is meant to be authoritative, delegating to source.loadPage for the project case rather than pre-matching would keep it so.
1 participant
Stacked on #183 → #182 → #167.
Problem
All five framework static-params helpers — Next
createGenerateStaticParams, AstrocreateGetStaticPaths, NuxtcreatePrerenderRoutes, SvelteKitcreateEntries, TanStack StartcreateStaticParams— mapped each page's collection-localslug(internal/framework.tslistJoinedSlugs,next/index.ts).slugis derived from the file path with no mount applied, whileurlPathis mount-aware (source/index.ts), so:/changelog/1-0yielded['1-0'], collection indexes yielded duplicate params, and a single catch-all fed by local slugs omitted or misrouted collections (established across Tell one canonical wiring story across the docs #183's review threads).mountsentry (this repo's owndocs.config.tschangelog mount is that shape) rendered pages at URLs the generated sitemap never advertises — open Tell one canonical wiring story across the docs #183 thread onuse-the-source-primitive.mdx.createPrerenderRoutesjoined slugs ontoconfig.basePath ?? "/docs", so even the per-collection workaround prerendered/docs/1-0instead of/changelog/1-0— open Tell one canonical wiring story across the docs #183 thread onproject.mdx.Design
Params are now each page's mount-aware
urlPathrelative to a route base, and the load helpers resolve params back through the same route space:DocsSourcegains an optionalroutePrefixproperty — the URL prefix its unprefixed files resolve under (the catch-all mount'surlPrefix,"/docs"otherwise;baseUrlPrefixForMountsmirrorsresolveDocsPathMount's tie-breaking exactly).createDocsSourcealways sets it;createDocsProjectexposes the primary collection's;project.getSource(key)sources carry their collection's. Optional so hand-rolled structural sources keep compiling.source.routePrefix, overridable with a newbasePathoption on every params/load helper.basePath: "/"serves a whole multi-collection project from one site-root catch-all with route-prefixed params (['changelog', '1-0']), whichloadPageresolves route-path-first — never through the ambiguous local-slug path.project.getSource(key)per prefix, orbasePath) — loud error over silent misroute.basePathnow emits each page's realurlPathverbatim (correct for collection sources, mounts, and whole projects); an explicitbasePathkeeps its re-rooting meaning for in-prefix pages and throws for pages a re-root cannot represent.Backward compat: for an unmounted single collection,
urlPathrelative to/docsis byte-identical to today's slug, so existingapp/docs/[[...slug]]catch-alls (init-scaffolded, shown in every example) receive identical params — pinned by tests across all five adapters, including the pre-existing fixture assertions which pass unchanged.Per-adapter behavior
createGenerateStaticParams['quickstart'])['policies','privacy'], misrouted) → now URL (['legal','privacy'])getSource(key), full-route params viabasePath: "/", loud error otherwisecreateGetStaticPaths/createMarkdownStaticPathscreateEntriescreateStaticParamscreatePrerenderRoutes/docs/quickstart)/docs/policies/privacy→ now/docs/legal/privacybasePath ?? "/docs"→ now each page's real route, no option neededAll
createLoadPageDatahelpers resolve the new params back to the page they address (round-trip tested), with the raw-slug fallback intact.Docs
docs/reference/project.mdx— the multi-collection caveat Tell one canonical wiring story across the docs #183 added shrinks to the supported story: both layouts use stock helpers; the site-root layout isbasePath: "/"instead of hand-writtenlistPages()mapping; the Nuxt exception is gone.docs/pipeline/use-the-source-primitive.mdx— the single-collection claim now holds undermounts, stated as such.docs/reference/source.mdx— documentsroutePrefix.docs/paths.lock.json— regenerated hashes for the three edited pages only.Resolves both open #183 review threads (mounts breaking the single-collection claim; Nuxt
basePathmismatch).Verification
bun run test(packages/leadtype): 851 pass, incl. new adapter round-trip, collection-source, site-root, and loud-error tests plus real-project integration tests; pre-commit hook suite: 897 passtsgo --noEmit: cleanbun run lint(ultracite): cleanbun x leadtype lint docs --error-unknown --max-warnings 0: all 54 files passbun x leadtype doctor --src . --docs-dir docs: exit 0Changeset:
minor— additive (routePrefix,basePath) with identical output for the scaffolded path.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.