From ac9e242b053101cb36cdfa576393a3c7dd4f87d4 Mon Sep 17 00:00:00 2001 From: Jan Calanog Date: Thu, 8 Jan 2026 10:40:46 +0100 Subject: [PATCH] Fix navigation dropdowns --- .../Isolated/Node/FolderNavigation.cs | 5 +- .../Node/TableOfContentsNavigation.cs | 6 +- .../Isolated/Node/VirtualFileNavigation.cs | 3 +- .../Assembler/SiteNavigationTests.cs | 97 +++++++++++++++++++ .../Isolation/NavigationStructureTests.cs | 1 + 5 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/Elastic.Documentation.Navigation/Isolated/Node/FolderNavigation.cs b/src/Elastic.Documentation.Navigation/Isolated/Node/FolderNavigation.cs index 13f2b18e7..f2a9991fd 100644 --- a/src/Elastic.Documentation.Navigation/Isolated/Node/FolderNavigation.cs +++ b/src/Elastic.Documentation.Navigation/Isolated/Node/FolderNavigation.cs @@ -38,7 +38,7 @@ public class FolderNavigation( public int NavigationIndex { get; set; } /// - public string Id { get; } = ShortId.Create(parentPath); + public string Id { get; private set; } = null!; /// public ILeafNavigationItem Index { get; private set; } = null!; @@ -50,6 +50,9 @@ internal void SetNavigationItems(IReadOnlyCollection navigation { var indexNavigation = navigationItems.QueryIndex(this, $"{FolderPath}/index.md", out navigationItems); Index = indexNavigation; + // Include NavigationRoot.Id to ensure uniqueness across docsets in assembler builds + // (URLs alone aren't unique until path prefixes are applied by SiteNavigation) + Id = ShortId.Create(NavigationRoot.Id, Index.Url); NavigationItems = navigationItems; } } diff --git a/src/Elastic.Documentation.Navigation/Isolated/Node/TableOfContentsNavigation.cs b/src/Elastic.Documentation.Navigation/Isolated/Node/TableOfContentsNavigation.cs index e81138546..061368984 100644 --- a/src/Elastic.Documentation.Navigation/Isolated/Node/TableOfContentsNavigation.cs +++ b/src/Elastic.Documentation.Navigation/Isolated/Node/TableOfContentsNavigation.cs @@ -28,7 +28,8 @@ INavigationHomeProvider homeProvider Parent = parent; Hidden = false; IsUsingNavigationDropdown = false; - Id = ShortId.Create(parentPath); + // Id will be set in SetNavigationItems using Index.Url for uniqueness + Id = null!; ParentPath = parentPath; PathPrefix = pathPrefix; @@ -103,7 +104,8 @@ internal void SetNavigationItems(IReadOnlyCollection navigation { var indexNavigation = navigationItems.QueryIndex(this, $"{ParentPath}/index.md", out navigationItems); Index = indexNavigation; - Id = ShortId.Create(indexNavigation.Url); + // Include NavigationRoot.Id to ensure uniqueness across docsets in assembler builds + Id = ShortId.Create(NavigationRoot.Id, Index.Url); NavigationItems = navigationItems; } } diff --git a/src/Elastic.Documentation.Navigation/Isolated/Node/VirtualFileNavigation.cs b/src/Elastic.Documentation.Navigation/Isolated/Node/VirtualFileNavigation.cs index 5e5e17904..43c75dc0a 100644 --- a/src/Elastic.Documentation.Navigation/Isolated/Node/VirtualFileNavigation.cs +++ b/src/Elastic.Documentation.Navigation/Isolated/Node/VirtualFileNavigation.cs @@ -34,7 +34,8 @@ public class VirtualFileNavigation(TModel model, IFileInfo fileInfo, Vir public int NavigationIndex { get; set; } /// - public string Id { get; } = ShortId.Create(args.RelativePathToDocumentationSet); + // Include NavigationRoot.Id to ensure uniqueness across docsets in assembler builds + public string Id => ShortId.Create(NavigationRoot.Id, Index.Url); /// public ILeafNavigationItem Index { get; } = diff --git a/tests/Navigation.Tests/Assembler/SiteNavigationTests.cs b/tests/Navigation.Tests/Assembler/SiteNavigationTests.cs index ca2fa2795..9a0727a4d 100644 --- a/tests/Navigation.Tests/Assembler/SiteNavigationTests.cs +++ b/tests/Navigation.Tests/Assembler/SiteNavigationTests.cs @@ -166,6 +166,103 @@ public void SitePrefixAppliedToNavigationItemUrls(string? sitePrefix, string exp observabilityItem.Url.Should().Be(expectedObservabilityUrl, $"sitePrefix '{sitePrefix}' should result in URL '{expectedObservabilityUrl}'"); } + [Fact] + public void NavigationNodeIdsAreUniqueAcrossDocsets() + { + // This test verifies that navigation node IDs are unique even when + // multiple docsets have folders with the same relative path. + // This is critical because IDs are used as HTML id attributes. + + // Create two docsets, each with a "getting-started" folder at the same relative path + var fileSystem = new MockFileSystem(); + + // Docset 1: product-a with getting-started folder + var productADir = "/checkouts/current/product-a"; + // language=yaml + var productADocset = """ + project: product-a + toc: + - file: index.md + - folder: getting-started + children: + - file: index.md + - file: quick-start.md + """; + fileSystem.AddFile($"{productADir}/docs/docset.yml", new MockFileData(productADocset)); + fileSystem.AddFile($"{productADir}/docs/index.md", new MockFileData("# Product A")); + fileSystem.AddFile($"{productADir}/docs/getting-started/index.md", new MockFileData("# Getting Started A")); + fileSystem.AddFile($"{productADir}/docs/getting-started/quick-start.md", new MockFileData("# Quick Start A")); + + // Docset 2: product-b with getting-started folder (same relative path!) + var productBDir = "/checkouts/current/product-b"; + // language=yaml + var productBDocset = """ + project: product-b + toc: + - file: index.md + - folder: getting-started + children: + - file: index.md + - file: tutorial.md + """; + fileSystem.AddFile($"{productBDir}/docs/docset.yml", new MockFileData(productBDocset)); + fileSystem.AddFile($"{productBDir}/docs/index.md", new MockFileData("# Product B")); + fileSystem.AddFile($"{productBDir}/docs/getting-started/index.md", new MockFileData("# Getting Started B")); + fileSystem.AddFile($"{productBDir}/docs/getting-started/tutorial.md", new MockFileData("# Tutorial B")); + + // Create navigation for both docsets + var productAContext = SiteNavigationTestFixture.CreateContext(fileSystem, productADir, output); + var productADocsetFile = DocumentationSetFile.LoadAndResolve(productAContext.Collector, fileSystem.FileInfo.New($"{productADir}/docs/docset.yml"), fileSystem); + var productANav = new DocumentationSetNavigation(productADocsetFile, productAContext, GenericDocumentationFileFactory.Instance); + + var productBContext = SiteNavigationTestFixture.CreateContext(fileSystem, productBDir, output); + var productBDocsetFile = DocumentationSetFile.LoadAndResolve(productBContext.Collector, fileSystem.FileInfo.New($"{productBDir}/docs/docset.yml"), fileSystem); + var productBNav = new DocumentationSetNavigation(productBDocsetFile, productBContext, GenericDocumentationFileFactory.Instance); + + // Get the "getting-started" folders from each docset + var productAGettingStarted = productANav.NavigationItems.First() + .Should().BeOfType>().Subject; + var productBGettingStarted = productBNav.NavigationItems.First() + .Should().BeOfType>().Subject; + + // Both folders have the same relative path within their docsets + productAGettingStarted.FolderPath.Should().Be("getting-started"); + productBGettingStarted.FolderPath.Should().Be("getting-started"); + + // But they MUST have different IDs because they resolve to different URLs + // Product A: /product-a/getting-started + // Product B: /product-b/getting-started + productAGettingStarted.Id.Should().NotBe(productBGettingStarted.Id, + "folders with the same relative path but in different docsets should have different IDs " + + "because they have different URLs"); + + // Also verify in assembled navigation context + // language=yaml + var siteNavYaml = """ + toc: + - toc: product-a:// + path_prefix: /product-a + - toc: product-b:// + path_prefix: /product-b + """; + var siteNavFile = SiteNavigationFile.Deserialize(siteNavYaml); + var documentationSets = new List { productANav, productBNav }; + var siteContext = SiteNavigationTestFixture.CreateContext(fileSystem, productADir, output); + var siteNavigation = new SiteNavigation(siteNavFile, siteContext, documentationSets, sitePrefix: null); + + // Use production YieldAll() to collect all navigation items + var allNodeItems = ((INavigationTraversable)siteNavigation).YieldAll() + .OfType>() + .ToList(); + + // Verify all IDs are unique + var allIds = allNodeItems.Select(n => n.Id).ToList(); + var uniqueIds = allIds.Distinct().ToList(); + uniqueIds.Should().HaveCount(allIds.Count, + $"all navigation node IDs should be unique in assembled navigation. " + + $"Found duplicates: {string.Join(", ", allIds.GroupBy(id => id).Where(g => g.Count() > 1).Select(g => $"ID '{g.Key}' appears {g.Count()} times"))}"); + } + [Theory] [InlineData(null, "/observability", "/search")] [InlineData("docs", "/docs/observability", "/docs/search")] diff --git a/tests/Navigation.Tests/Isolation/NavigationStructureTests.cs b/tests/Navigation.Tests/Isolation/NavigationStructureTests.cs index 2d0c68488..3ebcb80bf 100644 --- a/tests/Navigation.Tests/Isolation/NavigationStructureTests.cs +++ b/tests/Navigation.Tests/Isolation/NavigationStructureTests.cs @@ -362,4 +362,5 @@ void VisitNavigationItems(INavigationItem item) context.Diagnostics.Should().BeEmpty(); } + }