web: make theme slots real (portable contract)

[PrzemyslawKlys]

This makes ThemeManifest.Slots actually work during partial resolution, so themes can expose stable slot names (e.g. site-header/site-footer) while mapping to concrete partials.

Also updates the scaffolder to emit theme.manifest.json (schemaVersion=2) with default slots and updates the sample docs.

Build: dotnet build PSPublishModule.sln -c Release
Tests: dotnet test PowerForge.Tests -c Release

[claude]

Pull Request Review: Theme Slots Implementation

Overview

This PR implements theme slot mapping functionality, enabling portable theme contracts where layouts can reference stable slot names (e.g., site-header, site-footer) that map to concrete partials. This is a valuable feature for theme portability and maintainability.

---

Code Quality & Best Practices ✅

Positives:

1. Well-documented code: The slot resolution logic includes clear comments explaining the purpose and behavior (ThemeLoader.cs:55-57)
2. Follows existing patterns: The implementation mirrors the existing Partials and Layouts resolution patterns, maintaining consistency
3. Clean separation: Changes are properly isolated to the theme loading/scaffolding layer
4. Backward compatibility: Maintains support for legacy theme.json files while preferring theme.manifest.json

Suggestions:

1. Magic number: The slot resolution loop uses a hardcoded limit of 8 iterations (ThemeLoader.cs:62). Consider extracting this to a named constant:

   private const int MaxSlotResolutionDepth = 8;

   This would improve readability and make the limit easier to adjust if needed.

2. Documentation: Consider adding XML documentation to explain the slot resolution depth limit and circular reference prevention strategy.

---

Potential Bugs or Issues 🔍

Minor Issues:

1. Infinite loop protection: The code uses both a counter (max 8 iterations) AND a seen HashSet for circular reference detection. While this is defensive, the logic could be slightly clearer:

   • The loop breaks if slotMapped is null/empty (line 64-65)
   • The loop breaks if we've seen this value before (line 66-67)
   • The loop stops at 8 iterations (line 62)

   Recommendation: Consider adding a comment explaining this multi-layered protection strategy, or simplify to rely primarily on the seen HashSet.

2. Slot resolution happens before partial mapping: The slot is resolved first (lines 55-72), then the result is looked up in the Partials dictionary (line 74). This means:

   • A slot can point to another slot
   • The final resolved slot name is then checked against the Partials mapping
   • This is likely intentional but should be documented in the code comments

---

Performance Considerations ⚡

Overall: Good performance characteristics

1. Efficient lookup: Uses Dictionary.TryGetValue with case-insensitive comparer - optimal for this use case
2. Bounded iteration: 8-iteration limit prevents runaway loops
3. HashSet for tracking: seen HashSet provides O(1) duplicate detection
4. No memory issues: Local scope, no allocations outside the resolution path

Recommendation: The performance is acceptable. The max depth of 8 should handle even complex slot chains efficiently.

---

Security Concerns 🔒

No significant security issues identified

1. Path traversal protection: Existing path handling logic in TryResolveMappedPath and Path.Combine should prevent directory traversal
2. No user input injection: Slot names come from manifest files (trusted source)
3. Circular reference protection: Properly prevents infinite loops via the seen HashSet

---

Test Coverage 🧪

Current State:

• ✅ Existing ThemeLoaderTests cover basic theme loading, inheritance, and token merging
• ✅ WebSiteScaffolderTests verify scaffolding creates proper structure
• ❌ Missing: No specific tests for the new slot resolution feature

Recommendations:

Add test cases for:

1. Basic slot resolution:

   // Slot "site-header" maps to "header" partial
   manifest.Slots = new() { ["site-header"] = "header" };
   var resolved = loader.ResolvePartialPath(root, manifest, "site-header");
   Assert.Equal(expectedHeaderPath, resolved);

2. Chained slot resolution:

   // Slot "A" -> "B" -> "actual-partial"
   manifest.Slots = new() { 
       ["A"] = "B", 
       ["B"] = "actual-partial" 
   };

3. Circular reference handling:

   // Slot "A" -> "B" -> "A" (should stop gracefully)
   manifest.Slots = new() { 
       ["A"] = "B", 
       ["B"] = "A" 
   };

4. Max depth limit:

   // Create a chain deeper than 8 levels

5. Slot + Partial mapping interaction:

   // Slot resolves to name that exists in Partials mapping

6. Case insensitivity:

   // Verify "Site-Header" matches "site-header"

These tests would ensure the slot resolution behaves correctly in edge cases and prevent regressions.

---

Additional Observations

Positive Changes:

1. Scaffolder improvements:

   • Creates scripts directory (line 109)
   • Sets SchemaVersion = 2 (line 118)
   • Includes default slot mappings (lines 125-130)
   • Generates theme-tokens partial for both engines

2. Manifest naming: Transition from theme.json to theme.manifest.json is well-handled with backward compatibility

3. Layout updates: Both Simple and Scriban templates now use semantic slot names (site-header, site-footer, theme-tokens) instead of direct partial references

Documentation Updates:

The README changes correctly reflect the new manifest filename. Consider updating any other documentation that references theme structure.

---

Summary

Recommendation: ✅ APPROVE with suggested improvements

This is a solid implementation that adds valuable functionality for theme portability. The code is clean, follows existing patterns, and includes proper protections against edge cases.

Before merging:

1. ✅ Extract magic number (8) to named constant
2. ⚠️ Add test coverage for slot resolution scenarios (especially edge cases)
3. 📝 Consider adding more inline documentation for the multi-layered loop protection

After merging:

• Update any additional documentation about theme structure
• Consider adding slot resolution examples to theme documentation

Great work on maintaining backward compatibility and following the established architectural patterns! 🎉

---

Review generated for commit 11d9f3d

[Copilot]

Pull request overview

This PR makes ThemeManifest.Slots functional during partial resolution so layouts can reference stable “slot” names while themes map those to concrete partials, and updates the scaffolder/sample docs to emit and reference the v2 theme.manifest.json contract.

Changes:

• Implement slot indirection in ThemeLoader.ResolvePartialPath to allow resolving stable slot names during partial lookup.
• Update WebSiteScaffolder to generate theme.manifest.json (schemaVersion=2) with default slots and update scaffolded layouts to use slot names.
• Update the sample README to reference theme.manifest.json and the theme-tokens partial.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
Samples/PowerForge.Web.Sample/README.md	Updates documentation references from legacy theme.json to theme.manifest.json.
PowerForge.Web/Services/WebSiteScaffolder.cs	Scaffolds v2 theme manifest with slots and updates generated layouts/partials to use stable slot names.
PowerForge.Web/Services/ThemeLoader.cs	Adds slot-based indirection to partial resolution to make ThemeManifest.Slots effective.

---

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

[Copilot]

ResolvePartialPath treats manifest.Slots values as partial names and then later combines them with partialsPath. This breaks the documented/validated contract where slots values are portable relative paths like partials/slots/hero.html (it would resolve to <theme>/partials/partials/slots/hero.html). Update slot handling to support path-style slot targets (relative to themeRoot) before falling back to treating the value as a partial name/alias.

[Copilot]

The new slot indirection behavior in ResolvePartialPath isn’t covered by tests. Add ThemeLoaderTests that verify slot resolution works for (1) slot values that are theme-root-relative paths (as in docs) and (2) alias chains / loop handling so regressions are caught.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 11d9f3d93e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

HTML-encode site name before injecting into header

Using {{SITE_NAME}} in the simple scaffold header renders the site name without escaping, because SimpleTemplateEngine.BuildTokens currently assigns SITE_NAME as raw text. If a site name contains characters like <, >, or & (or malicious HTML), generated pages will emit broken markup or executable HTML in the header. This is a regression introduced by switching the scaffolded header from a static label to SITE_NAME.

Useful? React with 👍 / 👎.