feat(web): image pipeline v2 (next-gen, responsive, budgets) + theme contract v2#66
Conversation
PrzemyslawKlys
deleted the
feat/image-pipeline-v2-contract-v2-20260206
branch
Pull Request Review: Image Pipeline v2, Budgets, and Theme Contract v2SummaryThis PR introduces substantial new functionality for image optimization (next-gen formats, responsive variants, budgets) and theme contract v2 (slots). The implementation is generally well-structured with good test coverage, but I've identified several areas for improvement. ✅ Strengths
🐛 Potential Bugs & IssuesCritical Issues1. Memory Leak Risk in WebAssetOptimizer.cs:584-586 optimizedStream.Position = 0;
using var fileStream = new FileStream(file, FileMode.Create, FileAccess.Write, FileShare.None);
optimizedStream.CopyTo(fileStream);The previous implementation used 2. Race Condition in RewriteHtmlImageTags (line 862+) 3. Budget Validation Timing (line 830) if (options.ImageMaxBytesPerFile > 0 && finalBytes > options.ImageMaxBytesPerFile)This checks budget AFTER optimization but doesn't account for generated variants (WebP/AVIF). A 500KB image optimized to 400KB might generate a 300KB WebP variant, but budget only checks the 400KB version. Budget validation should include variant sizes. Medium Issues4. Missing Null Checks in TryResolveImageReference (line 953+) var htmlDir = Path.GetDirectoryName(htmlFile) ?? siteRoot;
var candidate = Path.GetFullPath(Path.Combine(htmlDir, baseUrl.Replace('/', Path.DirectorySeparatorChar)));If 5. Regex Timeout Risk 6. Width Integer Overflow (line 573, 709) imageWidth = (int)image.Width;
Minor Issues7. Inconsistent Error Handling 8. SupportsQualitySetting Refactoring (line 1111) return format is MagickFormat.Jpeg or MagickFormat.Jpg or MagickFormat.WebP or MagickFormat.Heic or MagickFormat.Heif or MagickFormat.Avif;This is better, but 9. Missing Validation for ResponsiveImageWidths
|
![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: b47a56fb70
ℹ️ 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".
| continue; | ||
| } | ||
|
|
||
| var path = loader.ResolvePartialPath(themeRoot, manifest, partialName); |
There was a problem hiding this comment.
ValidateThemeSlots passes each slot value into ResolvePartialPath, but that resolver treats the input as a partial key/name and prefixes partialsPath when no map key matches. For contract v2 themes that define slots as explicit relative paths (for example partials/slots/hero.html, as documented), this produces a doubled path like partials/partials/... and raises a false “missing partial” warning even when the file exists, which can fail CI flows that gate on warnings.
Useful? React with 👍 / 👎.
|
|
||
| if (srcsetEntries.Count > 0) | ||
| { | ||
| attrsUpdated += $" srcset=\"{string.Join(", ", srcsetEntries)}\""; |
There was a problem hiding this comment.
This adds srcset entries for responsive variants, but the later hash-reference rewrite path only updates href/src attributes, not srcset. In configurations that hash image extensions (for example .webp/.avif), HashAssets renames the generated variant files while srcset keeps pre-hash names, so responsive image requests resolve to missing files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive image optimization pipeline v2 with next-generation format support (WebP/AVIF), responsive image variants, performance hints, and budget controls. It also adds theme contract v2 with slot-based extensibility for improved theme portability and reusability. The changes span schemas, core services, models, CLI integration, tests, and documentation.
Changes:
- Image pipeline v2: WebP/AVIF generation, responsive srcset, lazy loading hints, and byte budgets with fail gates
- Theme contract v2: contractVersion field, slots mapping, and stricter v2 validation rules
- Schema updates for new publish/pipeline/theme fields with both camelCase and kebab-case support
- CLI arguments for all new image pipeline features and budget enforcement
- Comprehensive test coverage for image variants, budgets, and theme slot validation
- Documentation updates for theme contract v2 and pipeline optimize step
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| schemas/powerforge.web.themespec.schema.json | Added contractVersion enum and slots dictionary for theme contract v2 |
| schemas/powerforge.web.publishspec.schema.json | Added image pipeline v2 fields: next-gen formats, responsive widths, hints, budgets |
| schemas/powerforge.web.pipelinespec.schema.json | Added image pipeline fields with camelCase and kebab-case variants for CLI flexibility |
| PowerForge.Web/Services/WebSiteVerifier.cs | Implemented theme contract v2 validation: version checks, slot validation, v2-specific requirements |
| PowerForge.Web/Services/WebAssetOptimizer.cs | Implemented image variant generation, HTML rewriting, srcset injection, lazy loading hints, budget tracking |
| PowerForge.Web/Services/ThemeLoader.cs | Added contractVersion and slots merging in theme inheritance |
| PowerForge.Web/Models/ThemeManifest.cs | Added ContractVersion and Slots properties to theme manifest model |
| PowerForge.Web/Models/WebPublishSpec.cs | Added image pipeline v2 configuration properties |
| PowerForge.Web/Models/WebLlmsResult.cs | Added result tracking for variants, HTML rewrites, hints, and budget warnings |
| PowerForge.Web.Cli/Program.cs | Integrated image pipeline v2 CLI arguments, budget fail gates, result reporting |
| PowerForge.Tests/WebSiteVerifierTests.cs | Added test for theme contract v2 slot validation warnings |
| PowerForge.Tests/WebSiteAuditOptimizeBuildTests.cs | Added tests for image variant generation, hints injection, and budget enforcement |
| Docs/PowerForge.Web.Theme.md | Updated theme documentation for contract v2, slots, and portability guidelines |
| Docs/PowerForge.Web.Pipeline.md | Updated optimize step documentation with image pipeline v2 features |
Comments suppressed due to low confidence (36)
PowerForge.Web/Services/WebAssetOptimizer.cs:362
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var rewrite in rewrites)
{
if (string.IsNullOrWhiteSpace(rewrite.Source) || string.IsNullOrWhiteSpace(rewrite.Destination))
continue;
var source = Path.GetFullPath(rewrite.Source);
if (!File.Exists(source)) continue;
var destRelative = rewrite.Destination.TrimStart('/', '\\');
if (!TryResolveUnderRoot(siteRoot, destRelative, out var dest))
{
Trace.TraceWarning($"Asset rewrite destination outside site root: {rewrite.Destination}");
continue;
}
Directory.CreateDirectory(Path.GetDirectoryName(dest)!);
File.Copy(source, dest, overwrite: true);
}
PowerForge.Web/Services/WebAssetOptimizer.cs:414
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var rewrite in rewrites)
{
if (string.IsNullOrWhiteSpace(rewrite.Match)) continue;
var kind = rewrite.MatchType?.ToLowerInvariant() ?? "contains";
switch (kind)
{
case "exact":
if (string.Equals(url, rewrite.Match, StringComparison.OrdinalIgnoreCase))
return rewrite.Replace;
break;
case "prefix":
if (url.StartsWith(rewrite.Match, StringComparison.OrdinalIgnoreCase))
return rewrite.Replace + url.Substring(rewrite.Match.Length);
break;
case "regex":
try
{
var regex = new Regex(rewrite.Match, RegexOptions.IgnoreCase | RegexOptions.CultureInvariant, RegexTimeout);
if (regex.IsMatch(url))
return regex.Replace(url, rewrite.Replace);
}
catch (Exception ex)
{
Trace.TraceWarning($"Invalid rewrite regex '{rewrite.Match}': {ex.GetType().Name}: {ex.Message}");
}
break;
default:
if (url.IndexOf(rewrite.Match, StringComparison.OrdinalIgnoreCase) >= 0)
return url.Replace(rewrite.Match, rewrite.Replace, StringComparison.OrdinalIgnoreCase);
break;
}
}
PowerForge.Web/Services/WebAssetOptimizer.cs:973
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var ext in source)
{
if (string.IsNullOrWhiteSpace(ext))
continue;
var normalized = ext.Trim();
if (!normalized.StartsWith(".", StringComparison.Ordinal))
normalized = "." + normalized;
set.Add(normalized);
}
PowerForge.Web/Services/WebAssetOptimizer.cs:993
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var pattern in patterns)
{
if (string.IsNullOrWhiteSpace(pattern)) continue;
var normalizedPattern = pattern.Replace('\\', '/');
if (GlobMatch(normalizedPattern, normalized) || GlobMatch(normalizedPattern, withLeadingSlash))
return true;
}
PowerForge.Web/Services/WebAssetOptimizer.cs:1008
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var pattern in patterns)
{
if (string.IsNullOrWhiteSpace(pattern)) continue;
var normalizedPattern = pattern.Replace('\\', '/');
if (GlobMatch(normalizedPattern, normalized) || GlobMatch(normalizedPattern, withLeadingSlash))
return true;
}
PowerForge.Web/Services/WebSiteVerifier.cs:96
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var collection in spec.Collections)
{
if (collection is null) continue;
var files = EnumerateCollectionFiles(plan.RootPath, collection.Input).ToArray();
if (files.Length == 0)
{
warnings.Add($"Collection '{collection.Name}' has no files.");
continue;
}
var leafBundleRoots = BuildLeafBundleRoots(files);
foreach (var file in files)
{
if (IsUnderAnyRoot(file, leafBundleRoots) && !IsLeafBundleIndex(file))
continue;
var markdown = File.ReadAllText(file);
var (matter, body) = FrontMatterParser.Parse(markdown);
var title = matter?.Title ?? FrontMatterParser.ExtractTitleFromMarkdown(body) ?? string.Empty;
if (string.IsNullOrWhiteSpace(title))
{
errors.Add($"Missing title in: {file}");
}
ValidateMarkdownHygiene(plan.RootPath, file, collection.Name, body, warnings);
var collectionRoot = ResolveCollectionRootForFile(plan.RootPath, collection.Input, file);
var relativePath = ResolveRelativePath(collectionRoot, file);
var relativeDir = NormalizePath(Path.GetDirectoryName(relativePath) ?? string.Empty);
var isSectionIndex = IsSectionIndex(file);
var isBundleIndex = IsLeafBundleIndex(file);
var slugPath = ResolveSlugPath(relativePath, relativeDir, matter?.Slug);
if (isSectionIndex || isBundleIndex)
slugPath = ApplySlugOverride(relativeDir, matter?.Slug);
if (string.IsNullOrWhiteSpace(slugPath))
{
errors.Add($"Missing slug in: {file}");
continue;
}
var projectSlug = ResolveProjectSlug(plan, file);
var baseOutput = ReplaceProjectPlaceholder(collection.Output, projectSlug);
var route = BuildRoute(baseOutput, slugPath, spec.TrailingSlash);
if (routes.TryGetValue(route, out var existing))
{
errors.Add($"Duplicate route '{route}' from '{file}' and '{existing}'.");
}
else
{
routes[route] = file;
}
if (!collectionRoutes.TryGetValue(collection.Name, out var list))
{
list = new List<CollectionRoute>();
collectionRoutes[collection.Name] = list;
}
list.Add(new CollectionRoute(route, file, matter?.Draft ?? false));
}
}
PowerForge.Web/Services/WebSiteVerifier.cs:95
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var file in files)
{
if (IsUnderAnyRoot(file, leafBundleRoots) && !IsLeafBundleIndex(file))
continue;
var markdown = File.ReadAllText(file);
var (matter, body) = FrontMatterParser.Parse(markdown);
var title = matter?.Title ?? FrontMatterParser.ExtractTitleFromMarkdown(body) ?? string.Empty;
if (string.IsNullOrWhiteSpace(title))
{
errors.Add($"Missing title in: {file}");
}
ValidateMarkdownHygiene(plan.RootPath, file, collection.Name, body, warnings);
var collectionRoot = ResolveCollectionRootForFile(plan.RootPath, collection.Input, file);
var relativePath = ResolveRelativePath(collectionRoot, file);
var relativeDir = NormalizePath(Path.GetDirectoryName(relativePath) ?? string.Empty);
var isSectionIndex = IsSectionIndex(file);
var isBundleIndex = IsLeafBundleIndex(file);
var slugPath = ResolveSlugPath(relativePath, relativeDir, matter?.Slug);
if (isSectionIndex || isBundleIndex)
slugPath = ApplySlugOverride(relativeDir, matter?.Slug);
if (string.IsNullOrWhiteSpace(slugPath))
{
errors.Add($"Missing slug in: {file}");
continue;
}
var projectSlug = ResolveProjectSlug(plan, file);
var baseOutput = ReplaceProjectPlaceholder(collection.Output, projectSlug);
var route = BuildRoute(baseOutput, slugPath, spec.TrailingSlash);
if (routes.TryGetValue(route, out var existing))
{
errors.Add($"Duplicate route '{route}' from '{file}' and '{existing}'.");
}
else
{
routes[route] = file;
}
if (!collectionRoutes.TryGetValue(collection.Name, out var list))
{
list = new List<CollectionRoute>();
collectionRoutes[collection.Name] = list;
}
list.Add(new CollectionRoute(route, file, matter?.Draft ?? false));
}
PowerForge.Web/Services/WebSiteVerifier.cs:240
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var root in roots)
{
if (string.IsNullOrWhiteSpace(root)) continue;
if (filePath.StartsWith(root + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) ||
filePath.Equals(root, StringComparison.OrdinalIgnoreCase))
return true;
}
PowerForge.Web/Services/WebSiteVerifier.cs:273
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var project in plan.Projects ?? Array.Empty<WebProjectPlan>())
{
if (string.IsNullOrWhiteSpace(project.RootPath))
continue;
var projectDataRoot = Path.Combine(project.RootPath, "data");
ValidateKnownDataFile(projectDataRoot, "faq.json", $"projects/{project.Slug}/data/faq.json", ValidateFaqJson, warnings);
ValidateKnownDataFile(projectDataRoot, "showcase.json", $"projects/{project.Slug}/data/showcase.json", ValidateShowcaseJson, warnings);
ValidateKnownDataFile(projectDataRoot, "pricing.json", $"projects/{project.Slug}/data/pricing.json", ValidatePricingJson, warnings);
ValidateKnownDataFile(projectDataRoot, "benchmarks.json", $"projects/{project.Slug}/data/benchmarks.json", ValidateBenchmarksJson, warnings);
}
PowerForge.Web/Services/WebSiteVerifier.cs:485
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var token in requiredTokens)
{
if (content.IndexOf(token, StringComparison.OrdinalIgnoreCase) >= 0)
continue;
warnings.Add($"Layout '{layoutName}' is missing required token '{token}'. " +
"Per-page assets (e.g., Prism) may not load.");
}
PowerForge.Web/Services/WebSiteVerifier.cs:676
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var key in manifest.Layouts.Keys)
{
if (!string.IsNullOrWhiteSpace(key))
names.Add(key);
}
PowerForge.Web/Services/WebSiteVerifier.cs:835
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var collection in spec.Collections)
{
if (collection is null) continue;
if (collection.UseToc == false) continue;
if (!collectionRoutes.TryGetValue(collection.Name, out var routes) || routes.Count == 0)
continue;
var tocPath = ResolveTocPath(collection, plan.RootPath);
if (string.IsNullOrWhiteSpace(tocPath) || !File.Exists(tocPath))
{
warnings.Add($"TOC is enabled for collection '{collection.Name}' but no toc.json/toc.yml was found.");
continue;
}
var tocItems = LoadTocFromPath(tocPath);
if (tocItems.Length == 0)
continue;
var tocUrls = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
CollectTocUrls(tocItems, tocUrls, spec.TrailingSlash);
if (tocUrls.Count == 0)
continue;
var outputRoot = string.IsNullOrWhiteSpace(collection.Output) ? "/" : collection.Output;
var normalizedRoot = NormalizeRouteForCompare(outputRoot, spec.TrailingSlash);
var routeSet = routes
.Where(r => !r.Draft)
.Select(r => r.Route)
.ToHashSet(StringComparer.OrdinalIgnoreCase);
var missing = routeSet
.Where(r => !tocUrls.Contains(r))
.ToList();
if (missing.Count > 0)
{
var preview = string.Join(", ", missing.Take(5));
var suffix = missing.Count > 5 ? " ..." : string.Empty;
warnings.Add($"TOC for collection '{collection.Name}' is missing {missing.Count} page(s): {preview}{suffix}");
}
var extra = tocUrls
.Where(u => u.StartsWith(normalizedRoot, StringComparison.OrdinalIgnoreCase))
.Where(u => !routeSet.Contains(u))
.ToList();
if (extra.Count > 0)
{
var preview = string.Join(", ", extra.Take(5));
var suffix = extra.Count > 5 ? " ..." : string.Empty;
warnings.Add($"TOC for collection '{collection.Name}' contains {extra.Count} missing page(s): {preview}{suffix}");
}
}
PowerForge.Web/Services/WebSiteVerifier.cs:911
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var item in items ?? Array.Empty<TocItem>())
{
if (item is null || item.Hidden) continue;
var url = item.Url ?? item.Href;
var normalized = NormalizeTocUrl(url, slashMode);
if (!string.IsNullOrWhiteSpace(normalized))
urls.Add(normalized);
if (item.Items is { Length: > 0 })
CollectTocUrls(item.Items, urls, slashMode);
}
PowerForge.Web/Services/WebSiteVerifier.cs:991
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var name in mapping.Bundles ?? Array.Empty<string>())
{
if (string.IsNullOrWhiteSpace(name))
continue;
if (!seen.Add(name))
continue;
if (bundleMap.TryGetValue(name, out var bundle))
selected.Add(bundle);
}
PowerForge.Web/Services/WebSiteVerifier.cs:1040
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var css in assets.CriticalCss ?? Array.Empty<CriticalCssSpec>())
{
if (string.IsNullOrWhiteSpace(css.Path)) continue;
if (IsExternalPath(css.Path)) continue;
var fullPath = Path.IsPathRooted(css.Path)
? css.Path
: Path.Combine(rootPath, css.Path);
if (!File.Exists(fullPath))
warnings.Add($"Missing {label} critical CSS: {css.Path}");
}
PowerForge.Web/Services/WebSiteVerifier.cs:1121
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
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}");
}
}
PowerForge.Web/Services/WebSiteVerifier.cs:1131
- 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}");
}
}
PowerForge.Web/Services/WebSiteVerifier.cs:1375
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var prop in properties)
{
if (element.TryGetProperty(prop, out _))
return true;
}
PowerForge.Web/Services/WebSiteVerifier.cs:1491
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var project in plan.Projects)
{
if (string.IsNullOrWhiteSpace(project.ContentPath))
continue;
if (filePath.StartsWith(project.ContentPath, StringComparison.OrdinalIgnoreCase))
return project.Slug;
}
PowerForge.Web.Cli/Program.cs:1742
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var issue in result.Issues)
{
if (!string.IsNullOrWhiteSpace(issue.Key))
keys.Add(issue.Key);
}
PowerForge.Web.Cli/Program.cs:1812
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var item in issueKeys.EnumerateArray())
{
if (item.ValueKind != JsonValueKind.String) continue;
var value = item.GetString();
if (!string.IsNullOrWhiteSpace(value))
keys.Add(value);
}
PowerForge.Web.Cli/Program.cs:1824
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var issue in issues.EnumerateArray())
{
if (issue.ValueKind != JsonValueKind.Object) continue;
if (!TryGetPropertyIgnoreCase(issue, "key", out var keyElement) || keyElement.ValueKind != JsonValueKind.String) continue;
var value = keyElement.GetString();
if (!string.IsNullOrWhiteSpace(value))
keys.Add(value);
}
PowerForge.Web.Cli/Program.cs:1861
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var property in element.EnumerateObject())
{
if (property.Name.Equals(propertyName, StringComparison.OrdinalIgnoreCase))
{
value = property.Value;
return true;
}
}
PowerForge.Web.Cli/Program.cs:1965
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var dependencyIndex in definition.DependencyIndexes)
{
if (!stepResultsByIndex.TryGetValue(dependencyIndex, out var dependencyResult) || !dependencyResult.Success)
{
throw new InvalidOperationException($"Step '{definition.Id}' dependency #{dependencyIndex} failed or was not executed.");
}
}
PowerForge.Web.Cli/Program.cs:2735
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var step in steps)
{
if (step.DependsOn.Length == 0)
continue;
var resolved = new List<int>();
foreach (var dependency in step.DependsOn)
{
if (string.IsNullOrWhiteSpace(dependency))
continue;
if (int.TryParse(dependency, out var numeric))
{
if (numeric <= 0 || numeric > steps.Count)
throw new InvalidOperationException($"Step '{step.Id}' has invalid dependsOn reference '{dependency}'.");
resolved.Add(numeric);
continue;
}
if (!aliases.TryGetValue(dependency, out var dependencyIndex))
throw new InvalidOperationException($"Step '{step.Id}' has unknown dependsOn reference '{dependency}'.");
resolved.Add(dependencyIndex);
}
step.DependencyIndexes = resolved
.Distinct()
.OrderBy(value => value)
.ToArray();
if (step.DependencyIndexes.Any(value => value >= step.Index))
throw new InvalidOperationException($"Step '{step.Id}' has dependsOn reference to current/future step.");
}
PowerForge.Web.Cli/Program.cs:2726
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var dependency in step.DependsOn)
{
if (string.IsNullOrWhiteSpace(dependency))
continue;
if (int.TryParse(dependency, out var numeric))
{
if (numeric <= 0 || numeric > steps.Count)
throw new InvalidOperationException($"Step '{step.Id}' has invalid dependsOn reference '{dependency}'.");
resolved.Add(numeric);
continue;
}
if (!aliases.TryGetValue(dependency, out var dependencyIndex))
throw new InvalidOperationException($"Step '{step.Id}' has unknown dependsOn reference '{dependency}'.");
resolved.Add(dependencyIndex);
}
PowerForge.Web.Cli/Program.cs:2959
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var property in step.EnumerateObject())
{
if (!FingerprintPathKeys.Contains(property.Name))
continue;
if (property.Value.ValueKind == JsonValueKind.String)
{
var value = property.Value.GetString();
if (string.IsNullOrWhiteSpace(value) || IsExternalUri(value))
continue;
var resolved = ResolvePath(baseDir, value);
if (!string.IsNullOrWhiteSpace(resolved))
yield return Path.GetFullPath(resolved);
continue;
}
if (property.Value.ValueKind != JsonValueKind.Array)
continue;
foreach (var item in property.Value.EnumerateArray())
{
if (item.ValueKind != JsonValueKind.String)
continue;
var value = item.GetString();
if (string.IsNullOrWhiteSpace(value) || IsExternalUri(value))
continue;
var resolved = ResolvePath(baseDir, value);
if (!string.IsNullOrWhiteSpace(resolved))
yield return Path.GetFullPath(resolved);
}
}
PowerForge.Web.Cli/Program.cs:2958
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var item in property.Value.EnumerateArray())
{
if (item.ValueKind != JsonValueKind.String)
continue;
var value = item.GetString();
if (string.IsNullOrWhiteSpace(value) || IsExternalUri(value))
continue;
var resolved = ResolvePath(baseDir, value);
if (!string.IsNullOrWhiteSpace(resolved))
yield return Path.GetFullPath(resolved);
}
PowerForge.Web.Cli/Program.cs:3113
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var output in outputs)
{
if (File.Exists(output))
continue;
if (Directory.Exists(output))
continue;
return false;
}
PowerForge.Web.Cli/Program.cs:3216
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var item in value.EnumerateArray())
{
if (item.ValueKind != JsonValueKind.Object) continue;
var path = GetString(item, "path") ?? GetString(item, "route") ?? GetString(item, "url");
if (string.IsNullOrWhiteSpace(path)) continue;
list.Add(new WebSitemapEntry
{
Path = path,
ChangeFrequency = GetString(item, "changefreq") ?? GetString(item, "changeFrequency"),
Priority = GetString(item, "priority"),
LastModified = GetString(item, "lastmod") ?? GetString(item, "lastModified")
});
}
PowerForge.Web/Services/WebAssetOptimizer.cs:1066
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (var path in map.Values)
{
var segments = path.TrimStart('/').Split('/');
if (segments.Length > 1)
immutablePaths.Add($"/{segments[0]}/*");
}
PowerForge.Web/Services/WebSiteVerifier.cs:686
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (var collection in spec.Collections)
{
var layout = collection?.DefaultLayout;
if (!string.IsNullOrWhiteSpace(layout))
names.Add(layout);
}
PowerForge.Web/Services/WebSiteVerifier.cs:1330
- These 'if' statements can be combined.
if (TryGetObject(root, "hero", out var hero))
{
if (!HasAnyProperty(hero, "title"))
warnings.Add($"Data file '{label}' hero missing 'title'.");
}
PowerForge.Web/Services/WebSiteVerifier.cs:783
- The expression 'A == false' can be simplified to '!A'.
if (collection.UseToc == false) continue;
PowerForge.Web/Services/WebSiteVerifier.cs:840
- The expression 'A == false' can be simplified to '!A'.
if (collection.UseToc == false)
PowerForge.Web/Services/ThemeLoader.cs:199
- Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
if (merged.TryGetValue(kvp.Key, out var existing) &&
existing is Dictionary<string, object?> existingMap &&
value is Dictionary<string, object?> childMap)
{
merged[kvp.Key] = MergeTokens(existingMap, childMap);
}
else
{
merged[kvp.Key] = value;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (webpBytes.LongLength > 0 && webpBytes.LongLength < finalBytes && | ||
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | ||
| { | ||
| generatedVariants.Add(new WebOptimizeImageVariantEntry | ||
| { | ||
| SourcePath = relative, | ||
| VariantPath = webpRelative, | ||
| Format = "webp", | ||
| Width = null, | ||
| Bytes = webpBytes.LongLength | ||
| }); | ||
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | ||
| { | ||
| preferredBytes = webpBytes.LongLength; | ||
| plan.PreferredRelativePath = webpRelative; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (options.ImageGenerateAvif && TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes)) | ||
| { | ||
| if (avifBytes.LongLength > 0 && avifBytes.LongLength < finalBytes && | ||
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) |
There was a problem hiding this comment.
The AVIF variant generation checks avifBytes.LongLength < finalBytes which compares against the source image size. However, if WebP was already generated and is smaller, the AVIF check should compare against preferredBytes instead to avoid generating AVIF variants that are smaller than the source but larger than WebP (and thus will never be preferred or used). This creates unnecessary disk usage. The same issue applies to the WebP check on line 611 if the order were reversed.
| result.ImageBytesAfter += finalBytes; | ||
| if (options.ImageMaxBytesPerFile > 0 && finalBytes > options.ImageMaxBytesPerFile) | ||
| budgetWarnings.Add($"Image '{relative}' exceeds max-bytes-per-file ({finalBytes} > {options.ImageMaxBytesPerFile})."); |
There was a problem hiding this comment.
The image budget check uses finalBytes which represents the optimized source image size, but when ImagePreferNextGen is enabled and a next-gen variant (WebP/AVIF) is preferred, the budget should check against preferredBytes instead. This ensures the budget reflects the actual image bytes that will be served to users after HTML rewriting, not the intermediate source image bytes.
| result.ImageBytesAfter += finalBytes; | |
| if (options.ImageMaxBytesPerFile > 0 && finalBytes > options.ImageMaxBytesPerFile) | |
| budgetWarnings.Add($"Image '{relative}' exceeds max-bytes-per-file ({finalBytes} > {options.ImageMaxBytesPerFile})."); | |
| result.ImageBytesAfter += preferredBytes; | |
| if (options.ImageMaxBytesPerFile > 0 && preferredBytes > options.ImageMaxBytesPerFile) | |
| budgetWarnings.Add($"Image '{relative}' exceeds max-bytes-per-file ({preferredBytes} > {options.ImageMaxBytesPerFile})."); |
| continue; | ||
| if (!TryEncodeVariant(image, width, responsiveFormat, quality, out var responsiveBytes)) | ||
| continue; | ||
| if (responsiveBytes.LongLength <= 0 || responsiveBytes.LongLength >= preferredBytes) |
There was a problem hiding this comment.
The condition responsiveBytes.LongLength >= preferredBytes rejects responsive variants that are larger than or equal to the preferred (full-size) image. However, this prevents generating responsive variants when they have the same size as the preferred image. In practice, smaller responsive variants (e.g., 480w) that happen to compress to the same size as the full image could still be useful for bandwidth savings on mobile devices. Consider changing to strictly greater than (>) to allow equal-sized variants, or document why equal-sized variants should be rejected.
| if (responsiveBytes.LongLength <= 0 || responsiveBytes.LongLength >= preferredBytes) | |
| if (responsiveBytes.LongLength <= 0 || responsiveBytes.LongLength > preferredBytes) |
| { | ||
| attrsUpdated += $" srcset=\"{string.Join(", ", srcsetEntries)}\""; | ||
| if (!ImgSizesAttrRegex.IsMatch(attrsUpdated)) | ||
| attrsUpdated += " sizes=\"100vw\""; |
There was a problem hiding this comment.
The default sizes="100vw" attribute assumes images span the full viewport width. While this is a safe default that works for many layouts, it may cause browsers to download larger images than necessary for images constrained by container width or CSS. Consider documenting that users should manually specify the sizes attribute for optimal responsive image behavior, or consider a more sophisticated default like sizes="(max-width: 768px) 100vw, 768px" for better mobile optimization.
| attrsUpdated += " sizes=\"100vw\""; | |
| attrsUpdated += " sizes=\"(max-width: 768px) 100vw, 768px\""; |
| foreach (var width in responsiveWidths) | ||
| { | ||
| if (width >= imageWidth) | ||
| continue; | ||
| if (!TryEncodeVariant(image, width, responsiveFormat, quality, out var responsiveBytes)) | ||
| continue; | ||
| if (responsiveBytes.LongLength <= 0 || responsiveBytes.LongLength >= preferredBytes) | ||
| continue; | ||
| if (!TryWriteVariant(siteRoot, plan.PreferredRelativePath, width, responsiveExtension, responsiveBytes, out var variantRelative, onUpdated)) | ||
| continue; | ||
|
|
||
| plan.ResponsiveVariants.Add(new ImageVariantPlan | ||
| { | ||
| RelativePath = variantRelative, | ||
| Width = width | ||
| }); | ||
| generatedVariants.Add(new WebOptimizeImageVariantEntry | ||
| { | ||
| SourcePath = relative, | ||
| VariantPath = variantRelative, | ||
| Format = responsiveExtension, | ||
| Width = width, | ||
| Bytes = responsiveBytes.LongLength | ||
| }); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var value in values) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(value)) | ||
| continue; | ||
| foreach (var token in value.Split(new[] { ',', ';' }, StringSplitOptions.RemoveEmptyEntries)) | ||
| { | ||
| if (int.TryParse(token.Trim(), out var parsed) && parsed > 0) | ||
| list.Add(parsed); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var token in value.Split(new[] { ',', ';' }, StringSplitOptions.RemoveEmptyEntries)) | ||
| { | ||
| if (int.TryParse(token.Trim(), out var parsed) && parsed > 0) | ||
| list.Add(parsed); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| if (options.ImageGenerateWebp && TryEncodeVariant(image, null, MagickFormat.WebP, quality, out var webpBytes)) | ||
| { | ||
| if (webpBytes.LongLength > 0 && webpBytes.LongLength < finalBytes && | ||
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | ||
| { | ||
| generatedVariants.Add(new WebOptimizeImageVariantEntry | ||
| { | ||
| SourcePath = relative, | ||
| VariantPath = webpRelative, | ||
| Format = "webp", | ||
| Width = null, | ||
| Bytes = webpBytes.LongLength | ||
| }); | ||
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | ||
| { | ||
| preferredBytes = webpBytes.LongLength; | ||
| plan.PreferredRelativePath = webpRelative; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (options.ImageGenerateAvif && TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes)) | ||
| { | ||
| if (avifBytes.LongLength > 0 && avifBytes.LongLength < finalBytes && | ||
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) | ||
| { | ||
| generatedVariants.Add(new WebOptimizeImageVariantEntry | ||
| { | ||
| SourcePath = relative, | ||
| VariantPath = avifRelative, | ||
| Format = "avif", | ||
| Width = null, | ||
| Bytes = avifBytes.LongLength | ||
| }); | ||
| if (options.ImagePreferNextGen && avifBytes.LongLength < preferredBytes) | ||
| { | ||
| preferredBytes = avifBytes.LongLength; | ||
| plan.PreferredRelativePath = avifRelative; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (options.ImageGenerateWebp && TryEncodeVariant(image, null, MagickFormat.WebP, quality, out var webpBytes)) | |
| { | |
| if (webpBytes.LongLength > 0 && webpBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = webpRelative, | |
| Format = "webp", | |
| Width = null, | |
| Bytes = webpBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = webpBytes.LongLength; | |
| plan.PreferredRelativePath = webpRelative; | |
| } | |
| } | |
| } | |
| if (options.ImageGenerateAvif && TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes)) | |
| { | |
| if (avifBytes.LongLength > 0 && avifBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = avifRelative, | |
| Format = "avif", | |
| Width = null, | |
| Bytes = avifBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && avifBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = avifBytes.LongLength; | |
| plan.PreferredRelativePath = avifRelative; | |
| } | |
| if (options.ImageGenerateWebp && | |
| TryEncodeVariant(image, null, MagickFormat.WebP, quality, out var webpBytes) && | |
| webpBytes.LongLength > 0 && | |
| webpBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = webpRelative, | |
| Format = "webp", | |
| Width = null, | |
| Bytes = webpBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = webpBytes.LongLength; | |
| plan.PreferredRelativePath = webpRelative; | |
| } | |
| } | |
| if (options.ImageGenerateAvif && | |
| TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes) && | |
| avifBytes.LongLength > 0 && | |
| avifBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = avifRelative, | |
| Format = "avif", | |
| Width = null, | |
| Bytes = avifBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && avifBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = avifBytes.LongLength; | |
| plan.PreferredRelativePath = avifRelative; |
| if (options.ImageGenerateWebp && TryEncodeVariant(image, null, MagickFormat.WebP, quality, out var webpBytes)) | ||
| { | ||
| if (webpBytes.LongLength > 0 && webpBytes.LongLength < finalBytes && | ||
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | ||
| { | ||
| generatedVariants.Add(new WebOptimizeImageVariantEntry | ||
| { | ||
| SourcePath = relative, | ||
| VariantPath = webpRelative, | ||
| Format = "webp", | ||
| Width = null, | ||
| Bytes = webpBytes.LongLength | ||
| }); | ||
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | ||
| { | ||
| preferredBytes = webpBytes.LongLength; | ||
| plan.PreferredRelativePath = webpRelative; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (options.ImageGenerateAvif && TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes)) | ||
| { | ||
| if (avifBytes.LongLength > 0 && avifBytes.LongLength < finalBytes && | ||
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) | ||
| { | ||
| generatedVariants.Add(new WebOptimizeImageVariantEntry | ||
| { | ||
| SourcePath = relative, | ||
| VariantPath = avifRelative, | ||
| Format = "avif", | ||
| Width = null, | ||
| Bytes = avifBytes.LongLength | ||
| }); | ||
| if (options.ImagePreferNextGen && avifBytes.LongLength < preferredBytes) | ||
| { | ||
| preferredBytes = avifBytes.LongLength; | ||
| plan.PreferredRelativePath = avifRelative; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (options.ImageGenerateWebp && TryEncodeVariant(image, null, MagickFormat.WebP, quality, out var webpBytes)) | |
| { | |
| if (webpBytes.LongLength > 0 && webpBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = webpRelative, | |
| Format = "webp", | |
| Width = null, | |
| Bytes = webpBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = webpBytes.LongLength; | |
| plan.PreferredRelativePath = webpRelative; | |
| } | |
| } | |
| } | |
| if (options.ImageGenerateAvif && TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes)) | |
| { | |
| if (avifBytes.LongLength > 0 && avifBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = avifRelative, | |
| Format = "avif", | |
| Width = null, | |
| Bytes = avifBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && avifBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = avifBytes.LongLength; | |
| plan.PreferredRelativePath = avifRelative; | |
| } | |
| if (options.ImageGenerateWebp && | |
| TryEncodeVariant(image, null, MagickFormat.WebP, quality, out var webpBytes) && | |
| webpBytes.LongLength > 0 && | |
| webpBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "webp", webpBytes, out var webpRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = webpRelative, | |
| Format = "webp", | |
| Width = null, | |
| Bytes = webpBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && webpBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = webpBytes.LongLength; | |
| plan.PreferredRelativePath = webpRelative; | |
| } | |
| } | |
| if (options.ImageGenerateAvif && | |
| TryEncodeVariant(image, null, MagickFormat.Avif, quality, out var avifBytes) && | |
| avifBytes.LongLength > 0 && | |
| avifBytes.LongLength < finalBytes && | |
| TryWriteVariant(siteRoot, relative, null, "avif", avifBytes, out var avifRelative, onUpdated)) | |
| { | |
| generatedVariants.Add(new WebOptimizeImageVariantEntry | |
| { | |
| SourcePath = relative, | |
| VariantPath = avifRelative, | |
| Format = "avif", | |
| Width = null, | |
| Bytes = avifBytes.LongLength | |
| }); | |
| if (options.ImagePreferNextGen && avifBytes.LongLength < preferredBytes) | |
| { | |
| preferredBytes = avifBytes.LongLength; | |
| plan.PreferredRelativePath = avifRelative; |
Summary
srcsetgenerationloading/decodingimage hint injectionmax-bytes-per-file,max-total-bytes) and fail gates in CLI/pipeline/publishcontractVersion,slots) + verifier checks and docs updatesValidation
dotnet test PowerForge.Tests/PowerForge.Tests.csproj -c Releasedotnet build PSPublishModule.sln -c ReleaseIntelligenceX/Website ./build.ps1CodeMatrix/Website ./build.ps1