fix(web): normalize edit links, API GitHub actions, and img dimensions#67
Conversation
Pull Request Review: PR #67SummaryThis PR introduces three main enhancements:
Code Quality Assessment✅ Strengths
🔍 Code Review by Component1. EditLinkResolver.cs - Path NormalizationLocation: Good:
Concerns:
// Current approach (lines 119-131)
var stack = new List<string>(parts.Length);
foreach (var part in parts)
{
if (part == ".")
continue;
if (part == "..")
{
if (stack.Count > 0)
stack.RemoveAt(stack.Count - 1); // O(n) operation
continue;
}
stack.Add(part);
}Suggestion: Consider using var stack = new Stack<string>(parts.Length);
foreach (var part in parts)
{
if (part == ".") continue;
if (part == "..")
{
if (stack.Count > 0)
stack.Pop(); // O(1) operation
continue;
}
stack.Push(part);
}
return string.Join("/", stack.Reverse());2. WebApiDocsGenerator.cs - GitHub Edit ActionLocation: Good:
Concerns:
Minor improvement: Consider adding a comment explaining the URL transformation: // Transform GitHub blob URL to edit URL
// Example: /owner/repo/blob/branch/path/file.cs → /owner/repo/edit/branch/path/file.cs3. WebAssetOptimizer.cs - Image DimensionsLocation: Good:
Concerns:
🐛 Potential IssuesNone identified - The code appears solid with no obvious bugs. 🚀 Performance Considerations
🔒 Security ConsiderationsAll good:
🧪 Test CoverageAdequate:
Recommendations:
📝 DocumentationGood: The documentation in 💡 Suggestions for Future Enhancements
VerdictAPPROVE ✅ This is a well-crafted PR that adds useful functionality with proper attention to security and encoding. The code quality is high, and the changes are focused and non-breaking. The only minor concern is the performance characteristic of the path normalization algorithm, but it's acceptable for the expected use case. Before MergingConsider adding the unit tests mentioned above to increase confidence in the path normalization and URL transformation logic. These are complex string operations that benefit from explicit test coverage. Review generated by Claude Code |
PrzemyslawKlys
deleted the
fix/web-editlinks-apidocs-image-sizing-20260206
branch
![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: 024df1bf89
ℹ️ 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 (plan.OriginalHeight > 0 && !ImgHeightAttrRegex.IsMatch(attrsUpdated)) | ||
| { | ||
| attrsUpdated += $" height=\"{plan.OriginalHeight}\""; | ||
| hintedInFile++; | ||
| changed = true; |
There was a problem hiding this comment.
This height injection runs independently from the width check, so an <img> that already has a custom width (for example width="300" or width="100%") will still get height forced to the source file's original pixel height, which can change the rendered aspect ratio and visibly distort images when EnhanceImageTags is enabled. The same issue happens in reverse when only height is authored, so width/height should be added as a pair (or computed proportionally) rather than independently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR enhances the PowerForge web build system with improved edit links, API documentation features, and image optimization. It normalizes edit link paths to collapse . and .. segments for cleaner GitHub URLs, adds GitHub-aware "Edit on GitHub" action buttons to API type documentation pages, and extends image tag enhancement to inject intrinsic width and height attributes alongside existing loading and decoding hints.
Changes:
- Normalize edit link paths to collapse relative path segments (
.and..) for clean GitHub URLs - Add API type-level source action button with GitHub-aware labeling ("Edit on GitHub" vs "View source")
- Inject intrinsic
width/heightattributes on image tags when dimensions are known
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| PowerForge.Web/Services/WebAssetOptimizer.cs | Add regex patterns and tracking for width/height attributes; inject dimensions into image tags when EnhanceImageTags is enabled |
| PowerForge.Web/Services/WebApiDocsGenerator.cs | Add RenderTypeSourceAction and TryBuildGitHubEditUrl methods to generate GitHub edit links for API type source files |
| PowerForge.Web/Services/EditLinkResolver.cs | Enhance NormalizePath to collapse . and .. segments using stack-based algorithm |
| PowerForge.Web/Assets/ApiDocs/fallback.css | Add CSS for type-source-action buttons and custom checkbox styling for member toggles |
| PowerForge.Tests/WebSiteAuditOptimizeBuildTests.cs | Add assertions to verify width and height attributes are injected in generated HTML |
| Docs/PowerForge.Web.Pipeline.md | Update documentation to reflect that width/height are now injected by imageEnhanceTags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (part == "..") | ||
| { | ||
| if (stack.Count > 0) | ||
| stack.RemoveAt(stack.Count - 1); | ||
| continue; |
There was a problem hiding this comment.
The path normalization logic handles ".." segments by removing the previous path component. However, if there are more ".." segments than path components (e.g., "../../file.txt" becomes empty), this could lead to unexpected behavior. Consider whether this is the intended behavior or if a warning/error should be raised when ".." segments would escape the root. For example, if the path is "../../../evil.txt", all ".." are silently ignored once the stack is empty, potentially masking configuration issues.
| foreach (var part in parts) | ||
| { | ||
| if (part == ".") | ||
| continue; | ||
| if (part == "..") | ||
| { | ||
| if (stack.Count > 0) | ||
| stack.RemoveAt(stack.Count - 1); | ||
| continue; | ||
| } | ||
| stack.Add(part); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Summary
./..segments for clean GitHub URLs across mixed content rootsEdit on GitHubwhen source URL is GitHub)width/heightattributes when knownValidation
dotnet test PowerForge.Tests/PowerForge.Tests.csproj -c Release --nologodotnet build PSPublishModule.sln -c Release --nologo