Reduce navigation config churn for large and repeated docs trees - #164
Reduce navigation config churn for large and repeated docs trees#164KayleeWilliams wants to merge 3 commits into
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 |
KayleeWilliams
changed the title
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two of these have concrete fallout I reproduced by running the real CLI: a pin that uses the leading-slash form documented in this PR hard-fails leadtype generate in any collections project, and the fromDirectory JSDoc example throws when copied. A third makes the new nav command print a tree that disagrees with its own page count.
Reviewed changes
navigation.fromDirectory()— new public config helper on theleadtype/navigationsubpath, returning[{ include: "<dir>/**", ...options }].pin?: string[]onDocsNavIncludeEntry, applied byapplyNavPinsfromresolveNavEntryPagesahead of the existing "include matched nothing" guard; unmatched pin is a hard error.leadtype nav— new read-only command (338 lines) printing the resolved tree and reportingunplaced/duplicate/unknownGroupdrift, exit 1 on drift.- Docs (
frontmatter.mdx,cli.mdx"Seven commands" → "Eight"), changeset,paths.lock.jsonrehash, regeneratednext-exampleMDX map.
The design is the right one — pin keeps the decisions and lets the inventory sort itself, and making an unmatched pin an error rather than a silent no-op is the correct call, since it's almost always a rename the config missed. Expansion staying in resolution rather than the helper is what preserves the single-resolved-tree invariant. All 21 new tests pass and the working tree is clean. What follows is about the edges where the new authoring surface meets assembly code the PR didn't touch.
pin isn't carried through collection mount prefixing
This is the one I'd fix before merge. prefixCollectionNavPageEntry (packages/leadtype/src/cli/generate.ts:1651-1673) rewrites include and exclude with the collection mountPath, but pin reaches the output only through the object spread — unprefixed. The new pin JSDoc explicitly promises "a leading slash escaping to the collection root", so the documented form is the one that breaks.
Reproduced end-to-end through runCli(["generate", ...]) on a two-collection fixture:
generate exit code: 1
leadtype generate: Nav pin "/1-0" under "releases" did not match any page included by "/changelog/**"
The message exposes the asymmetry directly: the include was prefixed to /changelog/**, the pin was left as /1-0. A control run with pin: ["1-0"] exits 0, isolating it to the leading-slash form.
Worth noting why this got through: every new pin test uses a single-source config with top-level navigation. Nothing exercises pin inside a collections map, which is the only shape that routes through the prefixer. A test at that shape would be the useful regression guard here.
leadtype nav omits root-level pages from the tree it prints
toTree (packages/leadtype/src/cli/nav.ts:123-135) walks only manifest.groups, but countPages (137-144) adds manifest.ungrouped.length, and renderHuman prints both on adjacent lines. For any config with root-level page entries the header count exceeds the rows shown.
That shape isn't hypothetical — it's what DocsNavEntry's own doc comment describes ("Strings and include entries become ordered root pages"), and it's what this PR's curated() fixture in nav.test.ts uses. The test asserting report.tree locks in the truncated output, so the gap reads as intended rather than accidental.
findDuplicates skips ungrouped too (line 171), so a page placed both at the root and inside a group isn't reported as duplicated even though it renders twice.
Excluding root pages from unplaced is correct — placedAtRoot is right about that. It's only the printed tree and the duplicate scan that should also see them.
Not anchorable to the diff
.agents/skills/leadtype/SKILL.md:41 still directs agents to leadtype doctor for the navigation tree. Now that nav exists and is the more precise answer, that pointer is stale.
Claude Opus ｜ 𝕏
| * ```ts | ||
| * { | ||
| * title: "Concepts", | ||
| * pages: navigation.fromDirectory("concepts", { | ||
| * pin: ["initialization-flow", "consent-models"], | ||
| * exclude: "concepts/internal-*", | ||
| * }), | ||
| * } |
There was a problem hiding this comment.
This example throws if copied. pin resolves relative to the node's nearest base, but the example has no base — so the pins resolve root-relative while the include is concepts/**, and nothing matches.
Reproduced verbatim from this snippet:
Nav pin "initialization-flow" under "concepts" did not match any page included by "concepts/**"
The prose docs get this right — frontmatter.mdx and the changeset both use base: "concepts" with fromDirectory("."), which resolves fine. It's only the JSDoc that's wrong, which is unfortunate because it's the copy-pasteable surface: it's what shows on IDE hover for a brand-new API.
The exclude in the example has the same mismatch — it's concepts/internal-* where the docs use internal-*, since exclude is also base-relative.
Technical details
applyNavPins resolves each pin via joinNavPath(group.base, pin). With no base on the node, group.base is empty, so "initialization-flow" stays as-is and is looked up against a matches map keyed on concepts/initialization-flow. Aligning the example with the docs (base on the node, fromDirectory(".")) is the smaller change and matches how the feature is meant to read.
| * ```ts | |
| * { | |
| * title: "Concepts", | |
| * pages: navigation.fromDirectory("concepts", { | |
| * pin: ["initialization-flow", "consent-models"], | |
| * exclude: "concepts/internal-*", | |
| * }), | |
| * } | |
| * { | |
| * title: "Concepts", | |
| * base: "concepts", | |
| * pages: navigation.fromDirectory(".", { | |
| * pin: ["initialization-flow", "consent-models"], | |
| * exclude: "internal-*", | |
| * }), | |
| * } |
| /** | ||
| * Paths to place first, in this order, ahead of the sorted remainder. | ||
| * | ||
| * A large section usually has two or three pages that must lead and a long | ||
| * tail whose order barely matters. Without pinning, keeping those first means | ||
| * listing every page in the section by hand — and re-listing them whenever | ||
| * one is added. Pins are resolved the same way page refs are: relative to the | ||
| * nearest `base`, with a leading slash escaping to the collection root. A pin | ||
| * that matches nothing the include matched is an error, because it is | ||
| * silently doing nothing. | ||
| */ | ||
| pin?: string[]; |
There was a problem hiding this comment.
Two things about this field as declared:
It isn't validated. validateDocsNavPageEntry (packages/leadtype/src/config/inherit.ts:127-155) type-checks exclude, sort, and required, but has no pin branch, so a wrong-typed pin from a JS config or an untyped source flows straight into applyNavPins.
It's asymmetric with exclude, which accepts string | string[]. Someone pinning one page will reasonably write pin: "setup" — and because the type is string[]-only, the string gets iterated character by character:
Nav pin "s" under "guides" did not match any page included by "**"
That error is genuinely hard to act on; nothing in it suggests the shape was the problem. Either accepting string | string[] to match exclude, or adding a pin branch to the validator that rejects a bare string with a message naming the fix, would close it.
Separately, this JSDoc is what makes the mount-prefixing bug in the review body a documentation contradiction rather than just a gap — the leading-slash promise here is exactly the form that fails under collections.
| /** | ||
| * Reorder an include expansion so pinned pages lead, in the order they were | ||
| * pinned. Everything else keeps its sorted position behind them, so adding a | ||
| * page to the directory never displaces a deliberate choice. | ||
| * | ||
| * Returns `undefined` when the entry declares no pins, so the caller keeps the | ||
| * unmodified sorted array rather than paying for a rebuild. | ||
| */ | ||
| function applyNavPins( | ||
| group: ResolvedGroup, | ||
| entry: DocsNavIncludeEntry, | ||
| matches: SourceDoc[] | ||
| ): SourceDoc[] | undefined { | ||
| if (!entry.pin || entry.pin.length === 0) { | ||
| return; | ||
| } | ||
| const byRelativePath = new Map( | ||
| matches.map((doc) => [normalizeNavPath(doc.relativePath), doc]) | ||
| ); | ||
| const leading: SourceDoc[] = []; | ||
| const pinnedPaths = new Set<string>(); | ||
| for (const pin of entry.pin) { | ||
| const ref = joinNavPath(group.base, pin); | ||
| const doc = byRelativePath.get(ref); | ||
| if (!doc) { | ||
| const scope = group.segmentPath.join("/") || "root"; | ||
| // A pin that matches nothing is doing nothing — usually a rename the | ||
| // config missed. Failing names it; warning would let it rot. | ||
| throw new Error( | ||
| `Nav pin "${pin}" under "${scope}" did not match any page included by "${entry.include}". Fix the path or remove the pin.` | ||
| ); | ||
| } | ||
| if (pinnedPaths.has(ref)) { | ||
| continue; | ||
| } | ||
| pinnedPaths.add(ref); | ||
| leading.push(doc); | ||
| } | ||
| return [ | ||
| ...leading, | ||
| ...matches.filter( | ||
| (doc) => !pinnedPaths.has(normalizeNavPath(doc.relativePath)) | ||
| ), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
applyNavPins reorders within its own entry's matches, which is correct in isolation but can be silently undone by the group assembly downstream.
buildNavigationGroupFromNav (line 3590-3602, unchanged by this PR) is strictly first-entry-wins by urlPath across group.pageEntries. So given:
pages: [
{ include: "featured/*" },
{ include: "**", pin: ["featured/zzz"] },
]entry 1 places featured/aaa and featured/zzz in sort order. Entry 2's applyNavPins succeeds — featured/zzz is in its own matches, so the unmatched-pin guard correctly doesn't fire — and hoists it locally. Then the merge loop skips every one of entry 2's pages as already-seen. The pin never leads, and nothing warns.
The reason the unmatched-pin error can't catch this is structural: it validates against the entry's own match set, which has no visibility into what earlier entries already consumed. The same first-wins pattern governs rootSeenUrlPaths (3645-3661), so root-level entries have it too.
Technical details
The new test "lets explicit entries and an expansion coexist in one section" (navigation.test.ts) does exercise multi-entry composition, but with disjoint sets — fromDirectory("deep") doesn't overlap the explicit "setup" ref — so it doesn't reach this path.
This is arguably pre-existing dedup behavior rather than something the PR introduced. But pin is the first feature whose entire contract is about position, so it's the first one for which first-wins-across-entries is observably wrong rather than merely arbitrary. Options range from hoisting pins at group-assembly time to warning when a pinned page was already claimed by an earlier entry; the latter is the smaller change and keeps the failure loud, consistent with how unmatched pins are treated.
f9a4035 to
f7e9e4b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
f7e9e4b makes leadtype nav resolve pinned remote sources — the right fix, and the two new tests genuinely fail without it. But threading inheritance through runNavCommand introduced a fourth origin value that three downstream consumers weren't written for. All three issues below were reproduced locally against temp fixtures.
Reviewed changes
packages/leadtype/src/cli/nav.ts— inheritance is now applied before the tree is resolved; content dir comes fromresolveCollection(...).absoluteDir; neworigin: "inherited"; new missing-content-dir early exit. This is the whole PR-authored delta since the last review.packages/leadtype/src/cli/nav.test.ts— newdescribe("remote collections")(2 tests). Real coverage: both fail without the fix.docs/reference/cli.mdx— neworiginparagraph; names all four values correctly.apps/next-example/**MDX map regen +paths.lock.jsonrehash (reference/cli,writing/frontmatteronly) — mechanical, matches the docs edits.
Note on scope: this branch was rebased, so the previously reviewed SHAs no longer exist. .changeset/git-source-groups.md, apps/c15t-example/leadtype.config.ts, and the sparse field on DocsCollection/GitSourceConfig come from base dx/155-doctor, not this PR — I excluded them.
🧵 Prior threads
The resolveCollection thread is resolved by this commit — retired. The other three remain open and re-verified unchanged in the current source; I'm not restating them inline. The one still worth blocking on is /-rooted pin not being prefixed by prefixCollectionNavPageEntry (src/cli/generate.ts:1676-1698), which hard-fails generate in collections projects and has no test.
✅ Verified fine — not issues
gitSource/sourcesconfigs do work withnav. I suspected a gap sincenavreads rawloaded.config.collections, butloadDocsConfigalready returns a config flattened byexpandGitSourcesand alias-folded byapplyCollectionAliases. Rannavinapps/c15t-example: resolves tocontent-fixtures/c15t/docs, correct.- Reading raw
loaded?.config.collectionsmatchesdoctor.ts:569— established sibling convention. - The new
existsSyncearly exit is an improvement, not a regression: previously this path produced a silently wrong filesystem-inferred tree.
Claude Opus ｜ 𝕏
| const rootIsLiteral = | ||
| origin !== "explicit" || | ||
| rootEntries.every( | ||
| (entry) => typeof entry === "string" || !("include" in entry) | ||
| ); |
There was a problem hiding this comment.
origin: "inherited" silently disables the unplaced drift check.
Before this commit, the origin !== "explicit" clause was a safe shortcut: every non-explicit origin (groups, inferred) is only reached when authoredNav is missing or empty, so rootEntries was always [] and the .every() was vacuously true anyway. "inherited" is the first origin that carries real, non-empty root entries — so the clause short-circuits to true and skips the include-glob check that the comment directly above exists to perform.
Net effect: every page placed by a root-level include in an inherited tree gets reported as unplaced. That's the exact drift class this command was added to detect, and it's now noise for the pinned-source projects the commit was written to support.
The fix is a deletion — the .every() already handles all four origins correctly.
Technical details
Two fixtures resolving the identical navigation [{ include: "guides/*" }], one inherited from a synced source and one authored locally:
### inherited — exit 0
origin: inherited
pageCount: 2
unplaced: ["/docs/guides/auth","/docs/guides/setup"]
### local — exit 0
origin: explicit
pageCount: 2
unplaced: []
Equivalence of the suggested deletion, per origin:
| origin | reachable with non-empty authoredNav? |
rootEntries |
.every() |
|---|---|---|---|
explicit |
yes | authored entries | already the checked path |
inherited |
yes | authored entries | the case being fixed |
groups |
no (lines 310-312 require empty nav) | [] |
true |
inferred |
no (lines 310-314) | [] |
true |
[].every(...) is true, so groups/inferred keep their current behaviour.
| const rootIsLiteral = | |
| origin !== "explicit" || | |
| rootEntries.every( | |
| (entry) => typeof entry === "string" || !("include" in entry) | |
| ); | |
| const rootIsLiteral = rootEntries.every( | |
| (entry) => typeof entry === "string" || !("include" in entry) | |
| ); |
| const mounts = [ | ||
| { pathPrefix: "", urlPrefix: collection?.routePrefix ?? "/docs" }, | ||
| ...(collection?.mounts ?? []), | ||
| ]; |
There was a problem hiding this comment.
Inherited mounts are dropped, so inherited trees can print URLs the build will never serve.
mounts is in DEFAULT_SOURCE_CONFIG_INHERIT (src/config/inherit.ts:51-57), so a pinned source that owns its navigation typically owns its path remapping too. Lines 304-305 correctly read the post-inheritance authored?. first — this mount table doesn't, reading only the pre-inheritance collection?.mounts.
Since mounts is what maps pathPrefix → urlPrefix, dropping them means nav reports the default path-derived URL instead of the mounted one. For a read-only inspection command whose entire value is "show me the tree the build will actually produce", printing wrong urlPaths is worse than printing none.
Technical details
Fixture: remote collection, inheritConfig: true, synced source docs.config.ts declaring both a nav node at base: "api-v2" and mounts: [{ pathPrefix: "api-v2", urlPrefix: "/docs/api/v2" }], with docs/api-v2/widgets.mdx.
### leadtype nav — exit 0
origin: inherited
nav says urlPath: /docs/api-v2/widgets
inherited mounts: [{"pathPrefix":"api-v2","urlPrefix":"/docs/api/v2"}]
honouring mounts: /docs/api/v2/widgets
The last line is resolveDocsNavigation called with the same inherited nav but with the inherited mounts included — i.e. what generate produces. nav and generate disagree on the URL of every page under a source-declared mount.
Note this also feeds placedAtRoot at lines 342-346 via toDocsUrlPath(entry, mounts), so a mounted root-level string entry won't match its own page and will be double-counted as unplaced.
| const mounts = [ | |
| { pathPrefix: "", urlPrefix: collection?.routePrefix ?? "/docs" }, | |
| ...(collection?.mounts ?? []), | |
| ]; | |
| const mounts = [ | |
| { pathPrefix: "", urlPrefix: collection?.routePrefix ?? "/docs" }, | |
| ...(authored?.mounts ?? collection?.mounts ?? []), | |
| ]; |
| if (declared && Object.values(declared).some((c) => c.inheritConfig)) { | ||
| try { | ||
| collections = await inheritCollectionSourceConfigs(declared, configDir); | ||
| navigationWasInherited = | ||
| collections[collectionKey]?.navigation !== undefined && | ||
| declared[collectionKey]?.navigation === undefined; | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| io.stderr.write( | ||
| `Warning: source-owned config could not be read, so inherited navigation is not shown: ${message}\n → Run \`leadtype sync\` first.\n` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Inheritance is all-or-nothing across collections, so one unsynced collection silently degrades every other collection's tree.
inheritCollectionSourceConfigs loops all declared collections and throws on the first unreadable source config (src/config/inherit.ts:383-400). The catch here discards the whole result — including the collections that read fine. So --collection good gets a filesystem-inferred tree because unrelated collection bad wasn't synced.
The stderr warning is the only signal: exit code stays 0, and --json reports ok: true with origin: "inferred" and no error field. doctor handles this same failure by recording a structured source.inherit-failed issue (doctor.ts:563-605, documented at docs/reference/doctor.mdx:140) — nav --json has no equivalent, so anything consuming the JSON can't tell a genuinely-inferred tree from a degraded one.
Suggested direction (no diff attached since there are a few reasonable shapes): if a collection was explicitly selected via --collection, only its own inheritance failure should degrade its output; and NavReport should carry the failure so --json consumers see it.
Technical details
Fixture: two remote collections. good is fully synced with a curated source-owned navigation ([{ title: "Curated Section Title", base: "guides", pages: ["zzz", "aaa"] }]). bad declares inheritConfig and was never synced. Same command both times; the only difference is whether bad exists in the config at all:
### --collection good, with unsynced sibling — exit 0
stderr: Warning: source-owned config could not be read, so inherited
navigation is not shown: ...
→ Run `leadtype sync` first.
ok: true
origin: inferred
tree: [["Guides",["Aaa","Zzz"]]]
### --collection good, sibling removed — exit 0
stderr: (none)
ok: true
origin: inherited
tree: [["Curated Section Title",["Zzz","Aaa"]]]
good is byte-identical and fully synced in both runs. Wrong section title, wrong page order, ok: true, exit 0.
f7e9e4b to
f960b35
Compare
f960b35 to
31feb68
Compare
31feb68 to
590e717
Compare
Config-owned navigation is the strength — one resolved tree drives the sidebar, llms.txt, AGENTS.md, the sitemap, and Agent Readability metadata. The cost shows up on large sites: a curated tree earns its keep while every entry is a decision, and stops earning it when a section is mostly inventory. Twenty pages where three must lead and the rest could be alphabetical means every new page gets added twice, on disk and in the config, and the two drift. `navigation.fromDirectory()` includes a directory without listing it, and the new `pin` option keeps the pages whose position is a decision at the front. Pinned pages lead in the order given; the rest follow in sort order, so adding a page appends it to the tail and never displaces a deliberate choice. Explicit refs and an expansion can share one `pages` array, so a section can mix curated entries with a derived tail. Guardrails, because the point is to keep curation rather than replace it with opaque filesystem behaviour. Expansion still happens once during navigation resolution — the helper only builds config — so one resolved tree keeps feeding every surface. A pin that matches nothing is an error rather than a silent no-op; that is almost always a rename the config missed. `leadtype nav` closes the gap expansions open: once globs and pins are in play the tree you get is several steps removed from the tree you wrote. It prints the resolved result and names what drifted — pages no curated entry places (they render, at the root, by default rather than by decision), pages two entries both claim (twice in the sidebar, twice in llms.txt), and pages whose `group:` names a group the config never declares. Human and JSON output, per collection, read-only. The fixture proves the claim the issue asks for: a three-framework tree with a ten-page section each resolves byte-identically hand-listed and derived, and the derived form is smaller.
This file is committed but was already stale on main — it was missing changelog/0-3, 0-4, and the aeo pages. Running check-types regenerates it, so a stale copy dirties the tree for anyone who runs the monorepo checks. Adds the two pages this stack introduces along with the pre-existing gap.
Running `nav` on the migrated c15t example reported an inferred tree with zero pages, for a 254-page project whose navigation is inherited from its source repository. Two bugs, the same two doctor had: A remote collection's `dir` is relative to its checkout, not the config directory. Resolving it as the latter pointed at `apps/c15t-example/docs`, which does not exist, so every page went missing — `resolveCollection` is what knows the difference. A dir that still isn't there now fails saying to run `leadtype sync`, rather than reporting an empty tree as if that were the answer. And config loading does not apply source-owned inheritance, so `nav` was describing a pre-inheritance project. It now applies it through the same shared implementation generation uses, and reports `inherited` as a distinct origin. Against real c15t content it now resolves all 254 pages under the seven sections c15t actually publishes, and finds drift in that config worth having: 63 pages absent from the curated tree, and `/docs/ai-agents` claimed by two entries, so it appears twice in the sidebar and twice in llms.txt.
590e717 to
55c2d6a
Compare
1 participant
Closes #156. Stacked on #163, final PR of #157.
Config-owned navigation is the strength — one resolved tree drives the sidebar,
llms.txt,AGENTS.md, the sitemap, and Agent Readability metadata. The cost shows up on large sites: a curated tree earns its keep while every entry is a decision, and stops earning it when a section is mostly inventory. Twenty pages where three must lead and the rest could be alphabetical means every new page gets added twice, on disk and in the config, and the two drift.Pinned pages lead in the order given; the rest follow in sort order, so adding a page appends it to the tail and never displaces a deliberate choice. Explicit refs and an expansion can share one
pagesarray, so a section can mix curated entries with a derived tail.Guardrails
The point is to keep curation, not replace it with opaque filesystem behaviour:
leadtype navOnce globs and pins are in play the tree you get is several steps removed from the tree you wrote.
leadtype navprints the resolved result and names what drifted:llms.txt)Human and JSON output, per collection, read-only.
The size claim, proven
The fixture is a three-framework tree with a ten-page section each. Hand-listed and derived resolve byte-identically, and the derived form is smaller — pin the three pages whose position is a decision, let the seven-page tail sort.
The scaffold-only [scaffold/update command] from the issue's "explore a combination of" list is deliberately not included: the diagnostics should prove themselves before anything writes config.
Made to work against pinned remote sources
Running
navon the migrated c15t example reported an inferred tree with zero pages, for a 254-page project. Two bugs, the same two doctor had:diris relative to its checkout, not the config directory. Resolving it as the latter pointed atapps/c15t-example/docs, which does not exist, so every page went missing. A dir that still isn't there now fails saying to runleadtype sync, rather than reporting an empty tree as if that were the answer.navwas describing a pre-inheritance project. It now applies it through the shared implementation and reportsinheritedas a distinct origin.Against real c15t content it resolves all 254 pages under the seven sections c15t publishes, and finds drift in that config worth having: 63 pages absent from the curated tree, and
/docs/ai-agentsclaimed by two entries — so it appears twice in the sidebar and twice inllms.txt.