feat(html-report): rewrite as split-pane design template

[thomhurst]

Summary

• Replaces the hand-rolled 2.4k-line HTML emitter with an embedded template that ships the new split-pane design: sticky banner with status pill, filterable rail (group/sort/categories), sticky detail pane with Output / Trace / Properties / Source tabs, separate run-view dashboard with donut, duration card, parallel Gantt, failure clustering, retry attempts, and OTel insights.
• Generator now maps ReportData to the renderer's JSON shape, gzip+base64-encodes it (kept small for large suites), and substitutes a single placeholder + the page title.
• Renderer is a <script type="module"> that decompresses via the browser's built-in DecompressionStream.

What changed

• New embedded resource: TUnit.Engine/Reporters/Html/TestReport.template.html (split-pane renderer, all CSS+JS inlined; no external assets beyond Google Fonts).
• HtmlReportGenerator.cs: ~2469 → ~360 lines. Builds the runner's expected JSON via Utf8JsonWriter, derives virtual worker lanes from start/duration (greedy interval coloring), buckets spans by traceId per test, infers span service from tags/source/kind, light-parses Expected: / Actual: / But was: lines into an expected/actual diff payload.
• Tests (HtmlReporterTests): extractor decompresses the gzip+base64 payload and asserts the new schema (parent, properties as object map). All 14 pass.
• Removed: HtmlReportJsonContext.cs (unused now that JSON is written manually).

Test plan

• dotnet build TUnit.Engine clean on all TFMs (netstandard2.0;net8.0;net9.0;net10.0)
• dotnet build TUnit.Engine.Tests clean
• HtmlReporterTests 14/14 pass (net10.0)
• HtmlReporterTruncateOutputTests 7/7 pass (net10.0)
• Generated report opens in a modern browser and renders sample test data
• Visual smoke-test in CI run output once this branch builds

[codacy-production]

Not up to standards ⛔

🔴 Issues 40 minor

Alerts:
⚠ 40 issues (≤ 0 issues of at least minor severity)

Results:
40 new issues

Category	Results
CodeStyle	40 minor

View in Codacy

🟢 Metrics 123 complexity

Metric	Results
Complexity	123

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

This is a substantial and well-executed rewrite. Moving from a 2,400-line imperative HTML emitter to a template+JSON architecture is the right call — the separation of concerns is much cleaner and the new split-pane UI looks genuinely better. The gzip+base64 compression approach is smart for large suites.

That said, there are a few issues worth addressing before merge.

---

1. Fragile title sentinel — use a proper placeholder

HtmlReportGenerator.cs

private const string TemplateTitleMarker = "Test Report — CloudShop.Tests";

This uses a project-specific demo string as the substitution sentinel. Two problems:

• If the template design ever changes (or the preview project is renamed), the constant silently fails to replace anything and every generated report ships with "CloudShop.Tests" as its title.
• It uses a different mechanism than the data placeholder (__REPORT_DATA__), so there are now two ad-hoc substitution patterns in the same pipeline.

Better approach: use a symmetric placeholder like __REPORT_TITLE__ in the template:

<title>__REPORT_TITLE__</title>

private const string TemplateTitleMarker = "__REPORT_TITLE__";

This makes the contract explicit and is immune to template redesigns. The same pattern should also be applied to the <div class="brand-name" id="projectName">CloudShop.Tests</div> element — even if JS updates it at runtime, the SSR text is still wrong until the script runs.

---

2. External Google Fonts CDN breaks offline/air-gapped use

TestReport.template.html

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;450;500;600;700&family=JetBrains+Mono:...&display=swap" rel="stylesheet">

The old design was fully self-contained. This PR reintroduces an external CDN dependency, which means:

• Reports are broken in air-gapped CI environments (common for regulated/security-sensitive projects)
• Google can track when/where reports are opened (privacy concern in enterprise settings)
• The font load is a render-blocking request that adds latency

Recommendation: Inline the WOFF2 data URIs for the handful of weights used, or fall back gracefully to system fonts. The CSS already has a good system-font stack (-apple-system, BlinkMacSystemFont, system-ui), so simply removing the Google Fonts links would be acceptable. Inlining is better for fidelity.

---

3. Demo/sample data ships in every production report

TestReport.template.html

/* ---------- sample data generator ---------- */
function generateSampleData() {
  const classes = [
    { ns: 'CloudShop.Tests.Performance', cls: 'LoadTests', ...

A large generateSampleData() function with hardcoded CloudShop fixture data is embedded in the template and ships in every report written to disk. This is useful for the template development workflow, but it's dead weight in production artifacts (adds kilobytes to every report, confusing if someone inspects the HTML).

Recommendation: Strip the sample data generator before shipping. Either:

• Remove it from the template and keep a separate dev-only file for the preview workflow
• Use a build step to strip /* ---------- sample data generator ---------- */ ... } before embedding

---

4. MapStatus silently maps unknown statuses to "skip"

HtmlReportGenerator.cs

private static string MapStatus(string status) => status switch
{
    "passed" => "pass",
    "failed" or "error" or "timedOut" => "fail",
    "skipped" => "skip",
    "cancelled" => "cancel",
    _ => "skip",  // ← wrong default
};

An unknown status (e.g. "inProgress", "unknown", a future status string) is silently promoted to "skip". This means tests that are in an unknown state appear as skipped in the UI — a misleading outcome that's hard to debug. The discard case should map to something the UI can render distinctly, or at minimum preserve the original string so the JS can apply a fallback style.

---

5. Contradictory comments on duplicate property handling

HtmlReportGenerator.cs

// properties — design schema is an object map; dedupe duplicate keys (last wins).
w.WritePropertyName("properties");
w.WriteStartObject();
if (t.CustomProperties is { Length: > 0 } props)
{
    // First occurrence of a duplicated key wins.
    var emitted = new HashSet<string>(StringComparer.Ordinal);
    foreach (var p in props)
    {
        if (emitted.Add(p.Key)) w.WriteString(p.Key, p.Value);
    }
}

The outer comment says "last wins" but the inner comment and implementation say "first wins". One of these is wrong. The implementation is correct (first wins via HashSet.Add), so the outer comment should be updated.

---

Summary

Severity	Issue
🔴 High	External Google Fonts CDN breaks offline/air-gapped reports
🟠 Medium	Fragile title sentinel using demo project name
🟠 Medium	Demo data (generateSampleData) ships in every production report
🟡 Low	MapStatus default maps unknown → "skip" instead of something distinct
🟢 Nit	Contradictory comments on first-vs-last win for duplicate properties

The architecture is solid and this is a big improvement over the old emitter. Fixing the offline-fonts issue and the title placeholder before merge would make this ready to ship.

[claude]

Follow-up Review (after second commit)

The second commit () fixes real bugs — tooltip overflow, engine-notice stripping from stderr, the view-switch blank-rail race, and Flat-sort ignoring the Sort selector. Good fixes.

However, none of the five issues from my previous review were addressed. Re-stating them in priority order:

---

1. External Google Fonts CDN — still present (High)

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;450;500;600;700&family=JetBrains+Mono:...&display=swap" rel="stylesheet">

This is a regression from the old self-contained emitter. Reports will fail to render correctly in air-gapped CI (common in regulated/enterprise environments), and the fetch gives Google visibility into when and where each report is opened. The system-font fallback stack in the CSS is already reasonable — the simplest fix is to remove these three lines entirely.

---

2. Demo sample data ships in every production report — still present (Medium)

/* ---------- sample data generator ---------- */
function generateSampleData() {
    const classes = [
        { ns: 'CloudShop.Tests.Performance', cls: 'LoadTests', ...

Line 1243 of the template: ~300 lines of CloudShop fixture data is embedded in every generated report. This isn't a correctness problem (line 1526 guards with if (!REPORT) REPORT = generateSampleData()), but it's dead weight in production artifacts — extra kilobytes in every file on disk, and confusing to anyone who inspects the HTML source.

---

3. Fragile title sentinel — still present (Medium)

private const string TemplateTitleMarker = "Test Report — CloudShop.Tests";

<title>Test Report — CloudShop.Tests</title>          <!-- line 6 -->
<div class="brand-name" id="projectName">CloudShop.Tests</div>  <!-- line 999 -->

Still using a project-specific demo string as the substitution key. If the template title ever changes for a redesign, the constant silently breaks and every report ships with "CloudShop.Tests". A symmetric __REPORT_TITLE__ placeholder is safer and more obviously intentional. Note also that line 999's brand-name div still hard-codes "CloudShop.Tests" and is not substituted server-side (it relies on JS to overwrite it at runtime).

---

4. MapStatus unknown → "skip" — still present (Low)

_ => "skip",   // line 401

Still silently maps any future or unknown status to "skip", causing tests to appear as skipped in the UI when they are actually in an unknown/in-progress state.

---

5. Contradictory comments — still present (Nit)

// properties — design schema is an object map; dedupe duplicate keys (last wins).  // line 195
...
// First occurrence of a duplicated key wins.                                         // line 200

One of these must be wrong. The code implements first-wins. The outer comment should say "first wins".

---

Summary of unaddressed items

#	Severity	Issue	Status
1	High	External Google Fonts CDN	❌ Not fixed
2	Medium	Demo generateSampleData in every production report	❌ Not fixed
3	Medium	Fragile title sentinel using "CloudShop.Tests"	❌ Not fixed
4	Low	MapStatus maps unknown → "skip"	❌ Not fixed
5	Nit	Contradictory "last wins" / "first wins" comments	❌ Not fixed

The architecture and the second-commit bug fixes are both good. Issues 1–3 are worth resolving before merge.

[claude]

Follow-up Review (after second commit)

The second commit (8d1750ed) fixes real bugs — tooltip overflow, engine-notice stripping from stderr, the view-switch blank-rail race, and Flat-sort ignoring the Sort selector. Good fixes.

However, none of the five issues from my previous review were addressed. Re-stating them in priority order:

---

1. External Google Fonts CDN — still present (High)

<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;450;500;600;700&family=JetBrains+Mono:...&display=swap" rel="stylesheet">

This is a regression from the old self-contained emitter. Reports will fail to render correctly in air-gapped CI (common in regulated/enterprise environments), and the fetch gives Google visibility into when and where each report is opened. The system-font fallback stack in the CSS is already reasonable — the simplest fix is to remove these three lines entirely.

---

2. Demo sample data ships in every production report — still present (Medium)

/* ---------- sample data generator ---------- */
function generateSampleData() {
    const classes = [
        { ns: 'CloudShop.Tests.Performance', cls: 'LoadTests', ...

Line 1243 of the template: ~300 lines of CloudShop fixture data is embedded in every generated report. This isn't a correctness problem (line 1526 guards with if (!REPORT) REPORT = generateSampleData()), but it's dead weight in production artifacts — extra kilobytes in every file on disk, and confusing to anyone who inspects the HTML source.

---

3. Fragile title sentinel — still present (Medium)

private const string TemplateTitleMarker = "Test Report — CloudShop.Tests";

<title>Test Report — CloudShop.Tests</title>          <!-- line 6 -->
<div class="brand-name" id="projectName">CloudShop.Tests</div>  <!-- line 999 -->

Still using a project-specific demo string as the substitution key. If the template title ever changes for a redesign, the constant silently breaks and every report ships with "CloudShop.Tests". A symmetric __REPORT_TITLE__ placeholder is safer and more obviously intentional. Note also that line 999's brand-name div still hard-codes "CloudShop.Tests" and is not substituted server-side (it relies on JS to overwrite it at runtime).

---

4. MapStatus unknown → "skip" — still present (Low)

_ => "skip",   // line 401

Still silently maps any future or unknown status to "skip", causing tests to appear as skipped in the UI when they are actually in an unknown/in-progress state.

---

5. Contradictory comments — still present (Nit)

// properties — design schema is an object map; dedupe duplicate keys (last wins).  // line 195
...
// First occurrence of a duplicated key wins.                                         // line 200

One of these must be wrong. The code implements first-wins. The outer comment should say "first wins".

---

Summary of unaddressed items

#	Severity	Issue	Status
1	High	External Google Fonts CDN	❌ Not fixed
2	Medium	Demo generateSampleData in every production report	❌ Not fixed
3	Medium	Fragile title sentinel using "CloudShop.Tests"	❌ Not fixed
4	Low	MapStatus maps unknown → "skip"	❌ Not fixed
5	Nit	Contradictory "last wins" / "first wins" comments	❌ Not fixed

The architecture and the second-commit bug fixes are both good. Issues 1–3 are worth resolving before merge.

[claude]

Follow-up Review — All Previous Issues Resolved

All five items from the previous two reviews have been addressed in this latest commit.

---

Previous issues — status update

#	Severity	Issue	Status
1	High	External Google Fonts CDN	✅ Fixed — no CDN references in template
2	Medium	Demo generateSampleData in every production report	✅ Fixed — wrapped in SAMPLE_DATA_BEGIN/SAMPLE_DATA_END markers; LoadAndStripTemplate() removes the block before embedding
3	Medium	Fragile title sentinel using CloudShop.Tests	✅ Fixed — __REPORT_TITLE__ on line 6, __REPORT_PROJECT__ on line 996; both substituted server-side
4	Low	MapStatus unknown → "skip"	✅ Fixed — default now "fail" (keeps unknown tests visible)
5	Nit	Contradictory "last wins" / "first wins" comments	✅ Fixed — both comments now consistently say "first wins"

---

New code — one minor nit

In several JS template literals, t.id is interpolated into data-id attributes without esc():

// e.g. line 1694
`<div class="tick ${t.status}" data-id="${t.id}"></div>`

Since test IDs are server-generated identifiers (not user text), this isn't a practical XSS risk — but it's inconsistent with the rest of the template which uses esc() for all output. Worth a follow-up pass, though not a blocker.

---

The architecture is solid, the strip-and-embed pattern is clean, and the new split-pane UI is a genuine improvement. Good to merge from a review standpoint.

[claude]

Code Review

Overall this is a very well-executed refactor. Replacing a 2,469-line hand-rolled string concatenation monster with a template-based approach is exactly the right call — separation of concerns between data serialization and rendering is now clear, and the generator is readable at ~360 lines. The Utf8JsonWriter approach is also better for AOT than the removed HtmlReportJsonContext.cs source-gen path.

A few things worth addressing:

---

Silently lenient sample-data stripping (concern)

private static string LoadAndStripTemplate()
{
    var raw = LoadTemplate();
    var begin = raw.IndexOf(SampleDataBeginMarker, StringComparison.Ordinal);
    if (begin < 0) return raw;
    var end = raw.IndexOf(SampleDataEndMarker, begin, StringComparison.Ordinal);
    if (end < 0) return raw;   // ← silently ships sample data if end marker missing
    return raw.Remove(begin, end + SampleDataEndMarker.Length - begin);
}

If SAMPLE_DATA_BEGIN exists but SAMPLE_DATA_END is missing (e.g. due to a template edit), the method returns the full raw template including the sample data block. This could silently bloat every generated report. An InvalidOperationException here (or at least a Debug.Fail) would catch the regression early rather than letting it ship quietly.

---

Hidden coupling between two iteration passes over data.Groups

// Pass 1 — populate rawStarts indexed by sequential counter
var ti = 0;
foreach (var g in data.Groups)
    foreach (var t in g.Tests)
        rawStarts[ti++] = (t.Id, sms, t.DurationMs);

// Pass 2 — consume rawStarts by the same sequential counter
var idx = 0;
foreach (var g in data.Groups)
    foreach (var t in g.Tests)
    {
        var abs = rawStarts[idx++].AbsStartMs;  // assumes identical iteration order

This is correct today because data is not mutated, but it's a silent invariant that a future refactor (e.g. deduplicating the nested loop, adding a LINQ query in between) will break without any compile-time warning. A dictionary keyed by t.Id — already used for testWorker — would be safer and nearly as fast:

var startByTestId = new Dictionary<string, long?>(totalTests, StringComparer.Ordinal);
// fill during pass 1, then look up by t.Id in pass 2

---

External font dependency in a "self-contained" report

The PR description says "no external assets beyond Google Fonts." For test reports generated in air-gapped CI environments, or pages served with a strict CSP (blocking third-party origins), the Google Fonts import will fail silently. The font fallbacks in the CSS will kick in, so the UI won't break, but users in those environments will never get the intended font. If self-containment is a goal it's worth mentioning this explicitly in docs/comments, or providing a build-time opt-in to drop the import.

---

Qualified namespace in GzipBase64 while using short names elsewhere

using (var gz = new System.IO.Compression.GZipStream(output, System.IO.Compression.CompressionLevel.Optimal, leaveOpen: true))

The file already has using System.IO; and the rest of the file uses unqualified names. The old code handled SmallestSize vs Optimal with #if NET, but CompressionLevel.Optimal is fine for all targets — the qualification just looks like an oversight. A simple using System.IO.Compression; import cleans this up.

---

Positive callouts

• The greedy interval coloring for Gantt lane derivation is well-commented and algorithmically correct (laneEnd[i] <= startRel + 0.5 tolerance handles floating-point skew).
• FilterEngineNotices is a clean, targeted filter that avoids false-positive suppression by checking for [TUnit] prefix before allocating.
• MapStatus correctly maps future unknown statuses to fail (visible) rather than skip (buried) — the comment explains the rationale.
• Removing HtmlReportJsonContext.cs and switching to Utf8JsonWriter directly is strictly better for AOT compatibility and eliminates a source-gen dependency.
• Properties serialized as an object map ({"key":"value"}) rather than an array of pairs is a better schema design for JS consumers.

---

The silently-lenient template stripping and the sequential-index coupling are the two items I'd want fixed before merge. The rest are minor observations.

[claude]

Code Review — feat(html-report): rewrite as split-pane design template

Overall, this is an excellent refactor. Replacing 2.4k lines of hand-rolled HTML/CSS/JS in C# with an embedded template is the right architectural move — the old approach forced every UI change through string-builder hell. The new separation (C# serialises data → template renders it) is much more maintainable. 14/14 tests passing and a clean 85% line reduction on the generator speak for themselves.

---

Issues Worth Addressing

1. Fragile parallel-index coupling between the two iteration passes

In SerializeReport, rawStarts is populated in pass 1 and consumed in pass 2 using independent counters (ti, then idx):

// Pass 1
var rawStarts = new (string Id, long? AbsStartMs, double Dur)[totalTests];
var ti = 0;
foreach (var g in data.Groups)
    foreach (var t in g.Tests)
        rawStarts[ti++] = (t.Id, sms, t.DurationMs);

// Pass 2 — must traverse in identical order
var idx = 0;
foreach (var g in data.Groups)
    foreach (var t in g.Tests) {
        var abs = rawStarts[idx++].AbsStartMs;  // <-- implicit contract: same order
        ...
    }

This works today because both loops enumerate the same immutable arrays. But the contract is invisible to future maintainers — any filtering, sorting, or pagination inserted between the two passes silently produces wrong start times. A Dictionary<string, long?> keyed by test ID removes this footgun entirely with negligible allocation overhead:

var absStartByTestId = new Dictionary<string, long?>(totalTests, StringComparer.Ordinal);
foreach (var g in data.Groups)
    foreach (var t in g.Tests)
        absStartByTestId[t.Id] = TryParseUnixMs(t.StartTime);

// Pass 2 — now order-independent
foreach (var g in data.Groups)
    foreach (var t in g.Tests) {
        var abs = absStartByTestId.GetValueOrDefault(t.Id);
        ...
    }

2. Google Fonts is an external dependency — privacy and offline regression

The PR summary says no external assets beyond Google Fonts, but that exception is significant for a test report tool:

• CI/CD environments frequently block outbound traffic; the font request will time out and the page will fall back to system fonts, looking broken on first render.
• Google Fonts requests carry an IP address, meaning every report viewer is tracked by Google — a problem for air-gapped/regulated environments.
• The old report had zero external dependencies (self-contained).

Recommendation: Inline the font subset as a base64 @font-face data URL in the template CSS, or drop the Google Fonts <link> and rely on the font-family fallback stack that's already defined in --font. The system-font stack already includes 'Segoe UI Variable', -apple-system, etc. — it looks fine without a custom font.

---

Smaller Observations

GetBuffer() usage is correct but worth a note: Both GzipBase64 and SerializeReport use ms.GetBuffer() with an explicit length argument. This is correct (it avoids a copy), but is easy to misuse. The code is fine as-is.

SampleDataBeginMarker = "/* SAMPLE_DATA_BEGIN" — the intentionally unclosed comment marker is a slightly unusual pattern. A HTML comment (<!--SAMPLE_DATA_BEGIN-->) or a more obvious sentinel would be easier to search for in the template file, but this is minor.

classModes lookup in WriteTimelines iterates all tests in every group searching for the tunit.report.timeline property. Since this is called once per report generation (not a hot path), it's fine.

attempts only populated for retried tests (Count > 1): This is correct — single-attempt tests don't need the array — but worth confirming the renderer handles null attempts gracefully (it appears to, based on the JS t.spans || [] pattern).

---

Test Coverage Note

There's no test for the sample-data stripping logic in LoadAndStripTemplate. Given the marker-based stripping could silently no-op if the marker is renamed in the template, a single test asserting the shipped HTML does not contain generateSampleData (or the begin marker) would prevent a class of invisible regressions.

---

Summary

The refactor is solid and a clear improvement. The two items I'd call out before merging are the parallel-index coupling (correctness risk under future refactoring) and the Google Fonts external dependency (offline/privacy regression). Everything else is a low-priority suggestion. Great work on the architecture and test update.