API docs: warn when nav is set but header/footer fragments are missing

[PrzemyslawKlys]

Adds a generator warning when using the docs API template with
av configured but without headerHtml/ooterHtml fragments, since this produces API pages with no site header/footer navigation.

[claude]

Code Review: PR #92 - API docs warning for missing header/footer fragments

Summary

This PR adds a helpful generator warning when using the docs API template with NavJsonPath configured but without HeaderHtmlPath/FooterHtmlPath fragments. This prevents confusion when API pages render without site navigation.

---

✅ Strengths

1. Good User Experience: The warning clearly explains the misconfiguration and helps developers understand why navigation might be missing.

2. Appropriate Placement: The validation is placed early in the Generate method (lines 123-131), alongside other configuration warnings, which is consistent with the codebase pattern.

3. Clear Warning Message: The warning message is descriptive and actionable, explaining both the problem and the consequence.

4. Correct Template Check: The code properly checks that the template is "docs" using case-insensitive comparison, which is consistent with how templates are handled elsewhere (see line 2511 where template is "docs" or "sidebar").

5. No Breaking Changes: This is a non-breaking addition that only adds helpful diagnostic information.

---

🔍 Code Quality Observations

Style Consistency: ✅ The code follows the existing patterns in the file:

• Uses string.IsNullOrWhiteSpace() consistently (lines 124-126)
• Follows the same warning pattern as other validations (lines 118-121, 140)
• Uses StringComparison.OrdinalIgnoreCase for template comparison (line 127)

Logic Correctness: ✅ The condition properly identifies the problematic configuration:

• NavJsonPath is set (user wants navigation)
• HeaderHtmlPath AND FooterHtmlPath are both missing (no fragments to render nav)
• Template is "docs" (the template that uses these fragments)

---

💡 Suggestions for Consideration

1. Template Check - Consider "sidebar" Alias

In PowerForge.Web/Services/WebApiDocsGenerator.cs:2511, the code treats "sidebar" as an alias for "docs":

if (template is "docs" or "sidebar")
{
    GenerateDocsHtml(outputPath, options, types);
    return;
}

Question: Should the warning also trigger for template == "sidebar"? If "sidebar" uses the same template system as "docs", it would have the same issue.

Suggested change (if applicable):

template.Equals("docs", StringComparison.OrdinalIgnoreCase) ||
template.Equals("sidebar", StringComparison.OrdinalIgnoreCase)

Or using pattern matching:

var templateLower = template.ToLowerInvariant();
if (templateLower is "docs" or "sidebar")

2. Warning Specificity

The current warning requires BOTH HeaderHtmlPath AND FooterHtmlPath to be missing. Consider if a warning should also appear if only ONE is missing, since that would still result in incomplete navigation. However, this depends on whether partial navigation (header-only or footer-only) is a valid use case.

3. Consider Loading NavJsonPath Earlier (Minor)

The warning checks if NavJsonPath is set, but doesn't validate if the file actually exists. Other warnings in the code check for file existence (e.g., lines 118-121). Consider adding:

if (\!string.IsNullOrWhiteSpace(options.NavJsonPath) &&
    File.Exists(Path.GetFullPath(options.NavJsonPath)) &&
    string.IsNullOrWhiteSpace(options.HeaderHtmlPath) &&
    string.IsNullOrWhiteSpace(options.FooterHtmlPath) &&
    template.Equals("docs", StringComparison.OrdinalIgnoreCase))

This would only warn if the NavJsonPath file actually exists, avoiding noise if the path is set but invalid.

---

🧪 Testing Considerations

Missing Test Coverage: ⚠️ There don't appear to be unit tests for WebApiDocsGenerator in the test suite. While this PR is low-risk (only adds a warning), consider:

1. Adding a test that verifies the warning appears when:

   • NavJsonPath is set
   • HeaderHtmlPath and FooterHtmlPath are null/empty
   • Template is "docs"

2. Adding a test that verifies NO warning appears when:

   • At least one of HeaderHtmlPath or FooterHtmlPath is set
   • Template is not "docs" (e.g., "simple")
   • NavJsonPath is not set

Manual Testing Checklist:

• ✅ Warning appears with the problematic configuration
• ✅ Warning does NOT appear when HeaderHtmlPath or FooterHtmlPath is provided
• ✅ Warning does NOT appear when template is "simple"
• ✅ Warning does NOT appear when NavJsonPath is not set

---

🔒 Security Considerations

✅ No security concerns. This change only adds diagnostic output and doesn't modify any execution paths or handle user input in unsafe ways.

---

⚡ Performance Considerations

✅ No performance impact. The validation adds minimal overhead (a few string comparisons) during the initialization phase of document generation.

---

📋 Overall Assessment

Recommendation: ✅ Approve with minor suggestions

This is a clean, well-implemented quality-of-life improvement that will help developers catch configuration issues early. The code is consistent with existing patterns and adds valuable diagnostic information.

The only suggestion is to verify whether the "sidebar" template alias should also trigger this warning, based on whether it uses the same header/footer fragment system as "docs".

---

📝 Minor Nits

None - the code formatting, style, and structure are all consistent with the existing codebase.

---

Great work on improving the developer experience! 🚀

[Copilot]

Pull request overview

Adds a generator warning in WebApiDocsGenerator.Generate() to help catch misconfiguration when generating API docs with the docs-style template and a nav config, but without providing header/footer HTML fragments (which results in pages lacking site navigation).

Changes:

• Introduces a new warning when NavJsonPath is set, Template is docs, and both HeaderHtmlPath/FooterHtmlPath are unset.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The warning only triggers when Template == "docs", but GenerateHtml treats both "docs" and "sidebar" as docs-style templates. If users set Template to "sidebar", they can still end up with missing header/footer navigation with NavJsonPath set but won’t get this warning. Consider extending the condition to include the "sidebar" template as well (or reuse the same normalization logic used in GenerateHtml).

Suggested change

	template.Equals("docs", StringComparison.OrdinalIgnoreCase))
	(template.Equals("docs", StringComparison.OrdinalIgnoreCase) ||
	template.Equals("sidebar", StringComparison.OrdinalIgnoreCase)))

[Copilot]

This check only looks at whether HeaderHtmlPath/FooterHtmlPath are non-empty strings. If either path is provided but the file doesn’t exist, LoadOptionalHtml will still return an empty fragment and the docs template will render without navigation, but this warning won’t fire. Consider basing the condition on File.Exists for the provided paths (after GetFullPath) or on whether the loaded header/footer content is actually non-empty.

[claude]

Pull Request Review

Summary

This PR adds a helpful validation warning when using the API docs template with navigation configured but without the required header/footer HTML fragments. This is a good quality-of-life improvement that helps users catch a common configuration mistake early.

Code Quality ✅

Strengths:

• Clear, descriptive warning message that explains the problem and the consequence
• Follows existing warning pattern in the codebase (consistent with lines 118-121)
• Well-positioned in the validation section of the Generate method
• Uses appropriate string comparison (StringComparison.OrdinalIgnoreCase) for template name checks
• Defensive null handling with ?? string.Empty and .Trim()

Minor Observations:

• The logic correctly checks for "docs" OR "sidebar" templates, which appears intentional
• The warning only triggers when BOTH HeaderHtmlPath AND FooterHtmlPath are missing (using && operator), which seems correct based on the message

Potential Issues 🔍

None identified. The code is defensive and follows C# best practices:

• No null reference risks
• No resource leaks
• No exception handling issues
• String operations are safe

Performance Considerations ✅

No concerns. The validation logic:

• Runs once during generator initialization
• Performs simple null/whitespace checks and string comparisons
• Has negligible performance impact
• String trimming operation is minimal overhead

Security Concerns ✅

No security issues identified. The code only:

• Reads configuration values (no file I/O in this snippet)
• Performs string comparisons
• Adds a warning message to an internal list

Test Coverage ⚠️

Recommendation: Add a test case to verify this warning is triggered correctly.

Based on the existing test pattern in PowerForge.Tests/WebApiDocsGeneratorCrefTests.cs, consider adding a test like:

[Fact]
public void Generate_WarnsWhenNavSetWithoutHeaderFooter()
{
    var root = Path.Combine(Path.GetTempPath(), "pf-webapidocs-nav-warning-" + Guid.NewGuid().ToString("N"));
    Directory.CreateDirectory(root);
    var xmlPath = Path.Combine(root, "test.xml");
    var outputPath = Path.Combine(root, "out");
    var navPath = Path.Combine(root, "nav.json");

    File.WriteAllText(xmlPath, """
        <doc>
          <assembly><name>Test</name></assembly>
          <members>
            <member name="T:Test.Sample">
              <summary>Sample type.</summary>
            </member>
          </members>
        </doc>
        """);
    File.WriteAllText(navPath, "{}");

    var options = new WebApiDocsOptions
    {
        XmlPath = xmlPath,
        OutputPath = outputPath,
        Template = "docs",
        NavJsonPath = navPath
        // HeaderHtmlPath and FooterHtmlPath intentionally not set
    };

    try
    {
        var result = WebApiDocsGenerator.Generate(options);
        Assert.Contains(result.Warnings, w => 
            w.Contains("NavJsonPath") && 
            w.Contains("HeaderHtmlPath/FooterHtmlPath"));
    }
    finally
    {
        if (Directory.Exists(root))
            Directory.Delete(root, true);
    }
}

This would verify:

1. The warning is produced when NavJsonPath is set without header/footer
2. The warning message contains the expected keywords
3. The check works for the "docs" template

You could add a similar test for the "sidebar" template or use [Theory] with [InlineData] to test both templates.

Overall Assessment ✅

Recommendation: Approve with optional test addition

This is a clean, well-written change that improves the user experience by providing actionable feedback. The code quality is solid and consistent with the existing codebase standards outlined in CONTRIBUTING.md.

The only enhancement would be adding test coverage to ensure this warning behavior is maintained in future refactorings.