fix!: reconcile the duplicated rules behind folder URLs, nav titles, and sorting by gitKrystan · Pull Request #370 · universal-ember/kolay

gitKrystan · 2026-08-14T17:00:05Z

This library has several "rules" around URLs, titles, sorting that relied on duplicate and/or inconsistent applications. This PR attempts to resolve those duplications and fix several bugs / missing features along the way.

For example, authoring is a folder in the kolay docs, containing index.gjs.md + five siblings, but it has no page of its own, so kolay.nullvoxpopuli.com/authoring currently shows Page not found for path "/authoring".

This change allows a folder URL like that to redirect to the tree's first page.

My Claude claims this affects three cases. I don't know enough about how kolay's docs are set up to validate this claim:

A folder nested inside a group, /Group/sub-folder.
A folder in the co-located Home group, whose pages live in the root URL space, so its URL is a single segment (e.g. /authoring above).
A group's own root, on every mount shape: /Group on a top-level mount, /guides on a nested one. Both changed during review. See below.

(Builds on #368; the new helpers follow its PageTree vocabulary.)

Fixes along the way

Found while reconciling the above:

<PageNav />'s active-link check compared paths case-sensitively, while routing is deliberately case-insensitive — so a page reached at /Docs/Sub-Folder/PAGE.md rendered but its nav link never highlighted.
parse.js and setup.js carried two spellings of the glimmer-markdown extension rule.
reshape's configs was typed string[] while every caller passes { path, config } pairs.

Claude dump: How it works

Redirecting beats resolving. Rendering the index page at the tree's own URL would give one page two URLs, and is-active compares currentURL through samePagePath, so the second URL would highlight nothing in the nav.

The destination is the tree's first page. Sorting puts a folder's index page at the top of it, so a single rule covers both cases: the index page where there is one, the first ordered page where there isn't. That is what group.list[0] does at a group root.

It hangs off routeWillChange, not a route hook. This is the part worth reviewing. A mount route has no dynamic segment of its own, so once it is active Ember does not re-enter it when only the wildcard's param changes — a beforeModel hook there never fires for navigation within the mount. Since properLinks turns an authored markdown link into an in-app transition, that is how readers actually reach these URLs, so a hook would work on a hard load and do nothing on a click. It sits next to the config redirects in #setupRedirects, and needs the same arrival-correction half: setup runs inside the application route's model hook, mid-initial-transition, so that transition's routeWillChange has already fired.

Group roots resolve here too, and this took two passes to get right. The first revision kept them on handlePotentialIndexVisit, on the theory that wiring the hook up was the app's choice. Review showed the opt-in could not be exercised: on a nested mount the hook can only live on the mount route, which is exactly the route that never re-enters.

Removing that guard fixed group roots that sit in the wildcard, which is a top-level mount. It did not fix a nested mount's own URL (/help, /demos, and /Runtime on this very site), which carries no wildcard param at all — so the mechanism declined it and the hook still could not fire. Clicking a group's own nav link from a page inside that mount landed on a blank index. A second review caught it; it is reproduced by two tests and fixed in fc1b00a. handlePotentialIndexVisit now owns the app root (/) alone, which names no group for a transition to resolve.

Three more things worth a close read:

Ordering. The tree lookup runs only after a page lookup misses. An ordinary page visit resolves to the wildcard's index too, with the page as the wildcard param, and has to be left alone.
Which lookup. Manifest-wide, not findByPath. findByPath searches currentGroup, which derives from router.currentURL — mid-transition that still names the previous page.
Mount translation. The wildcard holds only the part of the URL below the mount, giving three cases: a scoped mount names its group in the binding, an unscoped nested mount takes the group as its own path (the route above the wildcard), and a top-level mount's wildcard already includes it. The group's URL prefix is read from group.tree.appRelativePath rather than assumed to equal the group's name — a review finding, and the reason a scoped Home mount works, since Home's prefix is the root.

New API: indexPageFor(tree) and indexPageForPath(appRelativePath, groupName?) on the docs service, plus findPageTree in browser/page-tree.ts. Callers holding a tree use the first, which needs no group: the group parameter guards the search for a tree at a path, and there is no search when the tree is in hand. The first page itself comes from PageTree.first, which the build already computes. Passing a group scopes the search, which a scoped mount needs: two groups can hold one manifest path, and the root-dwelling co-located group makes that reachable. A name matching no group answers undefined rather than widening back to every group — the lenient version returned a shadowed page from the wrong group, caught by its own test.

mountLocationFor in scoped-routes.ts owns the route-info parsing both this and handlePotentialIndexVisit had a copy of — between them they encoded all three mount shapes. handlePotentialIndexVisit keeps its own !wildcardParam guard, because the two callers want the mount's group under opposite conditions: one redirects to it, the other scopes a lookup with it.

The wiring lives in services/page-tree-redirects.ts, with the routeWillChange listener, its destructor and the boot dance in services/redirect-wiring.ts, shared with the authored redirects. That follows what #372's 6c0f0ad did for search. DocsService still grows over the branch, 534 lines to 558.

A folder's index page is now one idea, and an inclusive one, which is where several of these bugs lived. It is the page named index when the folder has one and the folder's first page otherwise, so every folder with pages has one and a folder heading can always be a link. Previously isIndex tested the path while sorting tested the name, so a folder holding api-index.md had sorting, getIndexPage and <PageNav /> giving three different answers, with that page linked from nowhere and listed nowhere.

An earlier revision split this into two public concepts, an index page and a "landing" page. Review rejected the split: the only use case for telling them apart is asking whether a folder's heading already says what a page's link would say, which is a title comparison. That is now isRedundantWithHeading(folder, page).

getIndexPage(tree) keeps its name and takes the inclusive definition, which is what stops index-less folders going unlinked. It reads PageTree.first, so it cannot disagree with where the folder's URL redirects. isIndex is removed: whether a node is named index does not on its own answer anything a nav asks. <PageNav />'s :section still yields index, now present for every folder with pages.

Titles resolve in one place. setup.js resolved title ?? first heading ?? cleaned name for search while the manifest kept only the authored title, so a page with just an # H1 was titled by its heading in search and by its filename in the nav. Titles now resolve once, on the manifest pages, where the source is in hand. PageTree carries a title too, so an app no longer derives section headings itself: a folder's own meta.json title, then its index page's title, then its cleaned name.

Behavior changes not in the upgrade guide

A folder's index page sorts first regardless of extension. Moved out of the upgrade guide at review request — a bugfix, not a migration step.

index.md has always been hoisted to the top of the folder holding it. index.gjs.md and index.gts.md were not: the build strips those extensions before sorting runs, so the test never matched. They now sort first too.

Two things move on any folder with a .gjs.md or .gts.md index and no meta.json order:

The nav lists the index page first, where it used to appear in alphabetical position.
group.list[0] becomes that index page, and with it the page a group's own URL resolves to.

A folder with a meta.json order is unaffected — an explicit index is hoisted before the order is applied, so it cannot be placed second.

To keep the old placement, give that folder a meta.json order. Build-time sorting also matches the node's name rather than its path now, so a folder named index sorts first among its siblings.

Manual QE

Visit these paths.

Visit	Expect
`/authoring`	redirects to `/authoring/index`, no error page
`/development`	redirects to `/development/rendering-pages` (no index page in that folder)
`/authoring/`	same as `/authoring`
`/AUTHORING`	same as `/authoring`
`/authoring/code-fences`	stays put — an ordinary page URL must not redirect
`/authoring/not-a-real-page`	still shows the error page
`/Runtime`, from a page already inside Runtime	redirects — this was the second review's finding, and is what the group nav links to
a folder with no index page, in the sidebar	renders as a link now, not plain text

After each redirect, check the nav highlights the destination page — that is the failure mode resolving-in-place would have caused, and it is not covered by the URL assertions alone. Browser back from a redirected URL should also leave you above it rather than bouncing forward again.

_{AI attribution: 🤖 Claude using Opus 5 + Krystan yelling at it a whole bunch.}

bolt-new-by-stackblitz · 2026-08-14T17:00:08Z

Run & review this pull request in StackBlitz Codeflow.

vercel · 2026-08-14T17:00:10Z

@gitKrystan is attempting to deploy a commit to the universal-ember Team on Vercel.

A member of the Team first needs to authorize it.

gitKrystan

Correctness review of the folder-index redirect

One careful pass over the diff, with the surrounding manifest pipeline (setup.js, hydrate.js, parse.js, sort.js) read for context. Nine comments below: two I verified by running code, three reasoned findings, four nits.

Two of these were confirmed by execution, not by reading:

The redirect never fires for in-app navigation inside a nested mount. I added a throwaway probe to test-apps/multiple-docs-routes and ran pnpm test:ember; both the scoped (/help) and unscoped (/demos) mounts fail. Details in the comment on the docs change.
The "sorting has already hoisted index.md" invariant is false for .gjs.md / .gts.md index pages. Confirmed against the real reshape(). Details in the comment on firstPageIn.

The design itself holds up well. I checked path construction for all four mount shapes, the page-visit guard, case-insensitivity, trailing slashes, rootURL stripping through appRelativeHrefFor, the absence of a redirect loop on the destination URL, the untouched group-root and app-root branches, and that firstPageIn's depth-first order agrees with getList (so it really does match group.list[0]). All four new test expectations match what the real sort produces. preAddCheck rules out the folder/page name collision I went looking for. Leaving the new helpers off the public entrypoint is right, and landingForPageTree's JSDoc flows into the existing <APIDocs @name="DocsService" /> page, so no manual docs are owed.

_{🤖 Drafted by Claude (Opus 5) as a correctness review. Not yet checked by a human.}

@gitKrystan

Addresses @gitKrystan's review. A collection group with no `src` names no group, so nothing in `availableGroups` resolved its URL and `/Packages` rendered a missing page. `handlePotentialIndexVisit` now falls back to the navigation: `navEntryNamed` looks an entry up by its own name, and the redirect lands on the first group it collects. Related to universal-ember#370, which covers the adjacent case of a page tree with no page of its own; a collection group is neither a page tree nor a group, so neither path reached it. The redirect needs the catching route to call `handlePotentialIndexVisit`, same as a group root does. The test app only wired that on its two nested mounts, so its top-level mount gets a route too — which is what the docs have always told apps to do. Also, per review: - Both nav assertions are reworded; the wording about "the groups nothing collects" was as confusing in the error as it was in the docs. The docs quote the new text verbatim. - The `collection` section is restructured to her suggestions: the before/after moves either side of the example, an example for a collection group WITH an `src`, and a before/after diff for the `availableGroups` → `navEntries` switch. `navEntryNamed` compares case-insensitively inline rather than reusing `equalsIgnoreCase`: this module's unit tests run in node, and `browser/utils.ts` imports `@ember/debug`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

gitKrystan

Round 2: the `routeWillChange` move, reviewed on its own

Both fixes verified by running them, not by reading. Summary of what I checked:

Claim	Verdict
In-mount folder navigation now redirects	Holds. My own probe passes for both mount shapes where it failed before; your four new tests pass; 19/19 in that app
The boot path still works (`checkArrival`)	Holds. `setupKolay` runs in the application route's `model()`, so application tests really do exercise setup mid-initial-transition
Index hoisting for `.gjs.md` / `.gts.md`	Holds. Verified against the real `reshape()`; `pnpm test:node` 167/167; migration entry accurate
`betterSort` safe to compare `aFull`	Holds. Not exported through `kolay/build`, so `betterSort('name')` is the only caller. Comparator is now consistent too, which the old one wasn't
Broken `test:ember` in the two parked apps	Your correction is right, and it's worse than my framing — there's no working way to run those apps at all

The routeWillChange move is a better answer than the one I suggested. Making it a property of the library instead of per-app wiring removes the sharp edge rather than documenting it.

One thing it did not fix, and I think it should: group roots are still on beforeModel, so the exact bug from round 1 survives for /help and /demos reached from inside their mounts. Verified the same way. Comment below.

A hazard I tested and found safe, so you don't have to wonder: two routeWillChange handlers now both call transitionTo — the config redirects and the page-tree landing. I expected the second to clobber the first. It doesn't; the configured redirect wins. Details and a suggestion below, since nothing pins that.

Four nits besides. Nothing here blocks; the group-root asymmetry is the only one I'd want resolved or explicitly deferred before merge.

_{🤖 Drafted by Claude (Opus 5) as a correctness review. Not checked by a human before sending.}

@gitKrystan

Addresses @gitKrystan's review. A collection group with no `src` names no group, so nothing in `availableGroups` resolved its URL and `/Packages` rendered a missing page. `handlePotentialIndexVisit` now falls back to the navigation: `navEntryNamed` looks an entry up by its own name, and the redirect lands on the first group it collects. Related to universal-ember#370, which covers the adjacent case of a page tree with no page of its own; a collection group is neither a page tree nor a group, so neither path reached it. The redirect needs the catching route to call `handlePotentialIndexVisit`, same as a group root does. The test app only wired that on its two nested mounts, so its top-level mount gets a route too — which is what the docs have always told apps to do. Also, per review: - Both nav assertions are reworded; the wording about "the groups nothing collects" was as confusing in the error as it was in the docs. The docs quote the new text verbatim. - The `collection` section is restructured to her suggestions: the before/after moves either side of the example, an example for a collection group WITH an `src`, and a before/after diff for the `availableGroups` → `navEntries` switch. `navEntryNamed` compares case-insensitively inline rather than reusing `equalsIgnoreCase`: this module's unit tests run in node, and `browser/utils.ts` imports `@ember/debug`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@gitKrystan

Addresses @gitKrystan's review. A collection group with no `src` names no group, so nothing in `availableGroups` resolved its URL and `/Packages` rendered a missing page. `handlePotentialIndexVisit` now falls back to the navigation: `navEntryNamed` looks an entry up by its own name, and the redirect lands on the first group it collects. Related to universal-ember#370, which covers the adjacent case of a page tree with no page of its own; a collection group is neither a page tree nor a group, so neither path reached it. The redirect needs the catching route to call `handlePotentialIndexVisit`, same as a group root does. The test app only wired that on its two nested mounts, so its top-level mount gets a route too — which is what the docs have always told apps to do. Also, per review: - Both nav assertions are reworded; the wording about "the groups nothing collects" was as confusing in the error as it was in the docs. The docs quote the new text verbatim. - The `collection` section is restructured to her suggestions: the before/after moves either side of the example, an example for a collection group WITH an `src`, and a before/after diff for the `availableGroups` → `navEntries` switch. `navEntryNamed` compares case-insensitively inline rather than reusing `equalsIgnoreCase`: this module's unit tests run in node, and `browser/utils.ts` imports `@ember/debug`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

gitKrystan · 2026-08-14T22:37:40Z

Answering both review summaries here, since neither is anchored to a line: round 1 and round 2. Every inline thread has its own reply; this is the part that doesn't fit in one.

Both round-1 findings were real, and I verified each independently before touching anything rather than taking the run output on trust. The in-mount probe failed for me the same way on both mount shapes, and the extension-stripping traces exactly as described from parse.js:129 into sort.js.

What landed across the two rounds:

The redirect moved off beforeModel onto routeWillChange. Fixing that surfaced a second bug the review hadn't caught: my first attempt repaired in-mount navigation and broke fresh visits, because setup runs mid-initial-transition, so that transition's routeWillChange had already fired. #setupRedirects solves this with a checkArrival half I had skipped.
The index hoist is its own fix!: commit, since group.list[0] had the same exposure and the invariant was worth making true rather than documenting around.
The group-root guard is gone. That argument was the one that moved me: an opt-in nobody can exercise is not a choice.
Both docstrings, the migration guide, and the docs page corrected. The prose pass caught a factual error of its own — the docs page still claimed group roots need the hook, which dropping the guard had just made false.

Two things deliberately left open, both with reasoning on their threads: pinning redirect precedence, and scoping the tree search to the mount's group. Neither is forgotten; both are stated as not-done rather than quietly closed.

One correction to this PR's own description. An earlier revision reported four custom-root-url failures as pre-existing on main. That was wrong. My node_modules still held @universal-ember/test-support 0.7.0, which keys the crawl on (source, target) pairs rather than the target alone; #369 had bumped it to 0.9.0 and I rebuilt without reinstalling. Worse, I "verified" it by checking out main detached and re-running — which felt controlled and wasn't, since detaching moves the lockfile and leaves the install alone, so both arms ran the same stale library. Corrected in the description rather than edited out.

Also noting the round-2 retraction on the two routeWillChange handlers clobbering each other. Withdrawing it after building the competing config is the right shape, and the residual point stands: the ordering is correct by construction and nothing pins it.

_{🤖 Reply drafted by Claude (Opus 5). Not checked by a human before sending.}

`betterSort` hoists an index page via `a.path.endsWith('index.md')` and `endsWith('index.gjs.md')`, but `build()` has already stripped that extension by the time the comparator runs — `parse.js` does `mdPath.replace(/\.g(j|t)s\.md$/, '')`, so a `.gjs.md` index arrives as `/foo/index` and matches neither branch. The `index.gjs.md` test was dead and `.gts.md` was never covered; only plain `.md` was ever hoisted. Compare the node's `name` instead. `betterSort` is only ever called as `betterSort('name')`, and `name` is the basename through `stripExt`, which is `index` for all three extensions. Against the real `reshape()`, before and after: ['foo/apple.md', 'foo/index.gjs.md'] -> apple, index => index, apple ['foo/apple.md', 'foo/index.gts.md'] -> apple, index => index, apple ['foo/apple.md', 'foo/index.md'] -> index, apple => unchanged Breaking, so it is in the v6 migration guide: a folder with a `.gjs.md` / `.gts.md` index and no `meta.json` order changes nav order, and `group.list[0]` with it. Folders with an explicit order are unaffected — `applyPredestinedOrder` hoists `index` on its own already. The callsite sorts folders alongside pages, so a folder named `index` now hoists too. The existing test paired `name: 'c'` with `path: '/c/index.md'`, which the build never produces; the path-based hoist is why that went unnoticed. Fixture corrected, and the extension-stripped case — the one the old hoist missed — added. Both `.gjs.md` and `.gts.md` reach the comparator as the same input, so one test covers them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

gitKrystan · 2026-08-19T02:14:50Z

+| a page   | its json `title`, then its first heading, then its cleaned filename   |
+| a folder | its `meta.json` `title`, then its index page's title, then its cleaned directory name |
+
+Cleaned names have digits removed, dashes turned into spaces, and are sentence-cased.


Suggested change

Cleaned names have digits removed, dashes turned into spaces, and are sentence-cased.

"Cleaned names" have digits removed, dashes turned into spaces, and are sentence-cased.

Took the first half, corrected the second, because I checked the implementation and "sentence-cased" claims more than happens.

cleanSegment is stripExt(segment.replaceAll(/\d/g, '').replaceAll('-', ' ')).trim() — digits out, dashes to spaces, and no casing at all. The capitalization is elsewhere, in titleFor, and it is only charAt(0).toUpperCase() + slice(1): the first character, with the rest untouched. Nothing is lowercased, so inIframe stays inIframe rather than becoming Iniframe.

The distinction matters because cleanedName is a public field — a consumer reading it directly gets the uncapitalized string. It now reads:

A "cleaned name" has its digits removed and its dashes turned into spaces. Where one stands in for a title, its first character is capitalized — the rest is left alone, so inIframe keeps its shape.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T02:16:11Z

 ## `isPageTree`

-Type guard that returns `true` if the given node is a `PageTree` (a folder of pages) rather than a `Page`.
+Type guard: `true` for a `PageTree` (a folder), `false` for a `Page`.


Suggested change

Type guard: `true` for a `PageTree` (a folder), `false` for a `Page`.

Type guard that returns `true` if the given node is a `PageTree` (a folder of pages) rather than a `Page`.

This change seemed unnecessary.

Agreed, reverted to your text exactly. My version was a reword with no new information in it.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T02:17:36Z

+for (const node of folder.pages) {
+  console.log(isPageTree(node) ? 'folder:' : 'page:', node.title);


Is the previous example still accurate? It has more detail.

Suggested change

for (const node of folder.pages) {

console.log(isPageTree(node) ? 'folder:' : 'page:', node.title);

for (const node of tree.pages) {

if (isPageTree(node)) {

console.log('folder:', node.name, node.pages.length, 'children');

} else {

console.log('page:', node.name, node.path);

}

It was not, and your version is back. The detail is the point of the example — node.pages.length and node.path are what show you the two branches yield different shapes, which a shorter example just asserts.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T02:18:03Z

 ```

-## `getIndexPage`
+## Where a folder's heading links


Suggested change

## Where a folder's heading links

## A folder's index page

Went the other way, now that the section documents a named export rather than a concept: it is ## \getIndexPage`, matching the ## `isPageTree`and## `isRedundantWithHeading`` sections either side of it.

Your sentence survives as the section's first line, which is where it reads best anyway: "A folder's index page is where its own URL goes: the page named index when there is one, and the folder's first page otherwise."

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T02:18:59Z

-for (const node of tree.pages) {
-  if (isPageTree(node)) {
-    const index = getIndexPage(node);
+const indexPage = docsManager(this).indexPageFor(folder);


What is this in this example?

Nothing, now — the example no longer has one. It was docsManager(context).indexPageFor(folder), which needed an owner and therefore needed explaining.

getIndexPage(folder) is a plain function on the tree, so the example is one line with no context to hand-wave:

import { getIndexPage } from 'kolay'; const indexPage = getIndexPage(folder);

The service method is still mentioned as the equivalent for anyone who already holds the service.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T02:20:33Z

+         * The folder's index page: the page named `index` when there is one,
+         * and the folder's first page otherwise. Absent only for a folder
+         * with no pages. A page it links to under the same title is left out
+         * of the `:page` block.


Suggested change

* The folder's index page: the page named `index` when there is one,

* and the folder's first page otherwise. Absent only for a folder

* with no pages. A page it links to under the same title is left out

* of the `:page` block.

* The folder's index page: the page named `index` when there is one,

* or the folder's first page otherwise. Absent only for a folder

* with no pages. A page it links to with the same title is omitted

* from the `:page` block.

Applied, with one wording change. Your "or the folder's first page otherwise" and "Absent only for a folder with no pages" are both in. I kept "under the same title" rather than "with the same title", because the omission is decided by comparing titles rather than by identity — a folder titled from its meta.json says something its index page does not, and that page stays in the list.

That rule is public now too, as isRedundantWithHeading, so the block comment no longer has to be the only place it is written down.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T02:21:56Z

+const isFolderOwnPage = (folder: Page | PageTree, page: Page | PageTree) =>
+  !isPageTree(page) && 'title' in folder && Boolean(folder.title) && page.title === folder.title;


Why is this so much more complicated than the example in the docs?

If this level of complexity is needed, we should have a public helper that can be called here.

and could this be better titled as isIndex (breaking change to existing isIndex API)?

Both addressed in d03bb51.

On the helper: it is public now, as isRedundantWithHeading(folder, page), exported from kolay and documented on the page-tree-utils page with the ejection use case spelled out. Your point stands — if <PageNav /> needs the rule, so does anyone re-implementing it.

On the name: not isIndex. The published isIndex asks whether a node is named index, which is a genuinely different question and, on its own, not one a nav needs — so it stays removed rather than getting new arguments. isRedundantWithHeading says what it decides.

On the complexity vs. the docs example: the docs now show the real thing rather than a simplification, so they cannot drift apart again.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T03:57:42Z

    return docsManager(this);
  }

+  private indexPageFor = (tree: PageTree) => this.docs.indexPageFor(tree);


Is passing this to pages necessary bc pages doesn't have access to the docs service?

It was, and that is gone now.

Pages is a TOC, so it had no owner to reach the docs service through — hence the callback threaded down from the component. Making the index lookup a pure function over the tree (getIndexPage) removed the need: Pages calls it directly, and the @indexPageFor arg, the private indexPageFor on the component, and both places it was forwarded through the recursion are deleted (d03bb51).

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:00:58Z

+   * The index page of the tree at a manifest-space path. `undefined` when the
+   * path names a page, or nothing. Pass `groupName` when you know which group
+   * serves it: two groups can hold one manifest path.


This comment is confusing. undefined or nothing? "when you know which group serves it" - what is "it"?

Reworded. "It" was doing too much work; the comment now names the path both times:

The index page of the tree at a manifest-space path, or undefined when no tree is there — because the path names a page, or nothing at all.

Pass groupName when you know which group serves that path: two groups can hold the same one.

The "undefined or nothing" ambiguity was real — those are two distinct reasons for the same answer, and the method could not tell you which. Spelling both out is the honest version.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:01:22Z

+   * serves it: two groups can hold one manifest path.
+   */
+  indexPageForPath = (appRelativePath: string, groupName?: string): Page | undefined => {
+    let groups = this.manifest?.groups ?? [];


let with all the re-assignments is confusing me

Fixed. It is three consts now — scoped, searched, then the filtered groups — with the early return for an unknown group name in between, instead of one binding reassigned down the method.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:04:54Z

+export class PageTreeRedirectService {
+  @service declare private router: RouterService;
+
+  get #docs() {


should this be cached?

Already is, and I checked rather than assumed. docsManager returns createStore(owner, DocsService), and createStore keys a WeakMap on the context — its own doc comment says repeat accesses "return a stable reference as if @cached were used", and shows the getter form as the intended way to make it lazy.

So the getter is one lookup in a WeakMap per access, and adding @cached would cache a cache.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:07:49Z

+    const path = trimSlashes(entry.appRelativePath, { leading: true });
    const url = `${base.endsWith('/') ? base : `${base}/`}${path}`;


It seems like trimSlashes is mostly used for path concatenation. I wonder if it would be better do make a helper for concatenating the path like:

concatenatePath(a: string, b: string): string

Done in db52dd2, with your name for it.

You were right about the pattern: of five trimSlashes callsites, three were the same join, each punctuating it differently — search.ts tested base.endsWith('/'), page-tree-redirects.ts pre-trimmed the left side instead. concatenatePath(left, right) trims both, so no caller has to know which side carries the separator.

The other two callsites are genuine normalization, not joins, so they still call trimSlashes directly. paths.js had no tests at all; it has some now, including the root-left-side case (concatenatePath('/', 'docs/page.md')) that both join sites hit.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:08:43Z

+  url: string,
+  redirects: { from: string; to: string }[]
+): string | undefined {
+  const [path = ''] = url.split(/[?#]/);


Whoa...I'm not sure I've seen this form of destructuring w/ a default before?! Had no idea

It is the same defaulting you would write in a parameter list, applied to a destructured property — const { a = 1 } = obj gives 1 when a is absent or undefined (not when it is null or 0).

No change here; flagging only that this is the mechanism trimSlashes uses for its opt-in ends.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:11:17Z

 */
 export function samePagePath(a: string, b: string): boolean {
-  return equalsIgnoreCase(a.replace(/\.md$/i, ''), b.replace(/\.md$/i, ''));
+  return equalsIgnoreCase(stripMarkdownExtension(a), stripMarkdownExtension(b));


Can we review all the places we're calling stripMarkdownExtension to see if we should be accounting for .gjs.md and .gts.md in these places also?

Reviewed all of them, and no change is needed — the extensions never reach that code.

The build strips them before anything is written to the manifest. Verified by running reshape directly:

['a/x.gjs.md', 'a/y.gts.md', 'a/z.md'] -> ['/a/x', '/a/y', '/a/z.md']

So a .gjs.md page's path is already extension-free, and plain .md is the only spelling samePagePath can ever see. Every callsite compares a manifest path against a URL, and kolay neither emits nor links /a/x.gjs.md — a reader who hand-types it gets the error page, which is right, since that is not the page's URL.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:15:28Z

+ * @returns {string[]}
+ */
+export function headingsIn(source) {
+  return [...source.matchAll(/^#{1,6}\s+(.+?)\s*#*\s*$/gm)].map((match) =>


Add inline comments for each regex explaining what it's supposed to do

Added. Each of the three now says what it matches: the ATX heading itself (hashes, text, optional closing hashes), the [^label] a footnote reference leaves behind, and the emphasis/code marks.

Drafted by Claude, reviewed before posting.

gitKrystan · 2026-08-19T04:16:32Z

-  // vanishes while the crawl stays green.
+  // The heading renders the folder's resolved title, which is its cleaned
+  // name (dashes become spaces). Rename the `:section` block and every
+  // heading silently vanishes while the crawl below stays green.


Gone. I also swept the branch for others and found one more I had introduced, in docs-app's runtime page comment — "silently drops any later scoped block" is now "ignores". The only remaining use is in setup.js, which predates this branch.

Drafted by Claude, reviewed before posting.

…thing The page taught two placements that no longer do anything. Usage showed `routes/page.ts`, and the nested-mount section showed a mount route — both redirect on their own now. The one placement that still matters, the top-level index route, was a sentence at the end. It exists because `/` is the app's URL, not kolay's: a folder or group URL is unambiguously docs, so kolay redirects those itself, and `/` might be a landing page. The Optro docs app is the case in point — it renders a homepage at `/` and has the hook on `routes/page.ts`, where it is a no-op. `page-tree-utils` is back to its original text apart from what actually changed: `getIndexPage` replaced by a folder's index page, and a titles table. The rewrite had lost detail that was better before. Migration guide takes the suggested wording, and the extension-hoist section moves below the redirect one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… regexes `isFolderOwnPage` describes the mechanism; `isIndex` describes what the caller is asking. The helper is module-scoped, so this does not put the removed public `isIndex` export back.

The concept is gone from the public API; the words should go with it.

An ejected `<PageNav />` needs the two rules the component applies: which page a folder's heading links to, and which page that heading makes redundant. Both were private, so a hand-rolled nav could not reproduce them. `getIndexPage(tree)` comes back with the index-page definition this PR settles on, reading `tree.first` so it cannot disagree with where the folder's URL redirects. `isRedundantWithHeading(folder, page)` is the title comparison `<PageNav />` was doing inline. `isIndex` stays removed: whether a node is named `index` does not on its own answer anything a nav asks. Splitting the pure tree queries into `page-tree.ts` is what lets them be unit tested — `utils.ts` imports `@ember/debug`, which vitest cannot load. `equalsIgnoreCase` and `samePagePath` move to `paths.js`, next to the `stripMarkdownExtension` the latter is built from. Two things fall out: `Pages` no longer threads an `indexPageFor` callback down to reach the docs service, and `selected.ts` uses the shared extension strip rather than its own regex.

`isIndexName` is build-only now that the browser reads `tree.first`, so its comment no longer describes a rule both halves share, and it moves next to the two files that use it. The titles table said a folder takes its title from "its index page", which now reads as the inclusive index. It is the explicit one — a folder titled by its first page would make that page redundant with its own heading and drop it from the nav. Drops the one "silently" this branch introduced.

gitKrystan · 2026-08-19T05:17:13Z

+
+`getIndexPage(tree)` follows the same change, so it now answers for every folder with pages where it used to answer `undefined`. Two smaller shifts come with it: it can descend into a first child folder, and it matches the page actually named `index` rather than any path ending in `index` — `api-index.md` no longer counts.
+
+`isIndex` is removed. It asked whether a node was named `index`, which on its own does not answer anything a nav needs. If you were using it to decide what a folder's heading links to, that is `getIndexPage(tree)`; to decide whether listing a page repeats its folder's heading, that is `isRedundantWithHeading(folder, page)`.


isRedundantWithHeading should probably only be true if the page is the index/landing page right?

You are right, and it was a real bug — thank you. Fixed in f6a0b39.

Title-only meant a page that merely happened to share its folder's title was dropped from the nav even when the heading linked somewhere else. Concretely: a folder titled Guides by its meta.json, holding an index page titled Overview and a page titled Guides. The heading says "Guides" and links to Overview; the page titled Guides was hidden, and that list was the only place it appeared. Unreachable.

Both halves are needed though, not identity alone — identity alone would hide an index page whose title differs from its folder's, and that one is worth showing precisely because it says something the heading does not. So it is now: the page is the folder's index page and their titles match.

The unit fixtures that missed this used pages: [], which is not a tree any real folder has; they model real ones now, and the new case fails against the old implementation.

Drafted by Claude, reviewed before posting.

…limmer The branch already claimed titles resolve to "an author's title, then the page's first heading, then the cleaned name". That only held for `.gjs.md` and `.gts.md`, whose text the build inlines. A plain `.md` page was never read at build time, so it fell straight through to its filename — the majority of pages, and the disagreement the claim was supposed to have ended. The build now reads every markdown page's headings. Inlining is unchanged: a plain page's text is still fetched on demand rather than shipped in the manifest. Two things follow. `<PageNav />`'s default `:page` block rendered `page.name`, so folder headings showed resolved titles while page links showed filenames. Both are titles now, and the default rendering finally has a test — every app here passes its own block, so nothing covered it. Search scores a page's path. Titles that honor headings make a page unfindable by its filename otherwise: `ember-resources.md` headed `# cell` stops answering to "resources", which the existing search test caught. Also corrects "frontmatter title" in two docs and a comment. kolay reads a sidecar `<page>.json`; it has never parsed frontmatter.

`trimSlashes` had five callsites; three were the same join, each punctuating it differently — one checked `endsWith('/')`, another pre-trimmed the left side. `concatenatePath` does it once, trimming both sides so no caller has to know which one carries the separator. `paths.js` had no tests at all; it has some now. Also adds the worked `<PageNav />` block example the docs never had, including the two public helpers a replacement nav would reach for.

Dogfooding the claim that `handlePotentialIndexVisit` is only needed for `/`: deleted every other call in this repo's nine apps and ran them. custom-root-url failed three tests, all on `/Home`. Nothing is mounted at `/Home` and no page tree sits there — the co-located group's pages live at the root — so the path lookup declines it. `/Home` is a group URL, not a page-tree URL, and only resolving the wildcard as a group name answers it. That was the one thing the hook was still doing that the automatic redirects were not, which is exactly what deleting the calls was meant to find out. The nine calls stay deleted, and `configuring-docs` no longer tells readers to pair the hook with every mount.

@gitKrystan

isRedundantWithHeading compared titles alone, so a page that merely shared its folder's title was dropped from the nav even when the heading linked somewhere else — and the list was the only place that page appeared. A folder titled "Guides" by its meta.json, holding an index page titled "Overview" and a page titled "Guides", lost the latter entirely. Both halves are needed: identity with the index page, and matching titles. Identity alone would hide an index page whose title differs from its folder's, which is worth showing precisely because it says something new. Caught by @gitKrystan in review. The unit fixtures that missed it used an empty `pages: []`, which is not a tree any folder can have — they now model real ones.

Reviewer's call: a bugfix, not a migration step. The one consequence I would have argued to keep is that `group.list[0]` changes for an affected folder, and with it the page a group's own URL resolves to.

main gained frontmatter support (universal-ember#384 and friends) while this branch was in review, and the two overlapped in `setup.js`: both changed it to read every markdown page's source. Resolved to keep both intents, with one correction — headings are now taken from the frontmatter-stripped `content` rather than the raw source, because a YAML comment line starts with `#` and would otherwise be read as a page's first heading. Frontmatter does not title a page: `defaultPopulateManifestEntry` nests it under `meta`, so `title:` in frontmatter is `meta.title` and the page still falls back to its heading or filename. Verified rather than assumed, and documented, since it is the obvious thing to expect. The three conflicts were `setup.js`, `parse.js` (imports), and markdown-only's application test, where both sides appended a module.

gitKrystan mentioned this pull request Aug 14, 2026

feat: a folder's own URL lands on its first page gitKrystan/kolay#1

Closed

gitKrystan changed the title ~~feat: a page tree's own URL lands on its first page~~ feat: a page tree with no page of its own lands on its first page Aug 14, 2026

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch from 32b9bd6 to 55a6d84 Compare August 14, 2026 18:06

gitKrystan commented Aug 14, 2026

View reviewed changes

gitKrystan mentioned this pull request Aug 14, 2026

collections: a group's collection is the groups it collects #366

Draft

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch from 55a6d84 to a9fbc63 Compare August 14, 2026 20:30

gitKrystan commented Aug 14, 2026

View reviewed changes

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch 2 times, most recently from 3a29995 to 213f548 Compare August 14, 2026 22:16

gitKrystan mentioned this pull request Aug 14, 2026

fix: serve group roots in markdown-only #373

Merged

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch from 213f548 to 2841d74 Compare August 14, 2026 22:32

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch 4 times, most recently from d40b9d3 to 7bb3cf6 Compare August 14, 2026 23:09

gitKrystan changed the title ~~feat: a page tree with no page of its own lands on its first page~~ feat (fix?): a page tree with no page of its own lands on its first page Aug 14, 2026

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch 2 times, most recently from d93ea38 to 8740ae3 Compare August 15, 2026 00:04

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch from 8740ae3 to 73ecfe6 Compare August 15, 2026 00:16

gitKrystan commented Aug 15, 2026

View reviewed changes

Comment thread docs-app/tests/docs-app/folder-index-redirect-test.gts

gitKrystan commented Aug 15, 2026

View reviewed changes

Comment thread docs/navigation/handle-potential-index-visit.gjs.md Outdated

gitKrystan commented Aug 15, 2026

View reviewed changes

Comment thread docs/navigation/handle-potential-index-visit.gjs.md Outdated

gitKrystan commented Aug 15, 2026

View reviewed changes

Comment thread src/browser/services/docs.ts Outdated

gitKrystan commented Aug 19, 2026

View reviewed changes

vercel Bot deployed to Preview August 19, 2026 04:36 View deployment

gitKrystan added 4 commits August 18, 2026 21:43

docs: name the nav's index check what it checks, annotate the heading…

77afe2b

… regexes `isFolderOwnPage` describes the mechanism; `isIndex` describes what the caller is asking. The helper is module-scoped, so this does not put the removed public `isIndex` export back.

docs: drop the last two uses of "landing page"

31eacb8

The concept is gone from the public API; the words should go with it.

gitKrystan commented Aug 19, 2026

View reviewed changes

gitKrystan added 5 commits August 18, 2026 22:29

docs: move the index-sorting change to the PR description

e3c3cf6

Reviewer's call: a bugfix, not a migration step. The one consequence I would have argued to keep is that `group.list[0]` changes for an affected folder, and with it the page a group's own URL resolves to.

gitKrystan force-pushed the gitkrystan/folder-index-redirect branch from 632b114 to e3c3cf6 Compare August 19, 2026 19:50

	Cleaned names have digits removed, dashes turned into spaces, and are sentence-cased.
	"Cleaned names" have digits removed, dashes turned into spaces, and are sentence-cased.

	Type guard: `true` for a `PageTree` (a folder), `false` for a `Page`.
	Type guard that returns `true` if the given node is a `PageTree` (a folder of pages) rather than a `Page`.

		for (const node of folder.pages) {
		console.log(isPageTree(node) ? 'folder:' : 'page:', node.title);

		const isFolderOwnPage = (folder: Page \| PageTree, page: Page \| PageTree) =>
		!isPageTree(page) && 'title' in folder && Boolean(folder.title) && page.title === folder.title;

		const path = trimSlashes(entry.appRelativePath, { leading: true });
		const url = `${base.endsWith('/') ? base : `${base}/`}${path}`;


		`getIndexPage(tree)` follows the same change, so it now answers for every folder with pages where it used to answer `undefined`. Two smaller shifts come with it: it can descend into a first child folder, and it matches the page actually named `index` rather than any path ending in `index` — `api-index.md` no longer counts.

		`isIndex` is removed. It asked whether a node was named `index`, which on its own does not answer anything a nav needs. If you were using it to decide what a folder's heading links to, that is `getIndexPage(tree)`; to decide whether listing a page repeats its folder's heading, that is `isRedundantWithHeading(folder, page)`.

Uh oh!

Conversation

gitKrystan commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes along the way

Behavior changes not in the upgrade guide

Manual QE

Uh oh!

bolt-new-by-stackblitz Bot commented Aug 14, 2026

Uh oh!

vercel Bot commented Aug 14, 2026

Uh oh!

gitKrystan left a comment

Choose a reason for hiding this comment

Correctness review of the folder-index redirect

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gitKrystan left a comment

Choose a reason for hiding this comment

Round 2: the routeWillChange move, reviewed on its own

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gitKrystan commented Aug 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gitKrystan Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gitKrystan commented Aug 14, 2026 •

edited

Loading

Round 2: the `routeWillChange` move, reviewed on its own

gitKrystan Aug 19, 2026 •

edited

Loading