Nested, collapsible sidebar navigation (NavPage groups + native <details>)

[adnaan]

What

A nav section's pages can now themselves be groups, so a long section renders as collapsible category sub-groups instead of one flat list. Sections and groups are native <details>/<summary>, so collapse needs zero JS and the disclosure chevron comes free from PicoCSS.

Motivated by the docs site's "UI Patterns" section, which lists 33 pattern pages as a single flat sidebar list. With this, they nest under their 7 categories.

Changes

• config (internal/config/config.go): NavPage gains Pages (recursive) + Collapsed. Presence of Pages makes an entry a group. Backward compatible — existing flat configs leave Pages empty.
• site (internal/site/manager.go): discoverFromConfig recurses via buildNavPageNode, registering every nested leaf in m.pages (the registration invariant — a missed leaf 303s to home in site mode). PageNode gains Collapsed + ContainsPath; the search-index section lookup now descends the whole subtree.
• server (internal/server/server.go): renderSidebar emits sections + groups as <details> honoring collapsed and auto-opening the branch that holds the active page. Pico's <details> margins/colors are neutralized with id-scoped overrides; top-level summaries keep class="nav-section-title", groups use nav-group-title.

Tests

internal/server/nested_nav_test.go: nested leaves serve 200, group/section structure + classes, and collapse/auto-open state. Full CI suite (go test -tags=ci -skip='E2E|e2e' ./...) green.

Verification

Built the docs site against this branch and verified in a real browser (chromedp): UI Patterns → 7 collapsible category groups, active category auto-opens, all 33 leaves serve 200, top-level IA section count unchanged.

🤖 Generated with Claude Code

[claude]

Code Review

Overall this is a clean, well-scoped implementation. The zero-JS <details>/<summary> approach is the right call, backward compatibility is preserved, and the three-invariant test strategy (registration, structure, collapse state) is exactly the right coverage to lock in. A few issues to address before merging:

---

Bug: empty-path leaf node renders as a dead link

buildNavPageNode in internal/site/manager.go returns a node with no Path and no Children when a NavPage has neither path nor pages set (e.g. a placeholder entry). In writeNavChildren, the group/leaf branch is chosen by len(node.Children) > 0 — so a no-path, no-children node falls through to the leaf branch and emits:

<li><a href="" class="nav-page-link">Placeholder</a></li>

A click on that link navigates to the site root, which is a silent bug. Suggest validating at Discover time or skipping the node in writeNavChildren:

// in writeNavChildren, leaf branch:
if node.Path == "" {
    continue // pure group with no children — skip, invalid config
}

Or return an error from buildNavPageNode when both page.Path and page.Pages are empty.

---

HTML encoding of titles and paths

node.Title and node.Path are interpolated directly into HTML strings in writeNavChildren (and section.Title in renderSidebar). This is consistent with the existing pre-PR code, but the new code extends it to group titles. The test fixture already surfaces this: "Forms & Editing" produces technically invalid HTML (bare &). It renders fine in all browsers, but titles with <, >, or " would break the markup or create an injection vector for anyone with config write access.

Worth fixing globally with html.EscapeString — a one-liner at each Sprintf site:

b.WriteString(fmt.Sprintf(`<summary class="nav-group-title">%s</summary>`, html.EscapeString(node.Title)))
// and for hrefs:
b.WriteString(fmt.Sprintf(`<li><a href="%s" class="nav-page-link%s">%s</a></li>`,
    node.Path, activeClass, html.EscapeString(node.Title)))

(Import html from the standard library.)

---

Minor: GenerateSearchIndex is now O(pages × nav_tree_size)

The old lookup was O(pages × flat_children). The new navNode.ContainsPath(page.Path) call is a DFS over the whole subtree, run once per page per top-level section in internal/site/manager.go:GenerateSearchIndex. For a docs site with 33 leaf pages and 7 groups this is negligible, but worth a note in case the site grows. No action needed now.

---

Test coverage gaps (non-blocking)

Three cases that are easy to add if you want belt-and-suspenders coverage:

1. A group with both Path and Pages (landing-page-for-a-group): verify the landing page itself is served and the group's children are served.
2. A 3-level nesting (section → group → subgroup → leaf): the recursion handles it, but a test would catch a future refactor that hardcodes depth.
3. GenerateSearchIndex resolves nested pages to their top-level section title.

None of these are blocking, just suggestions.

---

Nits

• TestNestedCollapseState calls renderNestedSidebar twice, spinning up two full servers. Could reuse the first server for the /home request — but readability > micro-efficiency here, so fine as-is.
• The CSS comment block in renderPage (/* Sidebar sections and nested category groups... */) is unusually long for inline CSS. Worth trimming to the key caveat (Pico override) rather than re-explaining what <details> does.

---

Summary: Two things worth fixing before merge — the dead-link edge case for nodes with no path/pages, and the missing html.EscapeString calls. Everything else is solid.

[claude]

Code Review — Nested, collapsible sidebar navigation

Nice, well-scoped feature. The zero-JS approach with native <details>/<summary> is the right call, the backward-compat story is clean, and the recursive registration invariant is clearly articulated in both code and tests. A few things worth addressing before merge:

---

Issues

1. node.Path is not HTML-escaped in the href attribute (minor security)

In server/server.go → writeNavChildren:

b.WriteString(fmt.Sprintf(`<li><a href="%s" class="nav-page-link%s">%s</a></li>`,
    node.Path, activeClass, escapeHTML(node.Title)))

node.Path is placed directly into a double-quoted HTML attribute without escaping. Paths come from config files so this isn't exploitable by end users, but a path containing " would break the attribute, and the inconsistency is a footgun if paths ever flow from a less-controlled source. Fix:

b.WriteString(fmt.Sprintf(`<li><a href="%s" class="nav-page-link%s">%s</a></li>`,
    escapeHTML(node.Path), activeClass, escapeHTML(node.Title)))

2. Missing test: empty NavPage (no Path, no Pages)

writeNavChildren silently skips nodes where node.Path == "" and len(node.Children) == 0, which is the right behavior. But buildNavPageNode will happily construct and return that empty node without error, and a user who writes a malformed nav entry in their config gets no warning — the page just disappears from the sidebar. Consider either:

• returning an error from buildNavPageNode when both page.Path and page.Pages are empty, or
• at minimum, adding a test that documents the current behavior so a future reader knows the skip is intentional.

---

Observations / suggestions (non-blocking)

3. Whitespace-only alignment changes in config.go add diff noise

The first ~30 lines of the config.go diff are pure struct tag alignment (adding/removing spaces). These are harmless but make the meaningful changes harder to spot during review. Consider keeping cosmetic reformatting out of feature PRs.

4. No recursion depth limit in buildNavPageNode / ContainsPath

In practice a nav tree will never be deep enough to blow the stack, but a deeply pathological or accidentally self-referential YAML config would cause a crash with no useful error message. A depth guard (e.g. max 10 levels) with a clear error would make this more robust. Low priority.

5. list-style: none on <summary> vs. summary::marker

#tinkerdown-sidebar .nav-section-title,
#tinkerdown-sidebar .nav-group-title {
    cursor: pointer;
    list-style: none;  /* hides the default bullet but not the disclosure triangle */
}

list-style: none suppresses the list-item bullet but not the native <details> disclosure triangle. To remove the triangle (if undesired) you also need summary::marker { display: none } or ::-webkit-details-marker { display: none }. The PR description says the Pico chevron is intentional ("chevron comes free from Pico's summary::after"), so this may be deliberate — but worth a comment in the CSS so a future editor doesn't add list-style: none thinking it controls the triangle.

---

What's done well

• Registration invariant is clearly explained in comments and locked by TestNestedLeavesAreServed — exactly the right thing to test for a recursive tree builder.
• escapeHTML package-level helper is a good solution to the html.Builder vs. html package shadowing problem, and the comment explaining why it's package-level is appreciated.
• ContainsPath on PageNode cleanly fixes the search-index section lookup that previously only searched one level deep.
• Test coverage hits the three real failure modes: registration, structure/CSS, and collapse state. TestGroupWithLandingPage for the mixed leaf+group case is a nice edge-case addition.
• Fully backward compatible — existing flat configs require no changes.

---

Summary: The main actionable item is escaping node.Path in the href attribute (#1). Everything else is low-priority polish. Happy to approve once that's addressed.

[claude]

Code Review

Overall this is a well-structured feature. The recursive design is clean, HTML escaping is handled correctly throughout, the backward-compatibility story is solid, and the test suite covers the key invariants. A few things to flag:

---

Bug: group landing page is sidebar-invisible

A NavPage with both Path and Pages (the landing-page-as-group case) is registered in m.pages and served correctly — TestGroupWithLandingPage confirms this — but writeNavChildren branches on len(node.Children) > 0 first and renders only the <details>/<summary> group shell. The node's own Path never gets an <a> tag, so:

1. There is no clickable link to the landing page anywhere in the sidebar.
2. No active class is applied when the user visits the landing page (the node.Path == currentPath check is only reached for leaf nodes).
3. ContainsPath does open the group correctly, but with nothing highlighted inside it — the active state appears to vanish.

Minimal fix — in writeNavChildren, after rendering the <summary>, check if the group node itself has a path and emit a link before recursing:

b.WriteString(fmt.Sprintf(`<summary class="nav-group-title">%s</summary>`, escapeHTML(node.Title)))
if node.Path != "" {
    activeClass := ""
    if node.Path == currentPath {
        activeClass = " active"
    }
    b.WriteString(fmt.Sprintf(`<li><a href="%s" class="nav-page-link%s">%s</a></li>`, escapeHTML(node.Path), activeClass, escapeHTML(node.Title)))
}
writeNavChildren(b, node.Children, currentPath)

TestGroupWithLandingPage should also assert that a link to /group/landing appears in the rendered sidebar HTML, not just that the page returns 200.

---

list-style: none on <summary> is a no-op

#tinkerdown-sidebar .nav-section-title,
#tinkerdown-sidebar .nav-group-title {
    list-style: none;  /* does not affect the disclosure triangle */
}

list-style: none removes list-item bullets; it has no effect on the <details> disclosure triangle. The code comment already acknowledges that Pico suppresses the native triangle and substitutes its own summary::after chevron — so the rule is harmless but dead weight. Worth removing or replacing with a comment that explains why nothing extra is needed here.

---

Empty <ul> when all children are groups

If a section contains only group nodes (no direct leaf pages), writeNavChildren still emits <ul class="nav-pages">…</ul>. Harmless, but it adds an empty element to the DOM. Easy fix: write the <ul> open tag only when there is at least one renderable child, or defer the write to the first item.

---

No recursion depth guard

buildNavPageNode and ContainsPath recurse without a depth limit. For the intended use case (sidebar nav, 2–3 levels) this is fine. If you ever expose config to untrusted input (e.g. a user-uploaded tinkerdown.yaml), add a depth parameter or a visited-set to prevent unbounded recursion.

---

Nits

• The writeNavChildren call site in renderSidebar wraps section.Children — worth a comment that the section node itself is never a renderable leaf (its Path is "/" + section.Path which may be "/" for sections without a path), so the existing leaf-link path in writeNavChildren won't accidentally render it.
• GenerateSearchIndex is now O(pages × nav_tree_depth) rather than O(pages × flat_children). Still negligible at any realistic nav size, but worth noting if the search index is ever benchmarked.

---

Summary: the landing-page-as-group sidebar rendering is the only functional issue. Everything else is non-blocking. The registration invariant, HTML escaping, collapse/auto-open logic, and search-index fix are all correct and well-tested.

[claude]

Code Review: Nested, Collapsible Sidebar Navigation

Overall this is a solid, well-scoped feature. The zero-JS <details>/<summary> approach is the right call, the backward-compat story is clean, and the test coverage is genuinely thorough for what it touches. A few things worth addressing before merge:

---

Bugs / Correctness

renderBreadcrumbs still has unescaped titles (line ~3707)

html.WriteString(fmt.Sprintf(`<li><a href="%s">%s</a></li>`, crumb.Path, crumb.Title))

The PR correctly adds escapeHTML in the new nav helpers, but renderBreadcrumbs — which lives in the same file and has the same variable-shadowing problem — still emits unescaped crumb titles and paths. Not introduced by this PR, but now that the pattern is established it's worth fixing in the same pass.

escapeHTML on hrefs conflates two different escaping needs

b.WriteString(fmt.Sprintf(`<li><a href="%s" ...>%s</a></li>`, escapeHTML(node.Path), ...))

html.EscapeString is correct for text content, but URL paths should use url.PathEscape (or at minimum just not HTML-escape them). A path containing & would become & in the href, producing a broken link. In practice, nav paths from YAML shouldn't contain those characters — but a comment noting the assumption would be safer than silently relying on it. Alternatively, apply url.PathEscape to the path component and html.EscapeString only to title text.

---

Code Smell

escapeHTML wrapper is a symptom, not a fix

// Defined at package scope (not inside renderSidebar, where the local `html`
// strings.Builder shadows the stdlib html package).
func escapeHTML(s string) string { return html.EscapeString(s) }

The root cause is that renderSidebar, renderBreadcrumbs, and other functions all declare var html strings.Builder, shadowing the html package import throughout a 3700-line file. The wrapper is a reasonable short-term workaround, but if you're touching this area anyway, renaming those local variables to buf or sb would be cleaner and remove the need for the package-level alias. Not blocking, but worth a follow-up.

---

CSS Nits

nav-group-item display: list-item is redundant

#tinkerdown-sidebar .nav-group-item {
    display: list-item;  /* <li> is already display:list-item */
    margin: 0;
    padding: 0;
}

The <li> already has display: list-item by default. Only margin: 0; padding: 0; are doing work here.

Hardcoded indent values could drift
3rem, 2.5rem, 3.75rem for section/group/leaf indents are correct now, but if the base nav link padding ever changes, all three need updating together. CSS custom properties (or at least a comment noting the relationship) would help.

---

Test Coverage — What's Missing

The existing tests are good. Two gaps worth noting:

1. Duplicate path in multiple groups: what happens if the same .md path appears in two groups? Currently m.pages[node.Path] = node would silently overwrite the first registration. A test (and a fmt.Errorf in buildNavPageNode) would lock this invariant.

2. Deeply-collapsed section with active page at depth 3+: TestNestedCollapseState tests depth-2. The ContainsPath recursion is correct, but a depth-3 variant in manager_test.go would give confidence without much extra code.

---

Minor Positives Worth Calling Out

• The registration invariant comment and test (TestNestedLeavesAreServed) are exactly right — this is the silent failure mode that matters most.
• ContainsPath being on PageNode (not inlined in the render loop) is the correct factoring.
• The buildNavPageNode fail-fast on entries with neither path nor pages is good defensive validation.
• Section titles are now HTML-escaped (escapeHTML(section.Title)) where the old code left them raw — that's a quiet bug fix bundled in.

🤖 Generated with Claude Code