elastic / elastic/docs-builder

Navigation 1st principles hardening

Open
#3,870 0 comments 0 reactions 0 assignees View on GitHub
needs triage
Dominant language
C#
Stars
24
Forks
44
Avg merge
1d 7h
Merged PRs (30d)
146

Description

The navigation core is genuinely principled — `INavigationItem` / `ILeafNavigationItem` / `INodeNavigationItem` / `IRootNavigationItem` form a clean recursive tree, `INavigationHomeProvider` buys O(1) re-homing with lazily-computed URLs, and `NavigationRenderModel` resolves the tree once into a dumb render model the templates consume without logic. The periphery is where it frays, in four recurring patterns. None of this is in scope for the current branch — tracking here so the investigation survives.

## 1. Render mode encoded as three orthogonal booleans

`NavigationRenderModel.Create` takes `isUsingNavigationDropdown`, `isPrimaryNavEnabled`, `isGlobalAssemblyBuild` — eight combinations for three real modes. Each conditional inside (`CreateRootIndex`, `CreateBackLinks`) re-derives the mode from a different pair. One mode enum would make any production-affecting flag flip obvious rather than a one-token diff.

Related: `IsUsingNavigationDropdown` is *both* an `INavigationItem` property and a `Create` parameter, and `GlobalNavigationHtmlWriter` previously hardcoded a literal that contradicted the root's own value.

## 2. ~15 hand-rolled recursive walkers

The same tree is walked by ~15 independent recursive visitors, each re-deriving its own leaf/node/hidden/crosslink policy, despite `INavigationTraversable` existing. There is already a behavioural divergence:

- `SitemapBuilder.cs:89-100` filters on `Hidden` where the rest of the pipeline uses `ExcludeFromIndexing`, so listing pages that are deliberately "hidden but indexed" are silently missing from the sitemap.
- `CodexNavigation.cs:48-54` documents why a second root over shared nodes cannot reuse `UpdateNavigationIndex` at all — mutable `NavigationIndex` on a shared node is the leak.

## 3. `docs-content` privileged in code rather than configuration

`docs-content` is special-cased in at least five places:

- `SiteNavigation.cs:59-71` — root-leaf hoisting
- `SiteNavigation.cs:224-240` — `path_prefix`-required exemption (error path invents a `bad-mapping-…` URL segment)
- `SiteNavigationFile.cs` — URI scheme default
- `AssembleSources.cs` — implicit special casing
- `HtmlWriter.cs` — indexing rule

Additionally, `SiteNavigationFile.cs:158-257` and `:259-332` are ~70 lines of copy-paste YAML converters that share the `docs-content://` implicit scheme.

## 4. Mutation-after-construction as the extension mechanism

`IAssignableChildrenNavigation`, `IAssignableIslandNavigation`, `SectionNavigation.Url`'s `internal set`, `Index = null!` / `Id = null!` sentinels, public `Parent` and `NavigationIndex` setters — each forces a defensive check downstream.

`SiteNavigation` implements `IAssignableChildrenNavigation` and then throws from `SetNavigationItems` (`:184-185`) — the interface contract is a lie for one implementation.

---

## Feature-specific follow-ups (from the top-nav branch)

- [ ] **Aliased section URLs.** `SectionNavigation.Url` = first child's URL (`SiteNavigation.cs:96-99`) is the root cause of both the `GetParents()` URL-dedupe drop and the "← Guides goes to get-started" limitation. A synthetic section landing page gives the node its own URL and removes both special cases.
- [ ] **Sections matched by display title.** `SectionTopNavBuilder.cs:34-41` keys plain tocs by `Identifier` but sections by `Title` with `OrdinalIgnoreCase` — renaming a section in YAML silently drops its tab. Match on identity (`section://…`).
- [ ] **Dead code.** `TopNavDropdownItem` and `TopNavLinkItem.SectionIds` are never constructed anywhere in `src/` — only in tests. The dropdown branch at `_SecondaryNav.cshtml`, `secondary-nav.ts`, and `secondary-nav-dropdown.css` are unreachable in real builds. Either wire a `dropdown:` concept into the schema or delete all of it.
- [ ] **`SiteNavigation.TopLevelItems` is marked `//TODO Obsolete?`** yet is an input to `SectionTopNavBuilder` and `GlobalNavigationHtmlWriter`. Resolve before adding a third consumer.
- [ ] **String-namespace type test.** `LlmsNavigationEnhancer.cs:96` dispatches on `item.GetType().FullName?.StartsWith("Elastic.ApiExplorer.")` to avoid an assembly reference — reflection-shaped dispatch in an AOT codebase. A marker interface on `INavigationItem` removes it.

---

## Localized messes worth their own cleanups

- `GlobalNavigationPathProvider.OutputFile` (`:59-111`) — a dozen hardcoded repo/path prefixes under a `//TODO clean up`
- `ConfigurationFileProvider.CreateNavigationFile` (`:140-212`) — regex line-surgery on navigation.yml plus an inlined hardcoded `docs-builder://` block

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.