Web: formalize portable theme contract checks#63
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb763d268
ℹ️ 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".
| if (Path.IsPathRooted(path)) | ||
| return false; | ||
| if (path.StartsWith("/", StringComparison.Ordinal) || path.StartsWith("\\", StringComparison.Ordinal)) | ||
| return false; |
There was a problem hiding this comment.
IsPortableRelativePath currently relies on Path.IsPathRooted plus leading slash checks, which is OS-dependent; when verification runs on Linux/macOS, values like C:\layouts\home.html (and other drive-letter paths) are treated as non-rooted and pass this check. That means ValidateThemeMappedPaths/ValidateThemeAssetPortability can miss exactly the non-portable paths this commit is meant to catch, so portability warnings become inconsistent across CI environments. Add an explicit drive-letter (and ideally URI-scheme) guard before returning true.
Useful? React with 👍 / 👎.
PR Review: Web: formalize portable theme contract checksSummaryThis PR adds comprehensive validation for theme portability by introducing contract checks in the WebSiteVerifier. The changes enforce that theme manifests use relative paths and follow portability best practices. The implementation includes thorough test coverage and updated documentation. ✅ Strengths1. Excellent Code Quality
2. Comprehensive Validation CoverageThe new
3. Well-Designed Helper Methods
4. Strong Test CoverageTwo comprehensive test cases:
5. Documentation AlignmentThe updated Docs/PowerForge.Web.Theme.md:
🔍 Potential Issues & Recommendations1. Missing Test for DefaultLayout Resolution (Minor)The code validates that defaultLayout resolves to an existing file (PowerForge.Web/Services/WebSiteVerifier.cs:352-358), but there's no test covering this scenario. Consider adding a test case. 2. External URL Validation Logic (Minor - Clarification Needed)In Question: Should external URLs be completely disallowed in theme bundles for true portability? The current implementation allows them with a warning, but the documentation emphasizes "portable relative paths." 3. Preload Validation Edge Case (Minor)In Also, there's a second check for 4. Engine Value Case Sensitivity (Minor)The engine validation (PowerForge.Web/Services/WebSiteVerifier.cs:337-340) uses case-insensitive comparison, which is good. Consider whether the warning message should clarify that values are case-insensitive. 5. Error Recovery in ValidateThemeContract (Minor)The try-catch around 6. Test Cleanup Race Condition (Very Minor)Both test methods use 📊 Performance Considerations
🔒 Security Considerations
📝 Documentation Quality
🎯 Best Practices Compliance
Final VerdictAPPROVE ✅ This is a well-implemented feature that adds valuable portability guarantees to the theme system. The code quality is high, test coverage is good, and the documentation improvements are clear. The minor issues identified above are suggestions for potential improvements but don't block approval. Recommended Actions (Optional):
Great work on enforcing portable theme contracts! This will prevent many subtle portability issues in theme development. |
PrzemyslawKlys
deleted the
improve/theme-contract-portability-20260206
branch
There was a problem hiding this comment.
Pull request overview
This PR formalizes theme contract portability by adding validation checks to ensure themes remain portable across different installations and configurations. It validates that theme manifests use relative paths instead of absolute or rooted paths, warns when themes define tokens without providing a theme-tokens partial, and corrects the documentation to accurately reflect the template engine precedence order.
Changes:
- Added
ValidateThemeContractmethod to verify theme manifest portability requirements - Implemented helper validation methods for paths, mapped dictionaries, and asset portability
- Added tests covering non-portable path detection and missing theme-tokens partial warnings
- Updated documentation to reflect correct engine precedence order (site → theme → default) and portability requirements
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PowerForge.Web/Services/WebSiteVerifier.cs | Added comprehensive theme contract validation with methods to check path portability, engine configuration, layout resolution, and token partial requirements |
| PowerForge.Tests/WebSiteVerifierTests.cs | Added two test cases validating warnings for non-portable paths and missing theme-tokens partial |
| Docs/PowerForge.Web.Theme.md | Updated contract status section, corrected engine precedence order to match implementation, and added guidance on portable paths and theme asset declarations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var preload in manifest.Assets.Preloads ?? Array.Empty<PreloadSpec>()) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(preload.Href)) | ||
| continue; | ||
| if (!IsPortableRelativePath(preload.Href) && | ||
| !preload.Href.StartsWith("/", StringComparison.Ordinal) && | ||
| !IsExternalPath(preload.Href)) | ||
| { | ||
| warnings.Add($"{label} preload href should be relative, absolute web URL, or root-relative URL: {preload.Href}"); | ||
| } | ||
|
|
||
| if (preload.Href.StartsWith("/themes/", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| warnings.Add($"{label} preload href hard-codes '/themes/...'. Prefer relative paths in theme assets for portability: {preload.Href}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var critical in manifest.Assets.CriticalCss ?? Array.Empty<CriticalCssSpec>()) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(critical.Path)) | ||
| continue; | ||
| if (!IsPortableRelativePath(critical.Path)) | ||
| { | ||
| warnings.Add($"{label} criticalCss '{critical.Name}' should be a portable relative path: {critical.Path}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Summary: add theme contract portability checks in verifier, add token partial guard, and align theme docs with actual engine precedence. Validation: dotnet test PowerForge.Tests/PowerForge.Tests.csproj --filter "WebSiteVerifierTests|ThemeLoaderTests".