HTML Report: Add source link and code snippet to Source tab

[Copilot]

The HTML report's Source tab only showed file path and line number. This adds a GitHub source link and a dynamically-fetched code snippet when CI context is available.

Changes

• Data model (HtmlReportDataModel.cs): Added EndLineNumber, SourceRelativePath, ServerUrl to ReportTestResult/ReportData
• Path computation (HtmlReporter.cs): ComputeSourceRelativePath() strips GITHUB_WORKSPACE to get repo-relative paths (mirrors GitHubReporter.GetSourceLink logic). GetCiContext() now also returns GITHUB_SERVER_URL for Enterprise support.
• JSON serialization (HtmlReportGenerator.cs): Emits source.endLine, source.relativePath, and top-level serverUrl
• HTML template:
  • "Jump to source ↗" link pointing to {serverUrl}/{repo}/blob/{sha}/{path}#L{start}-L{end}
  • Lazy-loaded code snippet fetched from raw.githubusercontent.com (or Enterprise raw URL) on tab activation, with per-file caching
  • Graceful degradation: features hidden when not in GitHub Actions; shows "Source unavailable" on fetch failure

Behavior

Context	Source tab shows
GitHub Actions CI	File path, jump-to-source link, fetched code snippet
Local / non-GitHub CI	File path and line number only (unchanged)
Fetch failure (private repo, no network)	File path, link, "Source unavailable" message

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 3 medium · 3 minor

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

Results:
7 new issues

Category	Results
ErrorProne	1 high
CodeStyle	3 minor
Performance	3 medium

View in Codacy

🟢 Metrics 85 complexity

Metric	Results
Complexity	85

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

The feature itself is a nice addition — surfacing a "Jump to source" link and an inline code snippet directly in the HTML report's Source tab is genuinely useful for CI workflows. The overall architecture is sound and the graceful-degradation story (local / non-GitHub / private repo) is well thought out. I have a handful of issues to raise, ranging from a correctness bug to design/maintainability concerns.

---

1. Bug: FindExpressionBodyEndLine does not skip strings when searching for ;

The inner scan loop searches for the first semicolon on any line without skipping string literals or comments:

for (var j = i; j < lines.Length; j++)
{
    if (lines[j].IndexOf(';') >= 0)
        return j + 1;
}

This fires on a semicolon that appears inside a string, e.g.:

public string Message() => "hello; world";

The semicolon at "hello; world" would be found on the same line as the expression body, so it happens to return the correct line here — but a multi-line case like:

public string Message() =>
    "first line;" +      // ← spurious semicolon, wrong end-line returned
    "second";

would report an end-line one too early. FindMatchingBraceEndLine correctly calls SkipQuotedLiteral for string tokens; FindExpressionBodyEndLine should do the same.

Also note: the expression-body inner loop starts from line i (the line containing =>), so if the => and the ; are on the same line the return value is i + 1, which is correct. But this is subtle and would break if a future refactor changed the semantics.

---

2. Design: GITHUB_SERVER_URL and GITHUB_WORKSPACE are raw string literals — should be constants

EnvironmentConstants already centralises every other GitHub Actions variable (GITHUB_ACTIONS, GITHUB_SHA, GITHUB_REF, etc.). The two new variables are used as raw strings in three different places:

• HtmlReporter.cs line 434 (GetCiContext)
• HtmlReporter.cs line 870 (ComputeSourceRelativePath)
• GitHubReporter.cs line 278–279 (pre-existing, but this PR duplicates the same pattern)

Adding GitHubServerUrl = "GITHUB_SERVER_URL" and GitHubWorkspace = "GITHUB_WORKSPACE" to EnvironmentConstants is a one-liner and keeps the codebase consistent. A typo in any of the three call-sites would be a silent runtime failure.

---

3. Design: Source-path computation duplicated across HtmlReporter and GitHubReporter

ComputeSourceRelativePath (new, in HtmlReporter.cs lines 850–884) is nearly identical to the path-stripping logic inside GitHubReporter.GetSourceLink (lines 596–611). Both:

1. Normalise backslashes to forward slashes.
2. Try GITHUB_WORKSPACE-prefix stripping.
3. Fall back to repo-name substring matching.

The only difference is that GetSourceLink returns a full Markdown hyperlink, while the new code returns just the relative path. Extracting the path-stripping step into a shared static helper (e.g. GitHubPathHelper.GetRepoRelativePath(string filePath, string workspace, string repo)) would eliminate the duplication and make the fallback logic easier to test and evolve in one place.

---

4. Design: Static SourceLinesCache is never cleared — unbounded memory growth

private static readonly ConcurrentDictionary<string, string[]> SourceLinesCache = new(...);

Every source file is read and retained for the lifetime of the process. For a large test suite with many test files, this can hold a non-trivial amount of memory. More importantly, because the field is static, it survives across multiple test sessions in the same process (e.g., dotnet test with --no-build in watch mode, or when the test host is reused). A simple upper-bound (e.g., cap at N files, or clear the dictionary once BuildReportData is called) would be safer.

---

5. Correctness: ComputeSourceRelativePath reads environment variables once per test call

ComputeSourceRelativePath is called from ExtractTestResult, which runs once per test node. Inside it, Environment.GetEnvironmentVariable is called twice on every invocation (for GITHUB_ACTIONS and GITHUB_REPOSITORY). These variables are constant for the duration of a run. The same pattern was explicitly avoided in GitHubReporter (see the "Cache env vars for source links (read once, not per test)" comment at line 275). The fix is to read them once in BuildReportData and pass them through, or to cache them in a field alongside the other _ciContext state.

---

6. Correctness: ResolveSourceEndLine only falls through to the inferred value when reportedEndLine == startLine

private static int? ResolveSourceEndLine(string? filePath, int startLine, int reportedEndLine, string methodName)
{
    if (reportedEndLine > startLine)
        return reportedEndLine;

    return TryInferSourceEndLine(filePath, startLine, methodName)
           ?? (reportedEndLine > 0 ? reportedEndLine : null);
}

When reportedEndLine == startLine (the common case for attribute-pointing metadata), TryInferSourceEndLine is called. If inference returns null (file not found, parse failure, etc.), the fallback returns reportedEndLine only if it is > 0. Since line numbers are 1-based in the property, this is almost always true, so the net effect is fine — but the condition reportedEndLine > 0 is redundant given startLine was already checked to be non-null at the call-site. Minor, but worth tidying for clarity.

---

7. Minor: HTML template uses inline style= on a flex container

<div style="display:flex;gap:8px;align-items:center;">

The surrounding elements all use CSS classes. This one inline style is inconsistent with the rest of the template's approach. Worth extracting to .source-actions or similar — especially since it may need media-query overrides on narrow screens.

---

Summary

#	Severity	Area
1	Bug	FindExpressionBodyEndLine misidentifies end-line when ; appears inside a string in a multi-line expression body
2	Maintainability	GITHUB_SERVER_URL / GITHUB_WORKSPACE should be EnvironmentConstants entries
3	Maintainability	Path-stripping logic duplicated between HtmlReporter and GitHubReporter
4	Performance/Correctness	Unbounded static SourceLinesCache — should be capped or cleared after the report is generated
5	Performance	ComputeSourceRelativePath reads environment variables on every test call; should be read once
6	Minor	Redundant reportedEndLine > 0 guard in ResolveSourceEndLine
7	Minor	Inline style in HTML template inconsistent with class-based approach

The most important fixes are items 1, 3, and 4. Items 2 and 5 are low-effort, high-value cleanup. The feature otherwise works correctly for the documented scenarios.

[claude]

Follow-up Review

The previous Claude review flagged five issues. None of them appear to have been addressed in this PR. Restating the most impactful ones and adding two new findings.

---

Previously flagged — still unresolved

**1. Bug: does not skip string literals when scanning for ** (originally raised as issue #1)

The inner scan:

for (var j = i; j < lines.Length; j++)
{
    if (lines[j].IndexOf(';') >= 0)
        return j + 1;
}

will misfire on multi-line expression bodies where an intermediate line contains ; inside a string literal:

public string Method() =>
    "first; part" +   // ← wrong end-line returned here
    "second";

FindMatchingBraceEndLine correctly calls SkipQuotedLiteral — FindExpressionBodyEndLine should do the same.

---

2. Design: GITHUB_WORKSPACE / GITHUB_SERVER_URL are raw string literals (originally raised as issue #2)

Both new variables are used as raw strings in HtmlReporter.cs while every other GitHub Actions env var goes through EnvironmentConstants. A typo is a silent runtime failure. Adding two constants is a one-liner fix.

---

3. Design: Path-stripping logic is duplicated between HtmlReporter and GitHubReporter (originally raised as issue #3)

ComputeSourceRelativePath in HtmlReporter.cs is nearly identical to the workspace-stripping in GitHubReporter.GetSourceLink. Both normalise backslashes, try GITHUB_WORKSPACE prefix stripping, then fall back to repo-name substring matching. This should be extracted to a shared helper.

---

4. Design: SourceLinesCache is a static unbounded cache (originally raised as issue #4)

private static readonly ConcurrentDictionary<string, string[]> SourceLinesCache = new(...);

Every source file read during discovery is held for the lifetime of the process. In watch-mode scenarios this can accumulate across multiple test runs. A simple cap (e.g., discard entries after BuildReportData completes, or limit to N files) avoids unbounded growth.

---

5. Performance: ComputeSourceRelativePath reads env vars per test (originally raised as issue #5)

Environment.GetEnvironmentVariable is called on every invocation of ExtractTestResult. These values are constant for a run. The existing _ciContext pattern in the class already shows the right approach — compute once, reuse.

---

New findings

6. Bug: SkipQuotedLiteral does not handle multi-line verbatim strings

FindMatchingBraceEndLine calls SkipQuotedLiteral to skip past string literals, but the helper only processes within a single line. A verbatim string that spans multiple lines:

var s = @"line with {brace
on next line}";

will cause the brace on the second line to be counted as a real }, potentially terminating the brace-matching algorithm at the wrong line. The fix is either to track verbatim-string state across lines in FindMatchingBraceEndLine, or to document that multi-line verbatim strings are a known limitation.

---

7. Design: GetCiContext() 5-tuple is a maintenance smell

The return type has grown from a 2-tuple to a 5-tuple:

private static (string? CommitSha, string? Branch, string? PullRequestNumber, string? RepositorySlug, string? ServerUrl) GetCiContext()

This is a sign the abstraction boundary is wrong. A small CiContext record would make this readable, allow positional members to be added without reordering risk, and give the type a name that future code can reference.

---

What's good

The core feature is well-conceived: the graceful-degradation table in the PR description accurately describes the actual code behaviour. The lazy-loading with per-file caching in the JS, the esc() usage for XSS prevention, and the brace-matching logic in FindMatchingBraceEndLine (block comment handling, string skipping) are all solid. The new tests in TestNodeLocationTests.cs covering both block-bodied and expression-bodied methods are a nice self-referential touch.

The issues above are worth fixing before merge — particularly items 1, 4, and 6 which affect correctness and memory.

[claude]

Follow-up Review

The previous Claude review flagged five issues. None of them appear to have been addressed in this PR. Restating the most impactful ones and adding two new findings.

---

Previously flagged — still unresolved

1. Bug: FindExpressionBodyEndLine does not skip string literals when scanning for semicolons (originally raised as issue #1)

The inner scan will misfire on multi-line expression bodies where an intermediate line contains a semicolon inside a string literal. FindMatchingBraceEndLine correctly calls SkipQuotedLiteral — FindExpressionBodyEndLine should do the same.

Example that breaks:

public string Method() =>
    "first; part" +   // wrong end-line returned here
    "second";

---

2. Design: GITHUB_WORKSPACE / GITHUB_SERVER_URL are raw string literals (originally raised as issue #2)

Both new variables are used as raw strings in HtmlReporter.cs while every other GitHub Actions env var goes through EnvironmentConstants. A typo is a silent runtime failure. Adding two constants is a one-liner fix.

---

3. Design: Path-stripping logic is duplicated between HtmlReporter and GitHubReporter (originally raised as issue #3)

ComputeSourceRelativePath in HtmlReporter.cs is nearly identical to the workspace-stripping in GitHubReporter.GetSourceLink. Both normalise backslashes, try GITHUB_WORKSPACE prefix stripping, then fall back to repo-name substring matching. This should be extracted to a shared helper.

---

4. Design: SourceLinesCache is a static unbounded cache (originally raised as issue #4)

private static readonly ConcurrentDictionary<string, string[]> SourceLinesCache = new(...);

Every source file read during discovery is held for the lifetime of the process. In watch-mode scenarios this can accumulate across multiple test runs. A simple cap (e.g., discard entries after BuildReportData completes, or limit to N files) avoids unbounded growth.

---

5. Performance: ComputeSourceRelativePath reads env vars per test (originally raised as issue #5)

Environment.GetEnvironmentVariable is called on every invocation of ExtractTestResult. These values are constant for a run. The existing _ciContext pattern in the class already shows the right approach — compute once, reuse.

---

New findings

6. Bug: SkipQuotedLiteral does not handle multi-line verbatim strings

FindMatchingBraceEndLine calls SkipQuotedLiteral to skip past string literals, but the helper only processes within a single line. A verbatim string that spans multiple lines will cause braces on subsequent lines to be counted as real braces, potentially terminating the brace-matching algorithm at the wrong line.

var s = @"line with {brace
on next line}";

The fix is either to track verbatim-string state across lines in FindMatchingBraceEndLine, or to document that multi-line verbatim strings are a known limitation.

---

7. Design: GetCiContext() 5-tuple is a maintenance smell

The return type has grown from a 2-tuple to a 5-tuple. A small CiContext record would make this readable, allow members to be added without reordering risk, and give the type a name that future code can reference.

---

What's good

The core feature is well-conceived: the graceful-degradation table in the PR description accurately describes the actual code behaviour. The lazy-loading with per-file caching in the JS, the esc() usage for XSS prevention, and the brace-matching logic in FindMatchingBraceEndLine (block comment handling, string skipping) are all solid. The new tests in TestNodeLocationTests.cs covering both block-bodied and expression-bodied methods are a nice self-referential touch.

The issues above are worth fixing before merge — particularly items 1, 4, and 6 which affect correctness and memory.

[claude]

Follow-up Review (3rd pass)

The only change since the last review is commit 0795f952 — Restore CloudShop source generation — which adds TUnit.Core.SourceGenerator as an analyzer reference to CloudShop.Tests.csproj. That change is reasonable and correct.

All seven issues flagged in the previous two reviews remain unresolved. Summarising below with the short fix for each.

---

Still open: Bugs

#1 — FindExpressionBodyEndLine does not skip string literals when scanning for ;

// Current (SourceLocationResolver.cs) — fires on ; inside a string
for (var j = i; j < lines.Length; j++)
{
    if (lines[j].IndexOf(';') >= 0)
        return j + 1;
}

Fix: reuse the existing SkipQuotedLiteral / IsEscaped helpers already present in FindMatchingBraceEndLine to skip past string literals before checking for ;.

#6 — SkipQuotedLiteral does not handle multi-line verbatim strings

FindMatchingBraceEndLine calls SkipQuotedLiteral per-line, which means a verbatim string literal spanning multiple lines (e.g. @"... { ... \n ... }") will let the brace on the continuation line be counted as a real brace. Fix: track inVerbatimString state across lines the same way inBlockComment is already tracked.

---

Still open: Design / Maintainability

#2 — GITHUB_SERVER_URL and GITHUB_WORKSPACE are raw string literals

Every other GitHub Actions env var goes through EnvironmentConstants. Two new raw string literals in HtmlReporter.cs are a typo-waiting-to-happen. Add:

internal static class EnvironmentConstants
{
    public const string GitHubServerUrl = "GITHUB_SERVER_URL";
    public const string GitHubWorkspace = "GITHUB_WORKSPACE";
    // ...
}

#3 — Path-stripping logic duplicated between HtmlReporter and GitHubReporter

ComputeSourceRelativePath (new) and GitHubReporter.GetSourceLink (existing) both normalise backslashes, strip GITHUB_WORKSPACE, then fall back to repo-name substring search. Extract to a shared GitHubPathHelper.GetRepoRelativePath(string filePath) helper so both reporters use the same code path.

#7 — GetCiContext() 5-tuple is a maintenance smell

private static (string? CommitSha, string? Branch, string? PullRequestNumber, string? RepositorySlug, string? ServerUrl) GetCiContext()

A small record struct CiContext with named properties is easier to extend safely (no positional-reordering bugs) and gives the type a discoverable name.

---

Still open: Performance / Memory

#4 — SourceLinesCache is a static, unbounded cache

private static readonly ConcurrentDictionary<string, string[]> SourceLinesCache = new(...);

In watch-mode or host-reuse scenarios this accumulates indefinitely. The simplest fix is to clear the cache after BuildReportData() returns (the data is already serialised at that point, so the in-memory lines are no longer needed).

#5 — ComputeSourceRelativePath calls Environment.GetEnvironmentVariable per test

These values are constant for the lifetime of a run. The existing _ciContext caching pattern in HtmlReporter already shows the right approach — read once in BuildReportData, pass through.

---

What's good

The core feature design (graceful degradation, lazy JS loading with per-file cache, XSS-safe esc() usage, block-comment tracking in FindMatchingBraceEndLine) is solid. The self-referential tests in TestNodeLocationTests.cs are a nice touch. Issues 1, 4, and 6 are the ones that matter most before merge.