-
Notifications
You must be signed in to change notification settings - Fork 32
Navigation Refactor: Generic Type System, O(1) Re-homing, and improved assembly routines #1995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Introduced draft documentation for all CLI commands under `docs/cli`. - Updated `_docset.yml` to include new CLI documentation. - Adjusted navigation order and reinstated missing migration files.
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
… url is dynamic based on parent toc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. (We did a highlevel review in a sync session.)
Just want to mention again that we want to have non-trailing slashes. (It's a bit hard to find the relevant code part)
P.S. Just adding Copilot as reviewer to see it creates some suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a significant refactoring of the navigation system, transitioning from a flat navigation model to an isolated, hierarchical navigation architecture. The changes introduce new navigation abstractions, improve URL resolution with trailing slashes, and add comprehensive test coverage for navigation building and validation.
Key changes:
- Introduced new isolated navigation architecture with covariant interfaces
- Added trailing slashes to generated URLs for consistency
- Implemented new test projects for navigation and configuration
- Refactored assembler navigation to use the new
SiteNavigationmodel - Updated cross-link and URI resolution logic
Reviewed Changes
Copilot reviewed 163 out of 171 changed files in this pull request and generated 92 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/authoring/Inline/RelativeLinks.fs | Updated expected HTML outputs to include trailing slashes in URLs |
| tests/authoring/Generator/LinkReferenceFile.fs | Reordered link entries in JSON structure |
| tests/authoring/Framework/TestValues.fs | Updated MinimalParseAsync call to accept document lookup function |
| tests/authoring/Framework/Setup.fs | Added exclusion pattern filtering for files starting with underscore |
| tests/authoring/Framework/CrossLinkResolverAssertions.fs | Added AssemblerBuild property to mock context |
| tests/Navigation.Tests/*.cs | New test files for navigation validation, structure, and assembler integration |
| tests/Elastic.Markdown.Tests/*.cs | Updated diagnostic collector implementation and file system extensions |
| tests/Directory.Build.props | Removed trailing blank line |
| tests-integration/Elastic.Assembler.IntegrationTests/*.cs | Added new integration tests and refactored existing navigation tests |
| src/tooling/docs-builder/Http/DocumentationWebHost.cs | Updated to use new navigation model and FilePath-based lookup |
| src/services/Elastic.Documentation.Assembler/Navigation/*.cs | Refactored to use SiteNavigation and updated URI resolution logic |
Comments suppressed due to low confidence (2)
src/services/Elastic.Documentation.Assembler/Navigation/LlmsNavigationEnhancer.cs:55
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
src/Elastic.Markdown/Myst/Directives/Image/ImageBlock.cs:125 - These 'if' statements can be combined.
This PR fundamentally rewrites the navigation system to solve long-standing maintainability issues and enable future extensibility.
The Problem
The original navigation implementation had four critical issues:
INavigationItem<INavigationModel, INavigationItem>, forcing runtime type checks and casts throughout the codebase to access model-specific propertiesMarkdownFileitself, creating tight coupling between navigation logic and a single model implementation tied to HTML renderingThese issues blocked API documentation integration, made the assembler process opaque, and created a brittle system where navigation concerns leaked into model classes.
The Solution
1. Generic Type System with Covariance
Before:
After:
Made all navigation classes generic over
TModel where TModel : IDocumentationFile. Enables:2. Navigation as Single Source of Truth
Before: URLs, paths, and navigation properties duplicated on
MarkdownFile:After: Navigation owns all navigational concerns:
Why this matters:
3. Dedicated Navigation Assembly
Created
Elastic.Documentation.Navigationas standalone assembly:IDocumentationFileimplementation4. Two-Phase Loading (Configuration → Navigation)
Phase 1: Configuration resolution - Parse YAML, resolve paths, validate files
Phase 2: Navigation construction - Build tree, calculate URLs, set relationships
Why this matters:
5. Home Provider Architecture: O(1) Re-homing
The assembler must combine repositories with custom URL prefixes. Naive approach requires O(n) tree traversal.
Our solution: Provider pattern with indirection.
Time complexity: O(1) - Single reference assignment, regardless of subtree size. URLs lazy-calculated on-demand from provider. This is what makes the assembler practical at scale.
What Changed
Core Navigation (
src/Elastic.Documentation.Navigation/)DocumentationSetNavigation<TModel>Configuration (
src/Elastic.Documentation.Configuration/)Assembler (
src/Elastic.Documentation.Assembler/)AssemblerBuildflag controls scope creationModels (Breaking Change)
Documentation (
docs/development/navigation/)10 comprehensive documents with progressive learning path:
Why This Was Necessary
Enabling Future Work
This refactor was extensive because we needed to:
What This Enables
ApiDocFilemodelBackward Compatibility
All 111 tests pass. Breaking change: Models no longer have navigation properties - access through navigation tree instead.
Testing