Replace MiKTeX/LaTeX reporting with Markdown + QuestPDF - #325
Merged
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
msevestre
force-pushed
the
183-move-away-from-miktex
branch
from
February 19, 2026 01:50
2c47994 to
3f2ec46
Compare
Removes the dependency on OSPSuite.TeXReporting and MiKTeX, replacing it with a simpler, pure .NET solution: - Markdown reports with embedded SVG charts (viewable in VS Code, GitHub) - PDF reports via QuestPDF (no external tools required) - Custom SVG chart generator for 2D line charts with log/linear scale - Report format switchable via ReportFormat enum New files: - Reporting/Charts/SvgChartGenerator.cs - SVG line chart generation - Reporting/Markdown/* - Markdown builder pattern implementation - Reporting/Pdf/PdfReportDocument.cs - QuestPDF document - Services/MarkdownReportingTask.cs, PdfReportingTask.cs Removed: - All *TeXBuilder.cs files - *Reporter.cs files - OSPSuite.TeXReporting dependency from all projects Closes #183 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
msevestre
force-pushed
the
183-move-away-from-miktex
branch
from
February 19, 2026 01:58
661ca12 to
dc49f4f
Compare
The method was removed during cross-platform refactoring but is needed by OperatingSystemInfoSpecs tests that use reflection to call it. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Keep the original Windows-specific OperatingSystemInfo that uses Registry, WMI, and SystemInformation APIs. Update Core and Tests projects to target net8.0-windows. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (17)
src/SimulationOutputComparer/SimulationOutputComparer.csproj (1)
41-41: Nit: inconsistent indentation on closing</ItemGroup>tag.The opening
<ItemGroup>on line 28 uses 2-space indentation, but this closing tag uses 4 spaces. Likely a leftover from the removal of the TeXReporting content lines.Proposed fix
- </ItemGroup> + </ItemGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SimulationOutputComparer/SimulationOutputComparer.csproj` at line 41, Adjust the inconsistent indentation of the closing XML tag so it matches the opening <ItemGroup> indentation: change the closing </ItemGroup> tag indentation from 4 spaces to 2 spaces to align with the opening <ItemGroup> (ensure indentation consistency within SimulationOutputComparer.csproj).src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs (1)
202-205:colorToHexis duplicated inMarkdownReportContext.AppendColoredStatus(line 43 of that file).Extract to a shared static utility (e.g.,
ColorExtensions.ToHex(this Color c)) to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs` around lines 202 - 205, The colorToHex method is duplicated (SvgChartGenerator.colorToHex and MarkdownReportContext.AppendColoredStatus); create a single shared static extension method like ColorExtensions.ToHex(this Color c) that returns $"#{c.R:X2}{c.G:X2}{c.B:X2}", replace calls to colorToHex in SvgChartGenerator and the logic inside MarkdownReportContext.AppendColoredStatus to use ColorExtensions.ToHex, and remove the duplicate colorToHex implementation to prevent divergence.tests/InstallationValidator.Tests/InstallationValidator.Tests.csproj (1)
31-31: Misaligned</ItemGroup>closing tag (same issue asInstallationValidator.csproj).🔧 Suggested fix
- </ItemGroup> + </ItemGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/InstallationValidator.Tests.csproj` at line 31, The closing XML tag </ItemGroup> in the project file is misaligned; locate the corresponding opening <ItemGroup> block in InstallationValidator.Tests (look for the matching <ItemGroup> that contains PackageReference or ProjectReference entries) and re-indent the </ItemGroup> to align with its opening tag so the XML block is properly formatted and well-formed; ensure there are no extra or missing ItemGroup tags and run an XML/project build to verify correctness.src/InstallationValidator/InstallationValidator.csproj (1)
41-41: Misaligned</ItemGroup>closing tag (4-space indent vs. 2-space opening).🔧 Suggested fix
- </ItemGroup> + </ItemGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator/InstallationValidator.csproj` at line 41, The closing ItemGroup tag is misindented (4 spaces) compared to its opening tag (2 spaces); edit the csproj so the `</ItemGroup>` closing tag uses the same 2-space indentation as the corresponding `<ItemGroup>` tag to keep XML formatting consistent.src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs (1)
21-24: Hard cast inBuild(object)gives an opaqueInvalidCastExceptionon misuse.A direct
(T)objectToReportcast, while correct if the repository routes types accurately, makes debugging difficult when a wrong type is passed. A minor improvement is to validate with anas-cast and a descriptive message.♻️ Suggested defensive cast
public void Build(object objectToReport, MarkdownReportContext context) { - Build((T)objectToReport, context); + if (objectToReport is not T typed) + throw new InvalidCastException( + $"{GetType().Name} expected {typeof(T).Name} but received {objectToReport?.GetType().Name ?? "null"}."); + Build(typed, context); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs` around lines 21 - 24, The current Build(object objectToReport, MarkdownReportContext context) uses a hard cast ((T)objectToReport) which produces an opaque InvalidCastException; change it to use an 'as' cast to T (or a safe pattern match), check for null, and throw a descriptive ArgumentException including the expected type (typeof(T)) and the actual object type (objectToReport?.GetType()) before calling Build(T, context) so callers get a clear error message; update the Build(object...) implementation in IMarkdownBuilder (the Build(object objectToReport, MarkdownReportContext context) method) accordingly.src/InstallationValidator.Core/Reporting/Markdown/OperatingSystemInfoMarkdownBuilder.cs (1)
7-17: Clean implementation. The builder correctly delegates toMarkdownReportContextmethods and uses asset constants for labels.Minor note: Line 12 uses a hardcoded
"OS"label while all other labels referenceAssets.Reporting.*constants (e.g.,ComputerName,Architecture). Consider usingAssets.Reporting.OperatingSystem(or a shorter constant if preferred) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/OperatingSystemInfoMarkdownBuilder.cs` around lines 7 - 17, In OperatingSystemInfoMarkdownBuilder.Build, the second list item uses a hardcoded "OS" label; change that string to use the appropriate asset constant (e.g., Assets.Reporting.OperatingSystem or another shorter asset) so the line that appends operatingSystem.FriendlyName uses the same Assets.Reporting.* pattern as the other list items; update the call in Build to reference the constant (locate the method Build in class OperatingSystemInfoMarkdownBuilder and replace the hardcoded label in the context.AppendListItem that formats operatingSystem.FriendlyName).tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs (2)
140-160: Consider also testing NaN in XValues and in Curve2.The NaN test only injects
float.NaNintoCurve1.YValues. If theSvgChartGenerator.calculateBoundslogic handles X and Y differently, NaN inXValuesor inCurve2could still produce"NaN"in the SVG output. Worth adding a case to increase confidence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs` around lines 140 - 160, Add tests that mirror When_generating_chart_with_nan_values but inject float.NaN into other data arrays: create one spec that sets _chartData.Curve1.XValues to include NaN and another that sets _chartData.Curve2.YValues (and optionally Curve2.XValues) to include NaN, then call sut.GenerateLineChart(_chartData) and assert the returned SVG contains "<svg"/"</svg>" and does not contain "NaN". This will exercise SvgChartGenerator.calculateBounds and GenerateLineChart for NaNs in XValues and in Curve2 as well.
162-182: Consider asserting negative values on log scale as well.Zero values are tested, but negative Y-values on a log scale are equally problematic (log of a negative is undefined). A similar test with negative values would strengthen coverage for the
calculateBoundssafeguards described in the relevant snippet fromSvgChartGenerator.cs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs` around lines 162 - 182, Add a sibling test to When_generating_log_scale_chart_with_zero_values that verifies negative Y-values are handled on log scale: reuse createTestChartData(useLogScale: true), set _chartData.Curve1.YValues to include a negative value (e.g. -1f) and call sut.GenerateLineChart(_chartData), then assert the output SVG is still produced (contains "<svg" and "</svg>"). This mirrors the zero-value case and validates the calculateBounds logic in SvgChartGenerator (ensure test references When_generating_log_scale_chart_with_zero_values, createTestChartData, _chartData.Curve1.YValues, and sut.GenerateLineChart).src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs (1)
9-9: Consider materializing_buildersto avoid multiple enumeration.
_buildersis iterated twice perfindBuilderForcall (twoFirstOrDefaultpasses). If the injectedIEnumerableis lazy, this could cause repeated resolution or unexpected behavior. Materializing to aListin the constructor is a simple safeguard.Proposed fix
- private readonly IEnumerable<IMarkdownBuilder> _builders; + private readonly List<IMarkdownBuilder> _builders; public MarkdownBuilderRepository(IEnumerable<IMarkdownBuilder> builders) { - _builders = builders; + _builders = builders.ToList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs` at line 9, The field _builders in MarkdownBuilderRepository is an IEnumerable that get enumerated multiple times in findBuilderFor; materialize it to avoid repeated enumeration by changing the backing field to a concrete collection (e.g., List<IMarkdownBuilder>) and assigning _builders = builders.ToList() in the MarkdownBuilderRepository constructor (ensure System.Linq is available), so findBuilderFor and any other callers iterate the materialized list instead of re-enumerating a lazy sequence.tests/InstallationValidator.Tests/Services/ValidationReportingTaskSpecs.cs (1)
42-42: Consider usingGetAwaiter().GetResult()instead of.Wait()for synchronous blocking.
.Wait()wraps exceptions inAggregateException, which can obscure the real failure in test diagnostics..GetAwaiter().GetResult()propagates the original exception directly. This applies to lines 42, 70, 98, and 122.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Services/ValidationReportingTaskSpecs.cs` at line 42, Replace the synchronous blocking calls that use .Wait() on the CreateReport task with .GetAwaiter().GetResult() to avoid AggregateException wrapping; specifically update the calls to sut.CreateReport(_installationValidationResult, _outputFolder).Wait() (and the other occurrences at the similar test calls around the same spec: the CreateReport invocations near lines 70, 98, and 122) to use sut.CreateReport(...).GetAwaiter().GetResult() so the original exceptions propagate directly in the test runner.src/InstallationValidator.Core/Services/PdfReportingTask.cs (1)
35-50:Task.Runwrapping vs. async I/O — inconsistency withMarkdownReportingTask.
PdfReportingTaskwraps synchronousGeneratePdfinTask.Run, whileMarkdownReportingTaskusesasync/awaitwithStreamWriter.WriteAsync. Both approaches work, but the mixed patterns in sibling classes can confuse maintainers. IfGeneratePdfis inherently synchronous, theTask.Runwrapper is fine — just worth a brief comment explaining the difference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs` around lines 35 - 50, CreateReport in PdfReportingTask currently offloads the synchronous PdfReportDocument.GeneratePdf call to Task.Run, while MarkdownReportingTask uses async I/O (StreamWriter.WriteAsync), causing mixed patterns across reporters; either document why Task.Run is used or make the PDF path consistent with async style. Fix by adding a brief comment in PdfReportingTask.CreateReport explaining that GeneratePdf is synchronous and therefore intentionally wrapped in Task.Run (referencing PdfReportingTask, CreateReport, PdfReportDocument.GeneratePdf) or, if GeneratePdf can be made async, convert it to an async method and await it here to mirror MarkdownReportingTask (which uses StreamWriter.WriteAsync) so both reporters follow the same async I/O pattern.src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs (1)
39-62:createChartDatais duplicated verbatim inPdfReportDocument.cs(lines 291-314).Both builders assemble identical
ChartDatafrom anOutputComparisonResultwith the same colors, labels, and structure. The chart-rendering guard logic (log chart always, linear chart when invalid) is also duplicated. Consider extracting a shared static helper (e.g.,ChartDataFactory.FromOutputComparison(output, useLogScale)) to keep this in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs` around lines 39 - 62, The createChartData duplication should be collapsed into a single shared factory: add a static helper method (e.g., ChartDataFactory.FromOutputComparison(OutputComparisonResult output, bool useLogScale)) that builds and returns the ChartData using the same Title, XAxisLabel/YAxisLabel, UseLogScale, Curve1 and Curve2 with Color.CornflowerBlue and Color.OrangeRed; then replace the createChartData implementations in OutputComparisonResultMarkdownBuilder.createChartData and the identical method in PdfReportDocument with calls to ChartDataFactory.FromOutputComparison(output, useLogScale) and remove the duplicated code, keeping any surrounding chart-rendering guard logic in the callers.src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs (1)
110-149: Inconsistent time formatting between PDF and Markdown reports.The PDF formatter uses
.ToIsoFormat()and.ToDisplay()(OSPSuite.Core extension methods), while the Markdown formatter usesToString("yyyy-MM-dd HH:mm:ss")and a customformatTimeSpan()method that produces human-readable durations (e.g., "2h 30min 15s", "45min 20s", "5s"). This results in different timestamp and duration representations across the two output formats. If intentional, document this decision; otherwise, consolidate to use a single formatting approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs` around lines 110 - 149, ComposeRunSummary currently formats timestamps/durations using summary.StartTime.ToIsoFormat()/ToIsoFormat() and ToDisplay(), which differs from the Markdown formatter that uses ToString("yyyy-MM-dd HH:mm:ss") and formatTimeSpan(). Update PdfReportDocument.ComposeRunSummary to use the same timestamp and duration formatting as the Markdown path (i.e., replace ToIsoFormat()/ToDisplay() usages with the shared formatting used by the Markdown reporter or delegate to a common helper like formatTimeSpan/standard timestamp formatter) so both PDF and Markdown produce identical date/time and duration strings; reference ComposeRunSummary and the extension methods ToIsoFormat/ToDisplay and the Markdown formatter’s ToString("yyyy-MM-dd HH:mm:ss")/formatTimeSpan when making the change.tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs (4)
17-17: Namespace does not reflect test project conventions.
InstallationValidator.Reportingis used, but the file lives undertests/InstallationValidator.Tests/Reporting/. The typical convention (matching the project assembly) would beInstallationValidator.Tests.Reporting.♻️ Proposed fix
-namespace InstallationValidator.Reporting +namespace InstallationValidator.Tests.Reporting🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs` at line 17, The file's namespace is declared as InstallationValidator.Reporting but it lives in the test assembly; update the namespace declaration to InstallationValidator.Tests.Reporting to match the test project conventions and assembly name (locate the namespace line in MarkdownReportingSpecs.cs and replace InstallationValidator.Reporting with InstallationValidator.Tests.Reporting).
196-202:_svgChartGeneratoris stored but never used insideTestMarkdownBuilderRepository.The field is accepted in the constructor and assigned, but no method in this class references it. It exists only in the outer
Context()wiring (line 51). Either remove it from the constructor/field, or document why it is retained.♻️ Proposed fix
- private readonly ISvgChartGenerator _svgChartGenerator; - public TestMarkdownBuilderRepository(IMarkdownBuilder[] builders, ISvgChartGenerator svgChartGenerator) { _builders = builders; - _svgChartGenerator = svgChartGenerator; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs` around lines 196 - 202, The TestMarkdownBuilderRepository stores a private field _svgChartGenerator and accepts an ISvgChartGenerator in its constructor but never uses it; remove the unused private field _svgChartGenerator and the corresponding ISvgChartGenerator parameter from the TestMarkdownBuilderRepository constructor (and any assignment) or, if the generator is intended for future use, add a comment explaining why it is retained; update any calling/context code that constructs TestMarkdownBuilderRepository to stop passing an ISvgChartGenerator when you remove the parameter.
100-100: Preferasync/awaitover.Wait()for async tasks in NUnit tests.
.Wait()wraps exceptions inAggregateException, which obscures test failure messages. NUnit fully supportsasync Taskoverrides.♻️ Proposed refactor
- protected override void Because() + protected override async Task Because() { - sut.CreateReport(_installationValidationResult, _reportsDir.FullName, false).Wait(); + await sut.CreateReport(_installationValidationResult, _reportsDir.FullName, false); _reportPath = Directory.GetFiles(_reportsDir.FullName, "*.md").FirstOrDefault(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs` at line 100, The test is calling sut.CreateReport(...).Wait(), which causes AggregateException wrapping and obscures failures; change the test method signature to an async Task (e.g., make the test method async Task Should_create_report()) and replace the .Wait() call with await sut.CreateReport(_installationValidationResult, _reportsDir.FullName, false); so the async CreateReport is awaited directly and NUnit can surface exceptions properly.
113-116: Inconsistent assertion style —StringAssert.Containsvs BDDHelper extensions.The rest of the observations use
ShouldNotBeNull()/ShouldBeTrue()fromOSPSuite.BDDHelper.Extensions. Prefer the same style here for uniformity.♻️ Proposed refactor
- StringAssert.Contains("Installation Validation", content); + content.ShouldContain("Installation Validation");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs` around lines 113 - 116, The test uses MSTest's StringAssert.Contains instead of the project's BDDHelper extension methods; replace StringAssert.Contains("Installation Validation", content) with the BDD helper style (call the extension on the content string, e.g. content.ShouldContain("Installation Validation")) and ensure the OSPSuite.BDDHelper.Extensions namespace is imported so the extension method is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/InstallationValidator.Core/InstallationValidator.Core.csproj`:
- Line 36: The PR added a QuestPDF package but omitted required startup license
configuration and may be using an outdated pinned version; confirm your org
qualifies for the Community license (or procure a paid license), then set the
license once at app startup by adding QuestPDF.Settings.License =
LicenseType.Community (or LicenseType.Professional/Enterprise) in your
application entry point (e.g., Program.cs Main/Host building code) before any
document creation, and consider updating the PackageReference version from
2024.3.0 to the current NuGet release.
In `@src/InstallationValidator.Core/Reporting/Charts/ChartData.cs`:
- Around line 5-13: ChartData exposes Curve1 and Curve2 but SvgChartGenerator
dereferences them without null checks; update SvgChartGenerator.calculateBounds
and SvgChartGenerator.appendLegend to guard access to ChartData.Curve1 and
ChartData.Curve2 (e.g., check for null before reading properties or skip/handle
missing curves) so you never access members on a null CurveData instance, and
adjust any looping/legend logic to gracefully handle one or both curves being
absent.
In `@src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs`:
- Around line 168-182: The appendLegend method can throw NullReferenceException
because chartData.Curve1 or ChartData.Curve2 may be null; update appendLegend to
null-check chartData.Curve1 and chartData.Curve2 before accessing their
properties (Curve1.Color, Curve1.Name, Curve2.Color, Curve2.Name) and only
render the corresponding line/text when the curve exists; for missing
colors/names either skip that legend row or supply a sensible default (e.g., a
default color via colorToHex fallback or "N/A" after escapeXml/truncate) and
keep using the existing helpers colorToHex, escapeXml, and truncate to format
values safely.
In
`@src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs`:
- Around line 27-33: The collection overload Report(IEnumerable<object>
objectsToReport, MarkdownReportContext context) lacks a null guard and will
throw if objectsToReport is null; add a null check at the start of that method
(mirroring the single-object Report(object obj, MarkdownReportContext context))
and return early when objectsToReport is null (or alternatively throw a clear
ArgumentNullException) before the foreach so the method handles null input
safely.
In `@src/InstallationValidator.Core/Reporting/Markdown/MarkdownReportContext.cs`:
- Around line 72-82: AppendTable builds Markdown tables but doesn't escape pipe
characters in headers or rows, breaking tables when cell values contain '|';
update AppendTable (and/or add a small helper like EscapeCell) to replace '|'
with '\|' (and handle nulls) for every header and each row cell before joining,
then use the escaped strings in the existing string.Join calls so generated
Markdown remains valid.
In
`@src/InstallationValidator.Core/Reporting/Markdown/OutputFileComparisonResultMarkdownBuilder.cs`:
- Around line 30-33: The code calls
fileComparisonResult.TimeComparison.IsValid() without ensuring TimeComparison is
not null; update the condition to first check TimeComparison for null before
calling IsValid (e.g., ensure fileComparisonResult.TimeComparison != null &&
!fileComparisonResult.TimeComparison.IsValid()), then call
_builderRepository.Report(fileComparisonResult.TimeComparison, context) only
when TimeComparison is non-null and invalid; reference TimeComparison,
IsValid(), fileComparisonResult and _builderRepository.Report to locate the
place to change.
In
`@src/InstallationValidator.Core/Reporting/Markdown/ValidationRunSummaryMarkdownBuilder.cs`:
- Around line 40-47: The durationFor method builds a paragraph using
Assets.Reporting.InstallationValidationPerformedIn which inserts
Environment.NewLine, but Markdown collapses single newlines; update the builder
to normalize those newlines into Markdown line breaks (either replace
Environment.NewLine with " \n" or "<br>") before passing strings to
InstallationValidationPerformedIn, and apply the same normalization to the
applicationVersions construction (or add a helper on MarkdownReportContext to
render multi-line paragraphs properly) so the start/end/duration and version
lines render with visible line breaks.
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 65-71: The writeReportAsync method currently opens a StreamWriter
for reportPath without ensuring the target directory exists, which can throw
DirectoryNotFoundException; before creating the StreamWriter in writeReportAsync
call Directory.CreateDirectory on Path.GetDirectoryName(reportPath) (handle
null/empty safely) to ensure the folder exists, and apply the same guard in
PdfReportingTask.GeneratePdf where a PDF file is written so the output directory
is created before writing.
- Around line 45-55: CreateReport currently dereferences
installationValidationResult.RunSummary.StartTime without checking for null;
update CreateReport in MarkdownReportingTask (and the analogous method in
PdfReportingTask) to validate that installationValidationResult and
installationValidationResult.RunSummary are not null before calling
reportOutputPath, and handle the null case (throw an ArgumentNullException with
a clear message or use a sensible default StartTime) so you never access
StartTime on a null RunSummary; ensure you reference the CreateReport method and
the reportOutputPath call when adding the null check and consistent error
handling.
In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs`:
- Around line 77-80: The reportOutputPath method in PdfReportingTask currently
hardcodes Assets.Reporting.InstallationValidation into the filename; change its
signature (and the analogous method in MarkdownReportingTask) to accept a report
label/string (e.g., subtitle or label) and use that value instead of the
hardcoded InstallationValidation; update callers (including the
BatchComparisonResult invocation that currently calls reportOutputPath from line
45) to pass the correct Assets.Reporting.FolderComparison for folder-comparison
reports (and Assets.Reporting.InstallationValidation for installation reports),
or pass the subtitle already supplied to PdfReportDocument so filenames reflect
the actual report type.
In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs`:
- Around line 46-52: The builders array currently includes
OperatingSystemInfoMarkdownBuilder and TimeComparisonResultMarkdownBuilder but
the TestMarkdownBuilderRepository.Report switch (in MarkdownReportingSpecs
tests) lacks cases for the corresponding ReportType values, so those builders
are never invoked; either remove these two builders from the builders array
passed into TestMarkdownBuilderRepository if you don't intend to test them, or
add switch cases for ReportType.OperatingSystemInfo and
ReportType.TimeComparisonResult that dispatch to the appropriate builder
implementations so OperatingSystemInfoMarkdownBuilder and
TimeComparisonResultMarkdownBuilder are exercised (refer to the builders array,
TestMarkdownBuilderRepository, and the Report switch to implement the fix).
- Line 23: The field `_comparisonSettings` is declared static but is assigned in
the instance method `Context()` and read in the instance method
`createOutputComparisonResult()`, breaking test isolation; change the
declaration of `_comparisonSettings` to an instance field (remove the `static`
modifier) so each test instance has its own ComparisonSettings, and update any
references if needed to use the instance field rather than a static reference
(ensure `Context()` and `createOutputComparisonResult()` continue to access
`_comparisonSettings` as an instance member).
- Around line 29-34: The test setup creates _reportsDir in Context() but never
cleans it, causing stale .md files to make should_create_markdown_file pass; add
a TearDown (or override CleanUp/Because) that deletes all "*.md" files (or
empties the directory) before/after each test so CreateReport is proven to have
created the file; modify the test class to implement TearDown/CleanUp and use
_reportsDir.FullName with Directory.GetFiles(..., "*.md") and File.Delete for
each result, ensuring the cleanup runs even if tests fail.
---
Duplicate comments:
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 81-84: reportOutputPath currently hardcodes
Assets.Reporting.InstallationValidation into the filename causing
BatchComparisonResult outputs to be misnamed; change reportOutputPath to accept
a report-specific suffix/label (e.g., string reportName or enum) instead of
always using Assets.Reporting.InstallationValidation, update callers that
produce BatchComparisonResult to pass the correct Assets.Reporting value (match
how PdfReportingTask was fixed), and ensure the MarkdownReportingTask and any
callers use the new signature so filenames reflect the actual report type.
---
Nitpick comments:
In `@src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs`:
- Around line 202-205: The colorToHex method is duplicated
(SvgChartGenerator.colorToHex and MarkdownReportContext.AppendColoredStatus);
create a single shared static extension method like ColorExtensions.ToHex(this
Color c) that returns $"#{c.R:X2}{c.G:X2}{c.B:X2}", replace calls to colorToHex
in SvgChartGenerator and the logic inside
MarkdownReportContext.AppendColoredStatus to use ColorExtensions.ToHex, and
remove the duplicate colorToHex implementation to prevent divergence.
In `@src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs`:
- Around line 21-24: The current Build(object objectToReport,
MarkdownReportContext context) uses a hard cast ((T)objectToReport) which
produces an opaque InvalidCastException; change it to use an 'as' cast to T (or
a safe pattern match), check for null, and throw a descriptive ArgumentException
including the expected type (typeof(T)) and the actual object type
(objectToReport?.GetType()) before calling Build(T, context) so callers get a
clear error message; update the Build(object...) implementation in
IMarkdownBuilder (the Build(object objectToReport, MarkdownReportContext
context) method) accordingly.
In
`@src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs`:
- Line 9: The field _builders in MarkdownBuilderRepository is an IEnumerable
that get enumerated multiple times in findBuilderFor; materialize it to avoid
repeated enumeration by changing the backing field to a concrete collection
(e.g., List<IMarkdownBuilder>) and assigning _builders = builders.ToList() in
the MarkdownBuilderRepository constructor (ensure System.Linq is available), so
findBuilderFor and any other callers iterate the materialized list instead of
re-enumerating a lazy sequence.
In
`@src/InstallationValidator.Core/Reporting/Markdown/OperatingSystemInfoMarkdownBuilder.cs`:
- Around line 7-17: In OperatingSystemInfoMarkdownBuilder.Build, the second list
item uses a hardcoded "OS" label; change that string to use the appropriate
asset constant (e.g., Assets.Reporting.OperatingSystem or another shorter asset)
so the line that appends operatingSystem.FriendlyName uses the same
Assets.Reporting.* pattern as the other list items; update the call in Build to
reference the constant (locate the method Build in class
OperatingSystemInfoMarkdownBuilder and replace the hardcoded label in the
context.AppendListItem that formats operatingSystem.FriendlyName).
In
`@src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs`:
- Around line 39-62: The createChartData duplication should be collapsed into a
single shared factory: add a static helper method (e.g.,
ChartDataFactory.FromOutputComparison(OutputComparisonResult output, bool
useLogScale)) that builds and returns the ChartData using the same Title,
XAxisLabel/YAxisLabel, UseLogScale, Curve1 and Curve2 with Color.CornflowerBlue
and Color.OrangeRed; then replace the createChartData implementations in
OutputComparisonResultMarkdownBuilder.createChartData and the identical method
in PdfReportDocument with calls to ChartDataFactory.FromOutputComparison(output,
useLogScale) and remove the duplicated code, keeping any surrounding
chart-rendering guard logic in the callers.
In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs`:
- Around line 110-149: ComposeRunSummary currently formats timestamps/durations
using summary.StartTime.ToIsoFormat()/ToIsoFormat() and ToDisplay(), which
differs from the Markdown formatter that uses ToString("yyyy-MM-dd HH:mm:ss")
and formatTimeSpan(). Update PdfReportDocument.ComposeRunSummary to use the same
timestamp and duration formatting as the Markdown path (i.e., replace
ToIsoFormat()/ToDisplay() usages with the shared formatting used by the Markdown
reporter or delegate to a common helper like formatTimeSpan/standard timestamp
formatter) so both PDF and Markdown produce identical date/time and duration
strings; reference ComposeRunSummary and the extension methods
ToIsoFormat/ToDisplay and the Markdown formatter’s ToString("yyyy-MM-dd
HH:mm:ss")/formatTimeSpan when making the change.
In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs`:
- Around line 35-50: CreateReport in PdfReportingTask currently offloads the
synchronous PdfReportDocument.GeneratePdf call to Task.Run, while
MarkdownReportingTask uses async I/O (StreamWriter.WriteAsync), causing mixed
patterns across reporters; either document why Task.Run is used or make the PDF
path consistent with async style. Fix by adding a brief comment in
PdfReportingTask.CreateReport explaining that GeneratePdf is synchronous and
therefore intentionally wrapped in Task.Run (referencing PdfReportingTask,
CreateReport, PdfReportDocument.GeneratePdf) or, if GeneratePdf can be made
async, convert it to an async method and await it here to mirror
MarkdownReportingTask (which uses StreamWriter.WriteAsync) so both reporters
follow the same async I/O pattern.
In `@src/InstallationValidator/InstallationValidator.csproj`:
- Line 41: The closing ItemGroup tag is misindented (4 spaces) compared to its
opening tag (2 spaces); edit the csproj so the `</ItemGroup>` closing tag uses
the same 2-space indentation as the corresponding `<ItemGroup>` tag to keep XML
formatting consistent.
In `@src/SimulationOutputComparer/SimulationOutputComparer.csproj`:
- Line 41: Adjust the inconsistent indentation of the closing XML tag so it
matches the opening <ItemGroup> indentation: change the closing </ItemGroup> tag
indentation from 4 spaces to 2 spaces to align with the opening <ItemGroup>
(ensure indentation consistency within SimulationOutputComparer.csproj).
In `@tests/InstallationValidator.Tests/InstallationValidator.Tests.csproj`:
- Line 31: The closing XML tag </ItemGroup> in the project file is misaligned;
locate the corresponding opening <ItemGroup> block in
InstallationValidator.Tests (look for the matching <ItemGroup> that contains
PackageReference or ProjectReference entries) and re-indent the </ItemGroup> to
align with its opening tag so the XML block is properly formatted and
well-formed; ensure there are no extra or missing ItemGroup tags and run an
XML/project build to verify correctness.
In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs`:
- Line 17: The file's namespace is declared as InstallationValidator.Reporting
but it lives in the test assembly; update the namespace declaration to
InstallationValidator.Tests.Reporting to match the test project conventions and
assembly name (locate the namespace line in MarkdownReportingSpecs.cs and
replace InstallationValidator.Reporting with
InstallationValidator.Tests.Reporting).
- Around line 196-202: The TestMarkdownBuilderRepository stores a private field
_svgChartGenerator and accepts an ISvgChartGenerator in its constructor but
never uses it; remove the unused private field _svgChartGenerator and the
corresponding ISvgChartGenerator parameter from the
TestMarkdownBuilderRepository constructor (and any assignment) or, if the
generator is intended for future use, add a comment explaining why it is
retained; update any calling/context code that constructs
TestMarkdownBuilderRepository to stop passing an ISvgChartGenerator when you
remove the parameter.
- Line 100: The test is calling sut.CreateReport(...).Wait(), which causes
AggregateException wrapping and obscures failures; change the test method
signature to an async Task (e.g., make the test method async Task
Should_create_report()) and replace the .Wait() call with await
sut.CreateReport(_installationValidationResult, _reportsDir.FullName, false); so
the async CreateReport is awaited directly and NUnit can surface exceptions
properly.
- Around line 113-116: The test uses MSTest's StringAssert.Contains instead of
the project's BDDHelper extension methods; replace
StringAssert.Contains("Installation Validation", content) with the BDD helper
style (call the extension on the content string, e.g.
content.ShouldContain("Installation Validation")) and ensure the
OSPSuite.BDDHelper.Extensions namespace is imported so the extension method is
available.
In `@tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs`:
- Around line 140-160: Add tests that mirror
When_generating_chart_with_nan_values but inject float.NaN into other data
arrays: create one spec that sets _chartData.Curve1.XValues to include NaN and
another that sets _chartData.Curve2.YValues (and optionally Curve2.XValues) to
include NaN, then call sut.GenerateLineChart(_chartData) and assert the returned
SVG contains "<svg"/"</svg>" and does not contain "NaN". This will exercise
SvgChartGenerator.calculateBounds and GenerateLineChart for NaNs in XValues and
in Curve2 as well.
- Around line 162-182: Add a sibling test to
When_generating_log_scale_chart_with_zero_values that verifies negative Y-values
are handled on log scale: reuse createTestChartData(useLogScale: true), set
_chartData.Curve1.YValues to include a negative value (e.g. -1f) and call
sut.GenerateLineChart(_chartData), then assert the output SVG is still produced
(contains "<svg" and "</svg>"). This mirrors the zero-value case and validates
the calculateBounds logic in SvgChartGenerator (ensure test references
When_generating_log_scale_chart_with_zero_values, createTestChartData,
_chartData.Curve1.YValues, and sut.GenerateLineChart).
In `@tests/InstallationValidator.Tests/Services/ValidationReportingTaskSpecs.cs`:
- Line 42: Replace the synchronous blocking calls that use .Wait() on the
CreateReport task with .GetAwaiter().GetResult() to avoid AggregateException
wrapping; specifically update the calls to
sut.CreateReport(_installationValidationResult, _outputFolder).Wait() (and the
other occurrences at the similar test calls around the same spec: the
CreateReport invocations near lines 70, 98, and 122) to use
sut.CreateReport(...).GetAwaiter().GetResult() so the original exceptions
propagate directly in the test runner.
Replace TestMarkdownBuilderRepository with the real MarkdownBuilderRepository and all production builders. This improves test coverage by exercising the actual builder implementations rather than duplicating their logic. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create MissingFileComparisonResultMarkdownBuilder to properly report missing file details including which folder contains the file and which folder is missing it - Add ComposeMissingFileResult in PdfReportDocument to display the missing file validation message in PDF reports - Register the new builder in ValidatorRegister - Add builder to test setup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…ring - Escape pipe characters in MarkdownReportContext.AppendTable to prevent broken markdown tables when cell values contain '|' - Add null validation for installationValidationResult and RunSummary in both MarkdownReportingTask and PdfReportingTask before accessing StartTime - Refactor MarkdownReportingSpecs to use ContextForIntegration, resolving IMarkdownReportingTask from the DI container instead of manually constructing all builders Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs (1)
116-183: Consider adding a test for nullCurve1/Curve2.The previous review round identified a
NullReferenceExceptionrisk when curves are null, which was fixed inSvgChartGenerator. There's no test to guard against regression. A spec likeWhen_generating_chart_with_null_curvesthat sets one or both curves tonulland asserts valid SVG output would lock in the fix.💚 Example test
public class When_generating_chart_with_null_curves : concern_for_SvgChartGenerator { protected override void Context() { base.Context(); _chartData.Curve1 = null; _chartData.Curve2 = null; } protected override void Because() { _result = sut.GenerateLineChart(_chartData); } [Observation] public void should_return_valid_svg_without_errors() { StringAssert.Contains("<svg", _result); StringAssert.Contains("</svg>", _result); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs` around lines 116 - 183, Add a new test class When_generating_chart_with_null_curves (in the same fixture set using concern_for_SvgChartGenerator) that in Context sets _chartData.Curve1 = null and _chartData.Curve2 = null, calls sut.GenerateLineChart(_chartData) in Because, and asserts the returned string contains "<svg" and "</svg>" in the observation; this ensures SvgChartGenerator.GenerateLineChart gracefully handles null Curve1/Curve2 and prevents regressions previously causing a NullReferenceException.src/InstallationValidator.Core/Reporting/Charts/ChartData.cs (1)
5-13: DuplicatedcreateChartDatafactory method across consumers.The relevant snippets show identical
createChartData(OutputComparisonResult, bool)implementations in bothOutputComparisonResultMarkdownBuilder.cs(lines 38–61) andPdfReportDocument.cs(lines 303–326). Consider extracting a shared factory (e.g., a static method onChartDataor a small helper) to avoid maintaining the same mapping logic in two places.♻️ Example: static factory on ChartData
public class ChartData { + public static ChartData FromOutputComparison(OutputComparisonResult output, bool useLogScale) => + new ChartData + { + Title = output.Path, + XAxisLabel = $"Time [{output.TimeDisplayUnit}]", + YAxisLabel = $"[{output.ValuesDisplayUnit}]", + UseLogScale = useLogScale, + Curve1 = new CurveData + { + Name = output.Output1.Caption, + XValues = output.Output1.Times, + YValues = output.Output1.Values, + Color = Color.CornflowerBlue + }, + Curve2 = new CurveData + { + Name = output.Output2.Caption, + XValues = output.Output2.Times, + YValues = output.Output2.Values, + Color = Color.OrangeRed + } + }; + public string Title { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Charts/ChartData.cs` around lines 5 - 13, Both consumers have identical createChartData(OutputComparisonResult, bool) logic; move that mapping into a single shared factory to avoid duplication by adding a static factory method on ChartData (e.g., ChartData.CreateFrom(OutputComparisonResult result, bool useLogScale)) that populates Title, XAxisLabel, YAxisLabel, UseLogScale and builds Curve1/Curve2 (CurveData) from the OutputComparisonResult. Replace the duplicated implementations in OutputComparisonResultMarkdownBuilder and PdfReportDocument to call ChartData.CreateFrom(result, useLogScale) (or a small internal helper with the same signature) so all mapping logic lives in one place.src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs (1)
304-327:createChartDatais duplicated verbatim inOutputComparisonResultMarkdownBuilder.cs.The private helper in this file is identical to the one visible in
src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs. Extract it to a shared static factory (e.g.,ChartDataFactory.FromOutputComparison) or an extension method onOutputComparisonResultto avoid the drift risk.♻️ Proposed extraction
// New file: src/InstallationValidator.Core/Reporting/Charts/ChartDataFactory.cs +using System.Drawing; +using InstallationValidator.Core.Domain; + +namespace InstallationValidator.Core.Reporting.Charts +{ + public static class ChartDataFactory + { + public static ChartData FromOutputComparison(OutputComparisonResult output, bool useLogScale) + { + return new ChartData + { + Title = output.Path, + XAxisLabel = $"Time [{output.TimeDisplayUnit}]", + YAxisLabel = $"[{output.ValuesDisplayUnit}]", + UseLogScale = useLogScale, + Curve1 = new CurveData + { + Name = output.Output1.Caption, + XValues = output.Output1.Times, + YValues = output.Output1.Values, + Color = Color.CornflowerBlue + }, + Curve2 = new CurveData + { + Name = output.Output2.Caption, + XValues = output.Output2.Times, + YValues = output.Output2.Values, + Color = Color.OrangeRed + } + }; + } + } +}Then replace both usages:
-private ChartData createChartData(OutputComparisonResult output, bool useLogScale) -{ - return new ChartData { ... }; -}and call
ChartDataFactory.FromOutputComparison(output, useLogScale)in both classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs` around lines 304 - 327, The createChartData method is duplicated; extract it into a shared static factory method e.g., add ChartDataFactory.FromOutputComparison(OutputComparisonResult output, bool useLogScale) that returns the ChartData built from OutputComparisonResult (using Title, XAxisLabel, YAxisLabel, UseLogScale, Curve1 and Curve2 with the same mappings and colors), then replace the private createChartData usages in PdfReportDocument.cs (createChartData) and OutputComparisonResultMarkdownBuilder.cs with calls to ChartDataFactory.FromOutputComparison(output, useLogScale); ensure the new factory references the ChartData and CurveData types and preserves the existing field mappings (Output1/Output2.Caption, Times, Values and color choices).src/InstallationValidator.Core/Reporting/Markdown/TimeComparisonResultMarkdownBuilder.cs (1)
1-16: LGTM — consider injectingDoubleFormatterif it's registered in DI.
new DoubleFormatter()is also instantiated directly inPdfReportDocument.cs(line 24). IfDoubleFormatteris registered as a singleton, prefer injection over construction for testability and consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/TimeComparisonResultMarkdownBuilder.cs` around lines 1 - 16, TimeComparisonResultMarkdownBuilder currently constructs DoubleFormatter with new DoubleFormatter(), and PdfReportDocument also new's it elsewhere; change both to accept a DoubleFormatter via constructor injection (add a readonly field on TimeComparisonResultMarkdownBuilder and assign it in the constructor, and update PdfReportDocument to receive and use the injected DoubleFormatter) so the formatter is provided by DI for consistency and testability; update any DI registration to register DoubleFormatter as the intended lifetime (e.g., singleton) and ensure classes requesting it are updated to accept the dependency.src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs (1)
10-15: Consider materializing_buildersin the constructor.
findBuilderFor(lines 33–37) runs twoFirstOrDefaultpasses over_builderson everyReportcall. If the injectedIEnumerable<IMarkdownBuilder>is a deferred/lazy sequence from the DI container, it will be re-evaluated on each call. Materializing once in the constructor is a cheap safeguard:♻️ Proposed change
-private readonly IEnumerable<IMarkdownBuilder> _builders; +private readonly IReadOnlyList<IMarkdownBuilder> _builders; public MarkdownBuilderRepository(IEnumerable<IMarkdownBuilder> builders) { - _builders = builders; + _builders = builders?.ToList() ?? new List<IMarkdownBuilder>(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs` around lines 10 - 15, The injected IEnumerable<IMarkdownBuilder> _builders in MarkdownBuilderRepository is stored as-is causing repeated enumeration (and possible re-evaluation) each time findBuilderFor or Report runs; materialize it once in the MarkdownBuilderRepository constructor (e.g., assign _builders = builders?.ToList() ?? Enumerable.Empty<IMarkdownBuilder>()) so subsequent calls to findBuilderFor and Report use the cached collection and avoid double FirstOrDefault enumeration and deferred execution costs.src/InstallationValidator.Core/Services/ValidationReportingTask.cs (1)
6-10:DefaultFormatis not exposed onIValidationReportingTask; callers bound to the interface cannot switch format.Any consumer that holds the interface (which is the typical DI injection point) will always get Markdown output. Exposing the property on the interface—or accepting the format as a constructor/method argument—would allow callers to configure the output format without depending on the concrete type.
♻️ Suggested interface extension
public interface IValidationReportingTask { + ReportFormat DefaultFormat { get; set; } Task CreateReport(InstallationValidationResult installationValidationResult, string outputFolderPath, bool openReport = false); Task CreateReport(BatchComparisonResult comparisonResult, string firstFolderPath, string secondFolderPath, bool openReport); }Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Services/ValidationReportingTask.cs` around lines 6 - 10, The interface IValidationReportingTask currently forces a default format (Markdown) on consumers; update the interface to expose the DefaultFormat property or accept a format parameter so callers can choose output format: add a read-only property like DefaultFormat (or overload the CreateReport methods to include a parameter e.g., ReportFormat format) to the IValidationReportingTask contract and update the implementations (and any callers bound to IValidationReportingTask) to honor that property/parameter when producing reports in the CreateReport(InstallationValidationResult...) and CreateReport(BatchComparisonResult...) flows so DI-bound consumers can switch formats without depending on the concrete type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs`:
- Line 42: The inline style on the SVG (built in SvgChartGenerator via the
sb.AppendLine call that uses Width and Height) will be stripped by GitHub;
remove the inline style and make the SVG responsive using attributes that GitHub
preserves (e.g., set width="100%" height="auto" and keep viewBox="{Width}
{Height}" and/or add preserveAspectRatio) or alternatively change the generator
to emit the SVG to an external .svg file and reference it with an <img> tag so
styling is preserved; update the sb.AppendLine invocation accordingly (or add a
new method that writes an external file) to implement one of these approaches.
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 85-88: The filename generator reportOutputPath currently hardcodes
Assets.Reporting.InstallationValidation; update reportOutputPath (or add an
overload) to accept a report type/name parameter and use that value instead of
the fixed InstallationValidation so filenames reflect the actual report; then
change CreateReport(BatchComparisonResult...) to call
createReportContext(Assets.Reporting.FolderComparison) and pass
Assets.Reporting.FolderComparison (or the resolved display name) into
reportOutputPath so the output filename matches the report heading.
In `@src/SimulationOutputComparer/SimulationOutputComparer.csproj`:
- Around line 4-6: Project targets net8.0-windows but OSPSuite.DevExpress
v21.2.15 only supports up to .NET 6; either downgrade TargetFramework to
net6.0-windows or upgrade the DevExpress package to a compatible version
(>=23.1, preferably 24.2+) so OSPSuite.DevExpress works with .NET 8; also remove
the redundant <EnableWindowsTargeting>true</EnableWindowsTargeting> entry (or
make it conditional for non-Windows CI) because TargetFramework="netX.Y-windows"
already enables the Windows surface.
In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs`:
- Around line 133-138: The observation method should first assert that the
report path is not null to avoid an ArgumentNullException from File.ReadAllText;
add an early guard in should_contain_report_content that checks _reportPath
(e.g., Assert.IsNotNull(_reportPath, "Expected report file to be created by
CreateReport") or Assert.Fail with a clear message) before calling
File.ReadAllText, so failures remain meaningful and consistent with
should_create_markdown_file.
---
Duplicate comments:
In `@src/InstallationValidator.Core/InstallationValidator.Core.csproj`:
- Line 36: Update the QuestPDF dependency and ensure runtime license
initialization: change the PackageReference for QuestPDF to the intended
supported version (e.g., 2026.2.1) and add startup code to set
QuestPDF.Settings.License with the appropriate license key before any PDF
generation occurs (reference QuestPDF.Settings.License and where your app
initializes services/startup logic, e.g., Program/Main or DI startup class).
Ensure the version bump is tested for any breaking changes and remove or
document the old pinned version 2024.3.0 in the project file.
In `@src/InstallationValidator.Core/Reporting/Markdown/MarkdownReportContext.cs`:
- Around line 72-82: The AppendTable method in MarkdownReportContext is not
escaping pipe characters so any '|' in headers or row cell values breaks the
Markdown; update AppendTable to escape pipe characters (e.g., replace '|' with
'\|' or use the HTML entity |) for every string in headers and for every
cell in rows before joining, and also defensively handle null cell values
(convert to empty string) so the produced lines stay well-formed.
In
`@src/InstallationValidator.Core/Reporting/Markdown/OutputFileComparisonResultMarkdownBuilder.cs`:
- Around line 30-33: The current code calls
fileComparisonResult.TimeComparison.IsValid() without checking for null which
can throw a NullReferenceException; update the logic in
OutputFileComparisonResultMarkdownBuilder to first ensure TimeComparison is not
null (e.g., if (fileComparisonResult.TimeComparison != null &&
!fileComparisonResult.TimeComparison.IsValid()) {
_builderRepository.Report(fileComparisonResult.TimeComparison, context); }) so
Report is only invoked with a non-null TimeComparison, or alternatively use a
null-safe check (?.) combined with explicit boolean evaluation to achieve the
same effect.
In
`@src/InstallationValidator.Core/Reporting/Markdown/ValidationRunSummaryMarkdownBuilder.cs`:
- Around line 40-47: The three-line string produced by durationFor (using
Assets.Reporting.InstallationValidationPerformedIn) currently uses bare
Environment.NewLine characters which render as a single line in Markdown; update
the output so line breaks are Markdown-safe by replacing the newline separators
passed into InstallationValidationPerformedIn with " \n" (two trailing spaces +
newline) or "<br>" (or alternatively wire the value through the
MarkdownReportContext normaliser) so the StartTime, EndTime and duration render
each on their own line; change this in the durationFor method where
validationRunSummary.StartTime/EndTime and formatted duration are composed and
passed to Assets.Reporting.InstallationValidationPerformedIn.
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 45-55: The CreateReport method can dereference
installationValidationResult.RunSummary.StartTime without a null check; update
CreateReport to guard against a null RunSummary (or a null
installationValidationResult) before calling reportOutputPath: validate
installationValidationResult and installationValidationResult.RunSummary at the
start of CreateReport and either throw a clear
ArgumentException/ArgumentNullException or use a sensible default timestamp
(e.g., DateTime.UtcNow) when calling reportOutputPath(reportPath, ...) so
reportOutputPath and writeReportAsync never see a null StartTime; reference the
CreateReport method, installationValidationResult.RunSummary.StartTime, and
reportOutputPath when making the change.
In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs`:
- Around line 79-82: The reportOutputPath method currently always uses
Assets.Reporting.InstallationValidation, causing BatchComparisonResult outputs
to be misnamed; change reportOutputPath(string outputFilePath, DateTime
dateTime) to accept an additional report identifier (e.g., string reportName or
an enum/Assets.Reporting value) and use that instead of the hardcoded
Assets.Reporting.InstallationValidation; update all callers (including wherever
BatchComparisonResult is composed) to pass the correct report identifier (e.g.,
Assets.Reporting.BatchComparison or a descriptive name) so filenames reflect the
correct report type.
---
Nitpick comments:
In `@src/InstallationValidator.Core/Reporting/Charts/ChartData.cs`:
- Around line 5-13: Both consumers have identical
createChartData(OutputComparisonResult, bool) logic; move that mapping into a
single shared factory to avoid duplication by adding a static factory method on
ChartData (e.g., ChartData.CreateFrom(OutputComparisonResult result, bool
useLogScale)) that populates Title, XAxisLabel, YAxisLabel, UseLogScale and
builds Curve1/Curve2 (CurveData) from the OutputComparisonResult. Replace the
duplicated implementations in OutputComparisonResultMarkdownBuilder and
PdfReportDocument to call ChartData.CreateFrom(result, useLogScale) (or a small
internal helper with the same signature) so all mapping logic lives in one
place.
In
`@src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs`:
- Around line 10-15: The injected IEnumerable<IMarkdownBuilder> _builders in
MarkdownBuilderRepository is stored as-is causing repeated enumeration (and
possible re-evaluation) each time findBuilderFor or Report runs; materialize it
once in the MarkdownBuilderRepository constructor (e.g., assign _builders =
builders?.ToList() ?? Enumerable.Empty<IMarkdownBuilder>()) so subsequent calls
to findBuilderFor and Report use the cached collection and avoid double
FirstOrDefault enumeration and deferred execution costs.
In
`@src/InstallationValidator.Core/Reporting/Markdown/TimeComparisonResultMarkdownBuilder.cs`:
- Around line 1-16: TimeComparisonResultMarkdownBuilder currently constructs
DoubleFormatter with new DoubleFormatter(), and PdfReportDocument also new's it
elsewhere; change both to accept a DoubleFormatter via constructor injection
(add a readonly field on TimeComparisonResultMarkdownBuilder and assign it in
the constructor, and update PdfReportDocument to receive and use the injected
DoubleFormatter) so the formatter is provided by DI for consistency and
testability; update any DI registration to register DoubleFormatter as the
intended lifetime (e.g., singleton) and ensure classes requesting it are updated
to accept the dependency.
In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs`:
- Around line 304-327: The createChartData method is duplicated; extract it into
a shared static factory method e.g., add
ChartDataFactory.FromOutputComparison(OutputComparisonResult output, bool
useLogScale) that returns the ChartData built from OutputComparisonResult (using
Title, XAxisLabel, YAxisLabel, UseLogScale, Curve1 and Curve2 with the same
mappings and colors), then replace the private createChartData usages in
PdfReportDocument.cs (createChartData) and
OutputComparisonResultMarkdownBuilder.cs with calls to
ChartDataFactory.FromOutputComparison(output, useLogScale); ensure the new
factory references the ChartData and CurveData types and preserves the existing
field mappings (Output1/Output2.Caption, Times, Values and color choices).
In `@src/InstallationValidator.Core/Services/ValidationReportingTask.cs`:
- Around line 6-10: The interface IValidationReportingTask currently forces a
default format (Markdown) on consumers; update the interface to expose the
DefaultFormat property or accept a format parameter so callers can choose output
format: add a read-only property like DefaultFormat (or overload the
CreateReport methods to include a parameter e.g., ReportFormat format) to the
IValidationReportingTask contract and update the implementations (and any
callers bound to IValidationReportingTask) to honor that property/parameter when
producing reports in the CreateReport(InstallationValidationResult...) and
CreateReport(BatchComparisonResult...) flows so DI-bound consumers can switch
formats without depending on the concrete type.
In `@tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs`:
- Around line 116-183: Add a new test class
When_generating_chart_with_null_curves (in the same fixture set using
concern_for_SvgChartGenerator) that in Context sets _chartData.Curve1 = null and
_chartData.Curve2 = null, calls sut.GenerateLineChart(_chartData) in Because,
and asserts the returned string contains "<svg" and "</svg>" in the observation;
this ensures SvgChartGenerator.GenerateLineChart gracefully handles null
Curve1/Curve2 and prevents regressions previously causing a
NullReferenceException.
Comment on lines
+4
to
+6
| <TargetFramework>net8.0-windows</TargetFramework> | ||
| <UseWindowsForms>true</UseWindowsForms> | ||
| <EnableWindowsTargeting>true</EnableWindowsTargeting> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat src/SimulationOutputComparer/SimulationOutputComparer.csprojRepository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 3190
🏁 Script executed:
find . -name "SimulationOutputComparer.csproj" -type fRepository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 153
🏁 Script executed:
cat ./src/SimulationOutputComparer/SimulationOutputComparer.csprojRepository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 3190
🏁 Script executed:
ls -la src/SimulationOutputComparer/Repository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 693
🏁 Script executed:
wc -l ./src/SimulationOutputComparer/SimulationOutputComparer.csproj && cat -n ./src/SimulationOutputComparer/SimulationOutputComparer.csprojRepository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 3725
🌐 Web query:
EnableWindowsTargeting net8.0-windows redundant TargetFramework
💡 Result:
If your project already has:
<TargetFramework>net8.0-windows</TargetFramework>then EnableWindowsTargeting is not a replacement for -windows and is usually redundant on Windows.
net8.0-windows(the-windowsTFM) is what actually marks the project as Windows-targeting (and is required for WPF/WinForms). (learn.microsoft.com)EnableWindowsTargeting=trueis mainly for building Windows-targeting projects on non-Windows OSes (Linux/macOS). It tells the SDK to download the Windows targeting/runtime packs that aren’t automatically available there (otherwise you can hit warnings/errors like NETSDK1100). (stackoverflow.com)
Typical pattern if you want cross-OS dev/build:
<PropertyGroup>
<TargetFramework>net8.0-windows</TargetFramework>
</PropertyGroup>
<PropertyGroup Condition="'$(OS)' != 'Windows_NT'">
<EnableWindowsTargeting>true</EnableWindowsTargeting>
</PropertyGroup>Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props-desktop?utm_source=openai
- 2: https://stackoverflow.com/questions/74026084/what-exactly-does-enablewindowstargeting-do?utm_source=openai
🌐 Web query:
DevExpress 21.2 .NET 8 compatibility Windows Forms
💡 Result:
DevExpress v21.2 WinForms does not support .NET 8. DevExpress 21.2 introduced support for .NET 6 (and VS 2022), but .NET 8 support came later. [1]
For WinForms projects targeting .NET 8, DevExpress’ compatibility table indicates you need at least v23.1/v23.2 (or newer). [2] DevExpress also notes that v24.2+ treats .NET 8 as a minimally supported target framework, so upgrading to 24.2 or later is the safer long-term choice. [3]
Sources:
[1] DevExpress “New in v21.2” (lists .NET 6 support) (devexpress.com)
[2] DevExpress WinForms docs: “.NET / .NET Core Support…” compatibility table (docs.devexpress.com)
[3] DevExpress v24.2 release notes / framework support statement (devexpress.com)
Citations:
- 1: https://www.devexpress.com/subscriptions/new-2021-2.xml?utm_source=openai
- 2: https://docs.devexpress.com/WindowsForms/401191/dotnet-core-support?utm_source=openai
- 3: https://www.devexpress.com/subscriptions/new-2024-2.xml?utm_source=openai
Critical: DevExpress 21.2.15 does not support .NET 8; EnableWindowsTargeting is redundant.
The project targets net8.0-windows, but OSPSuite.DevExpress version 21.2.15 does not support .NET 8. DevExpress 21.2 has support only for .NET 6; .NET 8 compatibility requires DevExpress v23.1 or later (v24.2+ recommended for long-term stability). This incompatibility will cause build or runtime failures.
Additionally, <EnableWindowsTargeting>true</EnableWindowsTargeting> is redundant: that property is only useful when building Windows-targeting projects on non-Windows hosts (Linux/macOS). With net8.0-windows, the Windows API surface is already unconditionally enabled. The typical pattern is to set this conditionally only on non-Windows platforms.
♻️ Remove redundant property
<TargetFramework>net8.0-windows</TargetFramework>
<UseWindowsForms>true</UseWindowsForms>
- <EnableWindowsTargeting>true</EnableWindowsTargeting>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <TargetFramework>net8.0-windows</TargetFramework> | |
| <UseWindowsForms>true</UseWindowsForms> | |
| <EnableWindowsTargeting>true</EnableWindowsTargeting> | |
| <TargetFramework>net8.0-windows</TargetFramework> | |
| <UseWindowsForms>true</UseWindowsForms> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SimulationOutputComparer/SimulationOutputComparer.csproj` around lines 4
- 6, Project targets net8.0-windows but OSPSuite.DevExpress v21.2.15 only
supports up to .NET 6; either downgrade TargetFramework to net6.0-windows or
upgrade the DevExpress package to a compatible version (>=23.1, preferably
24.2+) so OSPSuite.DevExpress works with .NET 8; also remove the redundant
<EnableWindowsTargeting>true</EnableWindowsTargeting> entry (or make it
conditional for non-Windows CI) because TargetFramework="netX.Y-windows" already
enables the Windows surface.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Replace DefaultFormat property with ReportOptions parameter that includes: - ReportFormat as flags enum (None, Markdown, Pdf, All) - OpenReport boolean - Convenience properties ExportToMarkdown and ExportToPdf This allows generating both Markdown and PDF reports in a single call by using ReportFormat.All. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…onResult caption The trailing space in the caption prevented proper bold formatting in Markdown output (**Result of the validation: ** instead of **Result of the validation:**).
Valid outputs with matching curves don't need charts. Removing the valid-outputs-with-data loop prevents generating thousands of unnecessary pages when a file has many outputs but only a few are invalid.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy MiKTeX/LaTeX-based reporting pipeline with a pure .NET reporting solution that can generate Markdown (with embedded SVG charts) and PDFs (via QuestPDF), while also updating projects to .NET 8 Windows targeting.
Changes:
- Introduces Markdown reporting infrastructure (builders + repository) and a new SVG chart generator.
- Adds QuestPDF-based PDF report generation and wires reporting selection through
ReportFormat/ReportOptions. - Removes TeX reporting builders/reporters and drops the TeXReporting dependency; updates projects/packages and tests accordingly.
Reviewed changes
Copilot reviewed 55 out of 58 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/InstallationValidator.Tests/Services/ValidationReportingTaskSpecs.cs | Updates unit specs to validate Markdown/PDF task routing via ReportOptions. |
| tests/InstallationValidator.Tests/Services/BatchStarterTaskSpecs.cs | Updates expectation for StartableProcess.Start signature behavior. |
| tests/InstallationValidator.Tests/Reporting/SvgChartGeneratorSpecs.cs | Adds tests for the new SVG line chart generator (linear/log/edge cases). |
| tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs | Adds integration test for Markdown report file output/content. |
| tests/InstallationValidator.Tests/Presentation/SimulationComparisonPresenterSpecs.cs | Updates presenter spec for new reporting API (ReportOptions). |
| tests/InstallationValidator.Tests/Presentation/MainPresenterSpecs.cs | Updates presenter spec for new reporting API (ReportOptions). |
| tests/InstallationValidator.Tests/IntegrationTests/ReportingSpecs.cs | Removes TeX/PDF reporting integration test relying on TeX tooling. |
| tests/InstallationValidator.Tests/InstallationValidator.Tests.csproj | Migrates tests to .NET 8 Windows targeting; updates OSPSuite packages and removes TeXReporting. |
| src/SimulationOutputComparer/SimulationOutputComparer.csproj | Migrates app to .NET 8 Windows targeting; updates packages and removes TeXReporting content. |
| src/InstallationValidator/Program.cs | Initializes QuestPDF license setting at application startup. |
| src/InstallationValidator/InstallationValidator.csproj | Migrates app to .NET 8 Windows targeting; updates packages and removes TeXReporting references/content. |
| src/InstallationValidator.Core/ValidatorRegister.cs | Replaces TeX reporting registration with Markdown/PDF reporting registration and SVG chart generator wiring. |
| src/InstallationValidator.Core/Services/ValidationReportingTask.cs | Introduces ReportFormat/ReportOptions and routes report generation to Markdown/PDF tasks. |
| src/InstallationValidator.Core/Services/PdfReportingTask.cs | Adds QuestPDF-based PDF report generation task. |
| src/InstallationValidator.Core/Services/MarkdownReportingTask.cs | Adds Markdown report generation task driven by markdown builders. |
| src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs | Adds QuestPDF document composer for validation and folder comparison reports. |
| src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs | Adds SVG chart rendering for two-curve line charts with linear/log scaling. |
| src/InstallationValidator.Core/Reporting/Charts/ChartData.cs | Adds chart and curve data DTOs for SVG/PDF rendering. |
| src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs | Introduces Markdown builder abstraction mirroring the legacy builder pattern. |
| src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs | Adds builder repository to resolve and invoke Markdown builders per reported object. |
| src/InstallationValidator.Core/Reporting/Markdown/MarkdownReportContext.cs | Adds Markdown output buffer/formatting helpers (headings, tables, SVG, status rendering). |
| src/InstallationValidator.Core/Reporting/Markdown/InstallationValidationResultMarkdownBuilder.cs | Adds Markdown builder for installation validation reports. |
| src/InstallationValidator.Core/Reporting/Markdown/BatchComparisonResultMarkdownBuilder.cs | Adds Markdown builder for batch comparison sections and per-simulation breakdown. |
| src/InstallationValidator.Core/Reporting/Markdown/OutputFileComparisonResultMarkdownBuilder.cs | Adds Markdown builder for output-file comparison details. |
| src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs | Adds Markdown builder for per-output details and embedded SVG charts. |
| src/InstallationValidator.Core/Reporting/Markdown/TimeComparisonResultMarkdownBuilder.cs | Adds Markdown builder for time-comparison failures. |
| src/InstallationValidator.Core/Reporting/Markdown/ValidationRunSummaryMarkdownBuilder.cs | Adds Markdown builder for run summary/environment details. |
| src/InstallationValidator.Core/Reporting/Markdown/OperatingSystemInfoMarkdownBuilder.cs | Adds Markdown builder for OS/environment listing. |
| src/InstallationValidator.Core/Reporting/Markdown/MissingFileComparisonResultMarkdownBuilder.cs | Adds Markdown builder for missing-file comparisons. |
| src/InstallationValidator.Core/Reporting/Markdown/ValidationStateReportMarkdownBuilder.cs | Adds Markdown builder for validation state rendering. |
| src/InstallationValidator.Core/Reporting/ValueComparisonsResultTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/ValueComparisonResultTeXBuilder.cs | Removes legacy TeX builder base. |
| src/InstallationValidator.Core/Reporting/ValidationStateReportTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/ValidationRunSummaryTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/TimeComparisonResultTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/OutputFileComparisonResultTexBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/OutputComparisonResultTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/OperatingSystemInfoTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/MissingFileComparisonResultTexBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/BatchComparisonResultTeXBuilder.cs | Removes legacy TeX builder. |
| src/InstallationValidator.Core/Reporting/BatchComparisonResultReporter.cs | Removes legacy TeX reporter. |
| src/InstallationValidator.Core/Reporting/InstallationValidationResultReporter.cs | Removes legacy TeX reporter. |
| src/InstallationValidator.Core/Reporting/FileComparisonResultsTeXBuilder.cs | Removes legacy TeX builder(s) for file comparison results. |
| src/InstallationValidator.Core/InstallationValidator.Core.csproj | Migrates core to .NET 8 Windows targeting; updates OSPSuite packages, adds QuestPDF, removes TeX reporting dependencies. |
| src/InstallationValidator.Core/Domain/OperatingSystemInfo.cs | Minor whitespace/formatting adjustment. |
| src/InstallationValidator.Core/Assets/Captions.cs | Minor caption formatting tweak for validation result label. |
| src/InstallationValidator.Core/Presentation/MainPresenter.cs | Updates report creation call to use ReportOptions. |
| src/InstallationValidator.Core/Presentation/SimulationComparisonPresenter.cs | Updates report creation call to use ReportOptions. |
| .gitignore | Ignores local Claude settings file. |
Files not reviewed (2)
- src/InstallationValidator/Views/MainView.Designer.cs: Language not supported
- src/SimulationOutputComparer/Views/SimulationComparisonView.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
@coderabbitai full review Please provide a comprehensive summary including:
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs (1)
103-107:⚠️ Potential issue | 🟡 MinorAssert
_reportPathbefore reading the Markdown file.If report creation fails, this observation throws from
File.ReadAllTextinstead of failing with the same clear signal asshould_create_markdown_file.🛠 Proposed fix
[Observation] public void should_contain_report_content() { + _reportPath.ShouldNotBeNull(); var content = File.ReadAllText(_reportPath); StringAssert.Contains("Installation Validation", content); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs` around lines 103 - 107, Add an explicit existence assertion for the Markdown file before reading it in the should_contain_report_content observation: check that _reportPath exists (e.g. Assert.IsTrue(File.Exists(_reportPath)) or Assert.That(File.Exists(_reportPath), Is.True)) at the start of should_contain_report_content so a missing report fails with the same clear signal as should_create_markdown_file, then proceed to File.ReadAllText and the StringAssert.Contains check.src/InstallationValidator.Core/Services/MarkdownReportingTask.cs (1)
39-40:⚠️ Potential issue | 🟡 MinorBatch reports still get the installation-validation filename.
CreateReport(BatchComparisonResult, ...)builds a folder-comparison report, butreportOutputPathalways usesAssets.Reporting.InstallationValidation. The generated.mdfilename therefore disagrees with the content/header for batch reports.🛠 Proposed fix
- var reportPath = reportOutputPath(secondFolderPath, DateTime.Now); + var reportPath = reportOutputPath(secondFolderPath, DateTime.Now, Assets.Reporting.FolderComparison); @@ - var reportPath = reportOutputPath(outputFolderPath, installationValidationResult.RunSummary.StartTime); + var reportPath = reportOutputPath(outputFolderPath, installationValidationResult.RunSummary.StartTime, Assets.Reporting.InstallationValidation); @@ - private string reportOutputPath(string outputFilePath, DateTime dateTime) + private string reportOutputPath(string outputFilePath, DateTime dateTime, string reportLabel) { - return Path.Combine(outputFilePath, $"{_applicationConfiguration.OSPSuiteNameWithVersion}-{Assets.Reporting.InstallationValidation}_{dateTime:MM_dd_yy_H_mm_ss}.md"); + return Path.Combine(outputFilePath, $"{_applicationConfiguration.OSPSuiteNameWithVersion}-{reportLabel}_{dateTime:MM_dd_yy_H_mm_ss}.md"); }Also applies to: 56-57, 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs` around lines 39 - 40, CreateReport(BatchComparisonResult, ...) is calling reportOutputPath with the wrong asset key (Assets.Reporting.InstallationValidation), so batch/folder-comparison reports get an installation-validation filename; update the batch-report CreateReport overload to call reportOutputPath with the correct reporting asset identifier for batch/folder comparisons (e.g., Assets.Reporting.FolderComparison or the appropriate BatchComparison constant) instead of Assets.Reporting.InstallationValidation, and ensure the same fix is applied to the other occurrences that call reportOutputPath (the other CreateReport overloads and the lines that call writeReportAsync) so the generated .md filename matches the report content and header.src/InstallationValidator/InstallationValidator.csproj (1)
4-6:⚠️ Potential issue | 🔴 CriticalPlease re-check the DevExpress line before completing the .NET 8 retarget.
DevExpress's WinForms support tables show .NET 8 support starting at v23.1, and the v21.2 release notes only advertise .NET 6 support. If
OSPSuite.DevExpress 21.2.15.1still carries the 21.2 DevExpress bits, this project—and the matching references inInstallationValidator.Core.csprojandSimulationOutputComparer.csproj—land outside the published support matrix. (docs.devexpress.com)#!/bin/bash # Expected: enumerate every net8.0-windows project using OSPSuite.DevExpress 21.2.15.1. rg -nP --glob '*.csproj' '(<TargetFramework>net8\.0-windows</TargetFramework>)|(<PackageReference Include="OSPSuite\.DevExpress" Version="21\.2\.15\.1")'Also applies to: 51-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator/InstallationValidator.csproj` around lines 4 - 6, The change retargeted InstallationValidator (and related projects InstallationValidator.Core and SimulationOutputComparer) to <TargetFramework>net8.0-windows</TargetFramework> while still referencing OSPSuite.DevExpress 21.2.15.1; verify whether OSPSuite.DevExpress 21.2.15.1 actually contains DevExpress bits that support .NET 8 (per DevExpress docs support starts at v23.1) and either (A) update the PackageReference to an OSPSuite.DevExpress/DevExpress version that explicitly supports .NET 8 (e.g., v23.1+) across InstallationValidator.csproj, InstallationValidator.Core.csproj and SimulationOutputComparer.csproj, or (B) if no compatible package exists, revert the TargetFramework value back to a supported framework (e.g., net6.0-windows) for those projects; run the provided ripgrep check to enumerate remaining net8.0-windows projects still using OSPSuite.DevExpress 21.2.15.1 and fix each identified project accordingly.
🧹 Nitpick comments (5)
src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs (1)
4-4: Unused import.
System.Collections.Genericis imported but not used in this file.🧹 Remove unused import
using OSPSuite.Utility; using OSPSuite.Utility.Extensions; using System; -using System.Collections.Generic;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs` at line 4, Remove the unused System.Collections.Generic import from the IMarkdownBuilder.cs file: delete the line "using System.Collections.Generic;" at the top of the file so the IMarkdownBuilder interface no longer has an unnecessary using directive.tests/InstallationValidator.Tests/Presentation/MainPresenterSpecs.cs (1)
178-182: Consider adding a more specific matcher forReportOptions.The
A<ReportOptions>._matcher accepts any options, which means the test won't catch regressions if the presenter passes incorrect values (e.g.,openReport: falseor wrong format flags).If verifying the specific options is important for this test, consider using a matcher that validates key properties:
💡 Optional: More specific assertion
[Observation] public void should_generate_the_report() { - A.CallTo(() => _validationReportingTask.CreateReport(_result, _outputFolderDTO.FolderPath, A<ReportOptions>._)).MustHaveHappened(); + A.CallTo(() => _validationReportingTask.CreateReport(_result, _outputFolderDTO.FolderPath, + A<ReportOptions>.That.Matches(x => x.OpenReport))).MustHaveHappened(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Presentation/MainPresenterSpecs.cs` around lines 178 - 182, The test uses a permissive matcher A<ReportOptions>._ which allows any ReportOptions and won't catch regressions; update the assertion on _validationReportingTask.CreateReport(_result, _outputFolderDTO.FolderPath, A<ReportOptions>._) to use a specific matcher (e.g., A<ReportOptions>.That.Matches(...)) that validates the critical properties you expect (for example OpenReport==true and the expected Format/flags) so the spec fails if the presenter passes incorrect ReportOptions.tests/InstallationValidator.Tests/Presentation/SimulationComparisonPresenterSpecs.cs (2)
130-134: Test coverage could be more specific about ReportOptions values.The test uses
A<ReportOptions>._which accepts anyReportOptionsvalue. Per the context snippet fromSimulationComparisonPresenter.cs(line 75), the presenter createsReportOptionswithopenReport: trueand a format derived from the DTO. Consider adding a verification that validates the expectedReportOptionsproperties to catch regressions if the presenter logic changes.💡 Example: More specific assertion
[Observation] public void should_generate_the_report_with_correct_options() { A.CallTo(() => _validationReportingTask.CreateReport( A<BatchComparisonResult>._, _firstFolder.FolderPath, _secondFolder.FolderPath, A<ReportOptions>.That.Matches(opts => opts.OpenReport == true))) .MustHaveHappened(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Presentation/SimulationComparisonPresenterSpecs.cs` around lines 130 - 134, The test currently uses a wildcard for ReportOptions which permits regressions; update the assertion in the observation test (should_generate_the_report) to verify the ReportOptions passed to ValidationReportingTask.CreateReport by using A<ReportOptions>.That.Matches(...) and assert at minimum opts.OpenReport == true and that opts.Format equals the expected format derived from the DTO (the same format logic exercised by SimulationComparisonPresenter when it constructs the ReportOptions). Target the CreateReport call and the ReportOptions type in the matcher so the test fails if the presenter changes those properties.
12-12: Remove the unusedOSPSuite.BDDHelper.Extensionsusing directive.The import is not used in this file; all assertions use FakeItEasy's
MustHaveHappened()andMustNotHaveHappened()methods instead of the BDD-style assertion extensions fromOSPSuite.BDDHelper.Extensions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Presentation/SimulationComparisonPresenterSpecs.cs` at line 12, Remove the unused using directive "using OSPSuite.BDDHelper.Extensions;" from SimulationComparisonPresenterSpecs.cs; locate the top-of-file imports in the SimulationComparisonPresenterSpecs class and delete that specific using line (ensuring no BDDHelper extension methods are referenced elsewhere), leaving only the required using statements for FakeItEasy assertions like MustHaveHappened()/MustNotHaveHappened().src/InstallationValidator.Core/Presentation/MainPresenter.cs (1)
114-120: Consider validating that at least one export format is selected.When both
ExportToPdfandExportToMarkdownare false,reportFormatFromDTO()returnsReportFormat.NoneandCreateReportis called without either flag set. While the downstreamCreateReportmethod safely handles this by guarding against each flag before executing the respective reporting tasks, calling it with no work to perform is inefficient. Prevent this state by either:
- Validating in the UI that at least one option is checked before starting.
- Short-circuiting in
CreateReportbefore calling downstream reporting tasks when both flags are false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Presentation/MainPresenter.cs` around lines 114 - 120, reportFormatFromDTO() can return ReportFormat.None when both _reportOptionsDTO.ExportToPdf and _reportOptionsDTO.ExportToMarkdown are false, causing CreateReport to be invoked with no work; update the flow to validate and short-circuit: either add a UI/validation check to require at least one export option before starting, or modify CreateReport to immediately return (no-op) when reportFormatFromDTO() == ReportFormat.None (or explicitly when both _reportOptionsDTO.ExportToPdf and ExportToMarkdown are false) to avoid unnecessary downstream calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs`:
- Around line 114-121: The first run-summary line is using the wrong label: in
the column.Item().Text(...) lambda that currently writes
text.Span(Assets.Reporting.BatchRunDuration + ": ").Bold() followed by the start
time (summary.StartTime), change the label to the appropriate start-time label
(e.g. Assets.Reporting.StartTime or equivalent resource key) so the span reads
the start-time label before summary.StartTime; do not change the duration line
that uses timeSpent.ToDisplay().
In `@src/InstallationValidator.Core/ValidatorRegister.cs`:
- Around line 77-96: The registerMarkdownReporting method misses an explicit
registration for IMarkdownReportingTask implemented by MarkdownReportingTask
causing DI resolution failure for ValidationReportingTask; inside
registerMarkdownReporting add a container.Register<IMarkdownReportingTask,
MarkdownReportingTask>(LifeStyle.Whatever) (matching the intended lifestyle like
the IPdfReportingTask registration) so the IMarkdownReportingTask service is
registered explicitly alongside the existing scanner and
IMarkdownBuilderRepository registration.
In `@tests/InstallationValidator.Tests/Reporting/PdfReportingSpecs.cs`:
- Around line 112-116: The test should_create_non_empty_pdf_file currently
constructs FileInfo(_reportPath) without asserting the file exists first; add an
explicit assertion that the file exists (e.g., Assert.True/File.Exists or
ShouldBeTrue on File.Exists(_reportPath)) referencing the same _reportPath
before calling new FileInfo(_reportPath).Length so failures indicate a missing
PDF rather than a path exception; apply the same change to the other tests that
use _reportPath (lines around the other two occurrences).
---
Duplicate comments:
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 39-40: CreateReport(BatchComparisonResult, ...) is calling
reportOutputPath with the wrong asset key
(Assets.Reporting.InstallationValidation), so batch/folder-comparison reports
get an installation-validation filename; update the batch-report CreateReport
overload to call reportOutputPath with the correct reporting asset identifier
for batch/folder comparisons (e.g., Assets.Reporting.FolderComparison or the
appropriate BatchComparison constant) instead of
Assets.Reporting.InstallationValidation, and ensure the same fix is applied to
the other occurrences that call reportOutputPath (the other CreateReport
overloads and the lines that call writeReportAsync) so the generated .md
filename matches the report content and header.
In `@src/InstallationValidator/InstallationValidator.csproj`:
- Around line 4-6: The change retargeted InstallationValidator (and related
projects InstallationValidator.Core and SimulationOutputComparer) to
<TargetFramework>net8.0-windows</TargetFramework> while still referencing
OSPSuite.DevExpress 21.2.15.1; verify whether OSPSuite.DevExpress 21.2.15.1
actually contains DevExpress bits that support .NET 8 (per DevExpress docs
support starts at v23.1) and either (A) update the PackageReference to an
OSPSuite.DevExpress/DevExpress version that explicitly supports .NET 8 (e.g.,
v23.1+) across InstallationValidator.csproj, InstallationValidator.Core.csproj
and SimulationOutputComparer.csproj, or (B) if no compatible package exists,
revert the TargetFramework value back to a supported framework (e.g.,
net6.0-windows) for those projects; run the provided ripgrep check to enumerate
remaining net8.0-windows projects still using OSPSuite.DevExpress 21.2.15.1 and
fix each identified project accordingly.
In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs`:
- Around line 103-107: Add an explicit existence assertion for the Markdown file
before reading it in the should_contain_report_content observation: check that
_reportPath exists (e.g. Assert.IsTrue(File.Exists(_reportPath)) or
Assert.That(File.Exists(_reportPath), Is.True)) at the start of
should_contain_report_content so a missing report fails with the same clear
signal as should_create_markdown_file, then proceed to File.ReadAllText and the
StringAssert.Contains check.
---
Nitpick comments:
In `@src/InstallationValidator.Core/Presentation/MainPresenter.cs`:
- Around line 114-120: reportFormatFromDTO() can return ReportFormat.None when
both _reportOptionsDTO.ExportToPdf and _reportOptionsDTO.ExportToMarkdown are
false, causing CreateReport to be invoked with no work; update the flow to
validate and short-circuit: either add a UI/validation check to require at least
one export option before starting, or modify CreateReport to immediately return
(no-op) when reportFormatFromDTO() == ReportFormat.None (or explicitly when both
_reportOptionsDTO.ExportToPdf and ExportToMarkdown are false) to avoid
unnecessary downstream calls.
In `@src/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cs`:
- Line 4: Remove the unused System.Collections.Generic import from the
IMarkdownBuilder.cs file: delete the line "using System.Collections.Generic;" at
the top of the file so the IMarkdownBuilder interface no longer has an
unnecessary using directive.
In `@tests/InstallationValidator.Tests/Presentation/MainPresenterSpecs.cs`:
- Around line 178-182: The test uses a permissive matcher A<ReportOptions>._
which allows any ReportOptions and won't catch regressions; update the assertion
on _validationReportingTask.CreateReport(_result, _outputFolderDTO.FolderPath,
A<ReportOptions>._) to use a specific matcher (e.g.,
A<ReportOptions>.That.Matches(...)) that validates the critical properties you
expect (for example OpenReport==true and the expected Format/flags) so the spec
fails if the presenter passes incorrect ReportOptions.
In
`@tests/InstallationValidator.Tests/Presentation/SimulationComparisonPresenterSpecs.cs`:
- Around line 130-134: The test currently uses a wildcard for ReportOptions
which permits regressions; update the assertion in the observation test
(should_generate_the_report) to verify the ReportOptions passed to
ValidationReportingTask.CreateReport by using A<ReportOptions>.That.Matches(...)
and assert at minimum opts.OpenReport == true and that opts.Format equals the
expected format derived from the DTO (the same format logic exercised by
SimulationComparisonPresenter when it constructs the ReportOptions). Target the
CreateReport call and the ReportOptions type in the matcher so the test fails if
the presenter changes those properties.
- Line 12: Remove the unused using directive "using
OSPSuite.BDDHelper.Extensions;" from SimulationComparisonPresenterSpecs.cs;
locate the top-of-file imports in the SimulationComparisonPresenterSpecs class
and delete that specific using line (ensuring no BDDHelper extension methods are
referenced elsewhere), leaving only the required using statements for FakeItEasy
assertions like MustHaveHappened()/MustNotHaveHappened().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9da96d74-5d1c-408b-8ddf-668359157dea
📒 Files selected for processing (30)
src/InstallationValidator.Core/Assets/Captions.cssrc/InstallationValidator.Core/InstallationValidator.Core.csprojsrc/InstallationValidator.Core/Presentation/DTO/ReportOptionsDTO.cssrc/InstallationValidator.Core/Presentation/MainPresenter.cssrc/InstallationValidator.Core/Presentation/SimulationComparisonPresenter.cssrc/InstallationValidator.Core/Presentation/Views/IMainView.cssrc/InstallationValidator.Core/Presentation/Views/ISimulationComparisonView.cssrc/InstallationValidator.Core/Reporting/Markdown/IMarkdownBuilder.cssrc/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cssrc/InstallationValidator.Core/Reporting/Markdown/MarkdownReportContext.cssrc/InstallationValidator.Core/Reporting/Markdown/OutputFileComparisonResultMarkdownBuilder.cssrc/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cssrc/InstallationValidator.Core/Services/MarkdownReportingTask.cssrc/InstallationValidator.Core/Services/PdfReportingTask.cssrc/InstallationValidator.Core/Services/ValidationReportingTask.cssrc/InstallationValidator.Core/ValidatorRegister.cssrc/InstallationValidator/InstallationValidator.csprojsrc/InstallationValidator/Views/MainView.Designer.cssrc/InstallationValidator/Views/MainView.cssrc/SimulationOutputComparer/Program.cssrc/SimulationOutputComparer/SimulationOutputComparer.csprojsrc/SimulationOutputComparer/Views/SimulationComparisonView.Designer.cssrc/SimulationOutputComparer/Views/SimulationComparisonView.cstests/InstallationValidator.Tests/InstallationValidator.Tests.csprojtests/InstallationValidator.Tests/Presentation/MainPresenterSpecs.cstests/InstallationValidator.Tests/Presentation/SimulationComparisonPresenterSpecs.cstests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cstests/InstallationValidator.Tests/Reporting/PdfReportingSpecs.cstests/InstallationValidator.Tests/Services/BatchStarterTaskSpecs.cstests/InstallationValidator.Tests/Services/ValidationReportingTaskSpecs.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/InstallationValidator.Core/Services/PdfReportingTask.cs
- src/InstallationValidator.Core/Reporting/Markdown/MarkdownReportContext.cs
- src/InstallationValidator.Core/Reporting/Markdown/MarkdownBuilderRepository.cs
Pass reportName parameter to reportOutputPath so PDF filenames reflect the actual report type (FolderComparison vs InstallationValidation).
Member
Author
|
ready for review |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs (1)
114-121:⚠️ Potential issue | 🟡 MinorUse the start-time label on the first summary row.
Line 117 still prefixes
summary.StartTimewithAssets.Reporting.BatchRunDuration, so the PDF reads like a duration label followed by a timestamp. Switch that span to the start-time resource and keep the actual duration text on Line 121 unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs` around lines 114 - 121, The first summary row in PdfReportDocument uses the wrong resource label: when rendering summary.StartTime inside the column.Item().Text block (the lambda that calls text.Span(...).Bold()), replace Assets.Reporting.BatchRunDuration with the start-time resource (e.g., Assets.Reporting.StartTime or the appropriate StartTime key in Assets.Reporting) so the span reads the start-time label for summary.StartTime, while leaving the final duration line that uses timeSpent (Validation performed in {timeSpent.ToDisplay()}) unchanged.
🧹 Nitpick comments (1)
src/InstallationValidator.Core/Services/PdfReportingTask.cs (1)
76-94: Consider extracting shared helpers to reduce duplication.
openReportIfRequired,reportOutputPath, and directory creation logic are nearly identical betweenPdfReportingTaskandMarkdownReportingTask(differing only in file extension). This is a good candidate for extraction to a shared base class or utility.♻️ Example: Shared base class
public abstract class ReportingTaskBase { protected readonly IValidationLogger _validationLogger; protected readonly IInstallationValidatorConfiguration _applicationConfiguration; protected ReportingTaskBase(IValidationLogger validationLogger, IInstallationValidatorConfiguration applicationConfiguration) { _validationLogger = validationLogger; _applicationConfiguration = applicationConfiguration; } protected abstract string FileExtension { get; } protected void OpenReportIfRequired(bool openReport, string reportPath) { if (openReport) FileHelper.TryOpenFile(reportPath); _validationLogger.AppendLine(Logs.ReportCreatedUnder(reportPath)); } protected string ReportOutputPath(string outputFilePath, DateTime dateTime, string reportName) { return Path.Combine(outputFilePath, $"{_applicationConfiguration.OSPSuiteNameWithVersion}-{reportName}_{dateTime:MM_dd_yy_H_mm_ss}.{FileExtension}"); } protected void EnsureDirectoryExists(string filePath) { var directory = Path.GetDirectoryName(filePath); if (!string.IsNullOrEmpty(directory)) Directory.CreateDirectory(directory); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs` around lines 76 - 94, PdfReportingTask’s helpers openReportIfRequired, reportOutputPath and ensureDirectoryExists are duplicated in MarkdownReportingTask; extract them into a shared base class or utility to remove duplication. Create an abstract ReportingTaskBase with protected members _validationLogger and _applicationConfiguration, add protected methods OpenReportIfRequired(bool openReport, string reportPath), ReportOutputPath(string outputFilePath, DateTime dateTime, string reportName) and EnsureDirectoryExists(string filePath), and an abstract/protected FileExtension property so ReportOutputPath builds the filename with the correct extension; then have PdfReportingTask and MarkdownReportingTask inherit from ReportingTaskBase and replace their local methods with calls to the base implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs`:
- Around line 120-121: The PDF generator currently uses hardcoded English
strings in PdfReportDocument (e.g., the calls to column.Item().Text($"End time:
{summary.EndTime.ToIsoFormat()}") and column.Item().Text($"Validation performed
in {timeSpent.ToDisplay()}"), plus the OS, Yes/No and missing-file sentences
around the other noted blocks), causing mixed-language output; replace those
literal strings with localized resources from Assets.Reporting (add or use keys
like EndTime, ValidationPerformedIn, OSLabel, YesText, NoText,
MissingFileTemplate), format them with the same values (e.g., pass
summary.EndTime.ToIsoFormat() and timeSpent.ToDisplay() into the resource
templates), and ensure the code that builds the sentences (including the
missing-file messages and OS/YesNo rendering) reads from Assets.Reporting so the
PDF uses the current culture consistently.
- Around line 26-42: Add guard clauses in both PdfReportDocument constructors to
fail fast: in the constructor taking InstallationValidationResult ensure the
validationResult and svgChartGenerator parameters are not null and throw
ArgumentNullException(nameof(validationResult)) or
ArgumentNullException(nameof(svgChartGenerator)) as appropriate; in the
constructor taking BatchComparisonResult ensure comparisonResult and
svgChartGenerator are not null and throw
ArgumentNullException(nameof(comparisonResult)) or
ArgumentNullException(nameof(svgChartGenerator)). This ensures
PdfReportDocument's constructors validate their inputs immediately (referencing
the PdfReportDocument(InstallationValidationResult, string, string,
ISvgChartGenerator) and PdfReportDocument(BatchComparisonResult, string, string,
ISvgChartGenerator) overloads).
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 33-43: The CreateReport(BatchComparisonResult comparisonResult,
string firstFolderPath, string secondFolderPath, bool openReport = false) method
in MarkdownReportingTask should validate that comparisonResult is not null at
the start of the method (same pattern used by the
CreateReport(InstallationValidationResult...) overload); if null, throw an
ArgumentNullException for comparisonResult. Add this null check before calling
_builderRepository.Report (which delegates to MarkdownBuilderRepository.Report)
so you don't produce an empty header-only report when comparisonResult is
missing.
In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs`:
- Around line 35-51: The CreateReport(BatchComparisonResult comparisonResult,
string firstFolderPath, string secondFolderPath, bool openReport = false) method
lacks a null check for comparisonResult; add a guard at the start of
CreateReport that throws ArgumentNullException(nameof(comparisonResult)) when
comparisonResult is null (matching the pattern used in the
InstallationValidationResult overload/MarkdownReportingTask), so
PdfReportDocument is never constructed with a null argument.
---
Duplicate comments:
In `@src/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cs`:
- Around line 114-121: The first summary row in PdfReportDocument uses the wrong
resource label: when rendering summary.StartTime inside the column.Item().Text
block (the lambda that calls text.Span(...).Bold()), replace
Assets.Reporting.BatchRunDuration with the start-time resource (e.g.,
Assets.Reporting.StartTime or the appropriate StartTime key in Assets.Reporting)
so the span reads the start-time label for summary.StartTime, while leaving the
final duration line that uses timeSpent (Validation performed in
{timeSpent.ToDisplay()}) unchanged.
---
Nitpick comments:
In `@src/InstallationValidator.Core/Services/PdfReportingTask.cs`:
- Around line 76-94: PdfReportingTask’s helpers openReportIfRequired,
reportOutputPath and ensureDirectoryExists are duplicated in
MarkdownReportingTask; extract them into a shared base class or utility to
remove duplication. Create an abstract ReportingTaskBase with protected members
_validationLogger and _applicationConfiguration, add protected methods
OpenReportIfRequired(bool openReport, string reportPath),
ReportOutputPath(string outputFilePath, DateTime dateTime, string reportName)
and EnsureDirectoryExists(string filePath), and an abstract/protected
FileExtension property so ReportOutputPath builds the filename with the correct
extension; then have PdfReportingTask and MarkdownReportingTask inherit from
ReportingTaskBase and replace their local methods with calls to the base
implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 821e0414-4c64-409d-b6ac-72e2e17a436b
📒 Files selected for processing (5)
src/InstallationValidator.Core/Reporting/Markdown/OutputFileComparisonResultMarkdownBuilder.cssrc/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cssrc/InstallationValidator.Core/Services/BatchComparisonTask.cssrc/InstallationValidator.Core/Services/MarkdownReportingTask.cssrc/InstallationValidator.Core/Services/PdfReportingTask.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/InstallationValidator.Core/Reporting/Markdown/OutputFileComparisonResultMarkdownBuilder.cs
- Extract ChartDataFactory.CreateFor() to eliminate duplicate createChartData methods in OutputComparisonResultMarkdownBuilder and PdfReportDocument - Extract ColorExtensions.ToHexString() to replace colorToHex in SvgChartGenerator, toQuestColor in PdfReportDocument, and inline hex formatting in MarkdownReportContext - Fix PDF run summary labels: start/end time now have their own labels instead of reusing BatchRunDuration for the start time - Disable start button in MainView and SimulationComparisonView when neither PDF nor Markdown report format is selected - Replace Enumerable.Select allocation with Enumerable.Repeat in MarkdownReportContext.AppendTable separator row - Add MarkdownReportContextSpecs unit tests covering headings, tables, colored status, bold formatting, and content accumulation - Add observations to MarkdownReportingSpecs for SVG chart, validation state, and deviation section
Yuri05
reviewed
Mar 10, 2026
Yuri05
reviewed
Mar 10, 2026
Yuri05
reviewed
Mar 10, 2026
Member
Looks good for me. Once the target branch is changed to v13 and AI-review comments are addressed - can be merged IMO. |
Member
Author
|
you said > Valid outputs with data are never rendered in Markdown (they are in PDF via ComposeOutputFileResult) I don't see it in the code. Please cehck it again |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the actual code directly to verify this. 🏁 Script executed: Length of output: 2570
var allValidOutputsWithData = fileComparisonResult.OutputComparisonResults.Where(x => x.IsValid()).Where(x => x.HasData);
allValidOutputsWithData.Each(x => _builderRepository.Report(x, context));
var validOutputsWithData = result.OutputComparisonResults.Where(x => x.IsValid()).Where(x => x.HasData);
validOutputsWithData.Each(x => composeOutputComparison(column, x));The two implementations are identical in intent. My earlier analysis was wrong — the parity concern I flagged does not exist. Fix 1 from my "Full Prompt" section should also be disregarded accordingly. Everything else in the review stands. Sorry for the noise! 🧠 Learnings used |
- Add localized resource keys to Assets.Reporting: StartTime, EndTime, Yes, No, OSLabel, FileWasContainedInFolder, ButWasMissingInFolder, ValidationPerformedIn - Replace all hardcoded English strings in PdfReportDocument with Assets.Reporting resources for consistent localization - Add constructor null guards in PdfReportDocument for validationResult, comparisonResult, and svgChartGenerator - Add null guard for comparisonResult in both PdfReportingTask and MarkdownReportingTask CreateReport overloads
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/InstallationValidator.Core/Services/MarkdownReportingTask.cs (1)
93-96: Consider usingHHfor consistent filename sorting.The timestamp format
H_mm_ssproduces single-digit hours (0-9) without a leading zero, resulting in inconsistent filename lengths (e.g.,_9_30_00vs_10_30_00). This can cause alphabetical sorting issues in file explorers.📝 Suggested fix
private string reportOutputPath(string outputFilePath, DateTime dateTime, string reportName) { - return Path.Combine(outputFilePath, $"{_applicationConfiguration.OSPSuiteNameWithVersion}-{reportName}_{dateTime:MM_dd_yy_H_mm_ss}.md"); + return Path.Combine(outputFilePath, $"{_applicationConfiguration.OSPSuiteNameWithVersion}-{reportName}_{dateTime:MM_dd_yy_HH_mm_ss}.md"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs` around lines 93 - 96, The filename timestamp in reportOutputPath currently uses a single-digit hour format (H) causing inconsistent lengths; update the DateTime format string in reportOutputPath (the method building the Path.Combine with _applicationConfiguration.OSPSuiteNameWithVersion and reportName) to use a two-digit hour specifier (HH) so hours are zero-padded and filenames sort consistently.tests/InstallationValidator.Tests/Reporting/MarkdownReportContextSpecs.cs (1)
6-7: Consider aligning namespace with file path.The file is located at
tests/InstallationValidator.Tests/Reporting/but the namespace isInstallationValidator.Reporting. Consider usingInstallationValidator.Tests.Reportingto match the project structure convention.Proposed fix
-namespace InstallationValidator.Reporting +namespace InstallationValidator.Tests.Reporting🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportContextSpecs.cs` around lines 6 - 7, The namespace in this test file is incorrect for its path: change the namespace declaration from InstallationValidator.Reporting to InstallationValidator.Tests.Reporting so it matches the test project's folder structure and naming convention (update the namespace line at the top of the file where the namespace is declared).src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs (1)
231-240: Minor: Inconsistent indentation onescapeXmlmethod.The method declaration is missing the standard indentation (6 spaces) that other private methods in this class use.
Proposed fix
-private string escapeXml(string text) + private string escapeXml(string text) { if (string.IsNullOrEmpty(text)) return ""; return text .Replace("&", "&") .Replace("<", "<") .Replace(">", ">") .Replace("\"", """) .Replace("'", "'"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs` around lines 231 - 240, The private method escapeXml has incorrect indentation compared to other private methods; reformat its declaration and body to use the same 6-space indentation used across the class so the method signature and its internal lines align with other private methods (adjust the whitespace for the method name escapeXml and its return/return-chained lines to match the class style).src/InstallationValidator/Views/MainView.cs (1)
101-106: Make the initial Start-button state explicit.Right now the first enabled/disabled state depends on whether
ScreenBinder.BindToSource(...)happens to raiseChanged, becausesetOkButtonEnable()is only called from validation callbacks and the binder event. Recomputing after each initial bind avoids that hidden coupling and makes the no-format-selected guard deterministic on first load.♻️ Proposed change
public void BindTo(FolderDTO outputFolderDTO) { _screenBinder.BindToSource(outputFolderDTO); + setOkButtonEnable(); } public void BindToReportOptions(ReportOptionsDTO reportOptionsDTO) { _reportOptionsDTO = reportOptionsDTO; _reportOptionsBinder.BindToSource(reportOptionsDTO); + setOkButtonEnable(); }Also applies to: 121-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/InstallationValidator/Views/MainView.cs` around lines 101 - 106, The Start button's initial enabled state is implicit; after the initial ScreenBinder.BindToSource(...) call(s) explicitly recompute and set layoutItemButtonStart.Enabled by invoking setOkButtonEnable() (which uses hasReportFormatSelected()/ _reportOptionsDTO) so the no-format-selected guard is applied deterministically on load; apply the same explicit recompute after any initial binds or assignments that affect _reportOptionsDTO (also update the other similar place that mirrors lines 121-125) so the button state doesn't rely on a subsequent Changed event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs`:
- Around line 12-15: The constructor OutputComparisonResultMarkdownBuilder
currently assigns the svgChartGenerator to _svgChartGenerator without
validation; add a fail-fast null check that throws an ArgumentNullException if
svgChartGenerator is null (consistent with PdfReportDocument) so consumers get
an immediate clear error rather than a later failure in GenerateLineChart;
locate the OutputComparisonResultMarkdownBuilder constructor and add the
ArgumentNullException guard for the svgChartGenerator parameter.
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 33-46: The CreateReport signature currently includes an unused
firstFolderPath parameter in MarkdownReportingTask.CreateReport and the
corresponding PdfReportingTask.CreateReport and in the interface; remove
firstFolderPath from the interface method and from both implementations, update
their signatures to CreateReport(BatchComparisonResult comparisonResult, string
secondFolderPath, bool openReport = false), and update all callers; ensure you
only use secondFolderPath when computing reportPath via
reportOutputPath(secondFolderPath, ...) and when calling
writeReportAsync(reportPath, context); also remove any unused local variables or
parameters related to firstFolderPath (e.g., references in createReportContext
or logging) so the code compiles cleanly.
In `@src/SimulationOutputComparer/Views/SimulationComparisonView.cs`:
- Around line 128-136: setOkButtonEnable currently bases enablement on HasError
and hasReportFormatSelected but ignores validation state from _screenBinder, so
include _screenBinder's errors in the gate: update setOkButtonEnable to also
require that _screenBinder has no errors (e.g. ! _screenBinder.HasError or
equivalent API) before enabling layoutItemButtonStart; ensure the check uses the
existing hasReportFormatSelected() and _reportOptionsDTO logic and reference
_screenBinder, HasError, setOkButtonEnable, layoutItemButtonStart and
hasReportFormatSelected so the Start button is disabled when either HasError or
the screen binder indicates validation/conversion errors.
- Around line 172-176: BindToReportOptions currently assigns _reportOptionsDTO
and calls _reportOptionsBinder.BindToSource(reportOptionsDTO) but never
refreshes the Start/OK button state; after those two lines call
setOkButtonEnable() so the Start button is recomputed immediately based on the
newly bound ReportOptionsDTO and binder state (refer to BindToReportOptions and
setOkButtonEnable).
---
Nitpick comments:
In `@src/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cs`:
- Around line 231-240: The private method escapeXml has incorrect indentation
compared to other private methods; reformat its declaration and body to use the
same 6-space indentation used across the class so the method signature and its
internal lines align with other private methods (adjust the whitespace for the
method name escapeXml and its return/return-chained lines to match the class
style).
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs`:
- Around line 93-96: The filename timestamp in reportOutputPath currently uses a
single-digit hour format (H) causing inconsistent lengths; update the DateTime
format string in reportOutputPath (the method building the Path.Combine with
_applicationConfiguration.OSPSuiteNameWithVersion and reportName) to use a
two-digit hour specifier (HH) so hours are zero-padded and filenames sort
consistently.
In `@src/InstallationValidator/Views/MainView.cs`:
- Around line 101-106: The Start button's initial enabled state is implicit;
after the initial ScreenBinder.BindToSource(...) call(s) explicitly recompute
and set layoutItemButtonStart.Enabled by invoking setOkButtonEnable() (which
uses hasReportFormatSelected()/ _reportOptionsDTO) so the no-format-selected
guard is applied deterministically on load; apply the same explicit recompute
after any initial binds or assignments that affect _reportOptionsDTO (also
update the other similar place that mirrors lines 121-125) so the button state
doesn't rely on a subsequent Changed event.
In `@tests/InstallationValidator.Tests/Reporting/MarkdownReportContextSpecs.cs`:
- Around line 6-7: The namespace in this test file is incorrect for its path:
change the namespace declaration from InstallationValidator.Reporting to
InstallationValidator.Tests.Reporting so it matches the test project's folder
structure and naming convention (update the namespace line at the top of the
file where the namespace is declared).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7aa8b0aa-1346-4286-9961-e56e09767345
📒 Files selected for processing (14)
src/InstallationValidator.Core/Assets/Captions.cssrc/InstallationValidator.Core/Reporting/Charts/ChartDataFactory.cssrc/InstallationValidator.Core/Reporting/Charts/ColorExtensions.cssrc/InstallationValidator.Core/Reporting/Charts/SvgChartGenerator.cssrc/InstallationValidator.Core/Reporting/Markdown/MarkdownReportContext.cssrc/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cssrc/InstallationValidator.Core/Reporting/Pdf/PdfReportDocument.cssrc/InstallationValidator.Core/Services/MarkdownReportingTask.cssrc/InstallationValidator.Core/Services/PdfReportingTask.cssrc/InstallationValidator.Core/ValidatorRegister.cssrc/InstallationValidator/Views/MainView.cssrc/SimulationOutputComparer/Views/SimulationComparisonView.cstests/InstallationValidator.Tests/Reporting/MarkdownReportContextSpecs.cstests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/InstallationValidator.Tests/Reporting/MarkdownReportingSpecs.cs
- src/InstallationValidator.Core/Assets/Captions.cs
- src/InstallationValidator.Core/Services/PdfReportingTask.cs
Comment on lines
+12
to
+15
| public OutputComparisonResultMarkdownBuilder(ISvgChartGenerator svgChartGenerator) | ||
| { | ||
| _svgChartGenerator = svgChartGenerator; | ||
| } |
There was a problem hiding this comment.
Add null guard for svgChartGenerator for consistency.
Unlike PdfReportDocument which throws ArgumentNullException for null svgChartGenerator, this constructor silently accepts null and will fail later when GenerateLineChart is called. Fail-fast validation improves debuggability.
Proposed fix
public OutputComparisonResultMarkdownBuilder(ISvgChartGenerator svgChartGenerator)
{
- _svgChartGenerator = svgChartGenerator;
+ _svgChartGenerator = svgChartGenerator ?? throw new ArgumentNullException(nameof(svgChartGenerator));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public OutputComparisonResultMarkdownBuilder(ISvgChartGenerator svgChartGenerator) | |
| { | |
| _svgChartGenerator = svgChartGenerator; | |
| } | |
| public OutputComparisonResultMarkdownBuilder(ISvgChartGenerator svgChartGenerator) | |
| { | |
| _svgChartGenerator = svgChartGenerator ?? throw new ArgumentNullException(nameof(svgChartGenerator)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/InstallationValidator.Core/Reporting/Markdown/OutputComparisonResultMarkdownBuilder.cs`
around lines 12 - 15, The constructor OutputComparisonResultMarkdownBuilder
currently assigns the svgChartGenerator to _svgChartGenerator without
validation; add a fail-fast null check that throws an ArgumentNullException if
svgChartGenerator is null (consistent with PdfReportDocument) so consumers get
an immediate clear error rather than a later failure in GenerateLineChart;
locate the OutputComparisonResultMarkdownBuilder constructor and add the
ArgumentNullException guard for the svgChartGenerator parameter.
Comment on lines
+33
to
+46
| public async Task CreateReport(BatchComparisonResult comparisonResult, string firstFolderPath, string secondFolderPath, bool openReport = false) | ||
| { | ||
| if (comparisonResult == null) | ||
| throw new ArgumentNullException(nameof(comparisonResult)); | ||
|
|
||
| var context = createReportContext(Assets.Reporting.FolderComparison); | ||
|
|
||
| _builderRepository.Report(comparisonResult, context); | ||
|
|
||
| var reportPath = reportOutputPath(secondFolderPath, DateTime.Now, Assets.Reporting.FolderComparison); | ||
| await writeReportAsync(reportPath, context); | ||
|
|
||
| openReportIfRequired(openReport, reportPath); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of CreateReport with BatchComparisonResult to understand caller expectations
rg -n "CreateReport.*BatchComparisonResult" --type=cs -A 3 -B 2
# Check if firstFolderPath appears in any related builder or context
rg -n "firstFolderPath" --type=csRepository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 6755
🏁 Script executed:
sed -n '35,60p' src/InstallationValidator.Core/Services/PdfReportingTask.csRepository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 1264
🏁 Script executed:
# Check the createReportContext method and _builderRepository.Report
sed -n '1,32p' src/InstallationValidator.Core/Services/MarkdownReportingTask.cs
# Check if BatchComparisonResult contains folder paths
rg -n "class BatchComparisonResult" --type=cs -A 15Repository: Open-Systems-Pharmacology/InstallationValidator
Length of output: 5164
Unused parameter firstFolderPath in both MarkdownReportingTask and PdfReportingTask.
The firstFolderPath parameter is declared in the method signatures but never referenced in either implementation. Both classes write the report only to secondFolderPath. The parameter is defined in the interface, so callers must pass it, but it has no effect on the output. Remove the unused parameter from the interface and both implementations, or document and implement its intended use (e.g., in report metadata or comparison logic).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/InstallationValidator.Core/Services/MarkdownReportingTask.cs` around
lines 33 - 46, The CreateReport signature currently includes an unused
firstFolderPath parameter in MarkdownReportingTask.CreateReport and the
corresponding PdfReportingTask.CreateReport and in the interface; remove
firstFolderPath from the interface method and from both implementations, update
their signatures to CreateReport(BatchComparisonResult comparisonResult, string
secondFolderPath, bool openReport = false), and update all callers; ensure you
only use secondFolderPath when computing reportPath via
reportOutputPath(secondFolderPath, ...) and when calling
writeReportAsync(reportPath, context); also remove any unused local variables or
parameters related to firstFolderPath (e.g., references in createReportContext
or logging) so the code compiles cleanly.
Comment on lines
128
to
136
| private void setOkButtonEnable() | ||
| { | ||
| layoutItemButtonStart.Enabled = !HasError; | ||
| layoutItemButtonStart.Enabled = !HasError && hasReportFormatSelected(); | ||
| } | ||
|
|
||
| private bool hasReportFormatSelected() | ||
| { | ||
| return _reportOptionsDTO != null && (_reportOptionsDTO.ExportToPdf || _reportOptionsDTO.ExportToMarkdown); | ||
| } |
There was a problem hiding this comment.
Include _screenBinder errors in the Start-button gate.
setOkButtonEnable() now controls whether comparison can start, but HasError still ignores _screenBinder. That means validation/conversion errors on fields bound through _screenBinder can still leave Start enabled as long as the two folder binders are valid and one report format is selected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SimulationOutputComparer/Views/SimulationComparisonView.cs` around lines
128 - 136, setOkButtonEnable currently bases enablement on HasError and
hasReportFormatSelected but ignores validation state from _screenBinder, so
include _screenBinder's errors in the gate: update setOkButtonEnable to also
require that _screenBinder has no errors (e.g. ! _screenBinder.HasError or
equivalent API) before enabling layoutItemButtonStart; ensure the check uses the
existing hasReportFormatSelected() and _reportOptionsDTO logic and reference
_screenBinder, HasError, setOkButtonEnable, layoutItemButtonStart and
hasReportFormatSelected so the Start button is disabled when either HasError or
the screen binder indicates validation/conversion errors.
This was referenced Jul 16, 2026
Yuri05
pushed a commit
that referenced
this pull request
Jul 16, 2026
TeX/LaTeX reporting was replaced by Markdown + QuestPDF in #325, which dropped the OSPSuite.TeXReporting dependency but left the setup still referencing it, breaking the WiX link step (LGHT0094) on the first 13.0 setup build. Remove the dangling ComponentGroupRef and the obsolete TeXTemplates directory/harvest/copy. Fixes #340
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OSPSuite.TeXReportingand MiKTeXChanges
New Components
IMarkdownReportingTaskandIPdfReportingTaskinterfacesReportFormatenumRemoved
*TeXBuilder.csfiles (13 files)*Reporter.csfilesOSPSuite.TeXReportingdependency from all projectsOSPSuite.Infrastructure.ReportingdependencyDependencies
QuestPDF(MIT license, pure .NET)OSPSuite.TeXReportingTest plan
SvgChartGeneratorCloses #183
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores