fix: address netcheck webview review feedback

- Fold netcheck_err/section errors into overall severity so the banner,
  telemetry, and Issues list agree
- Distinguish an undetermined port mapping ("Unknown") from "None detected"
- Treat PreferredDERP 0 as no preferred region in the regions table
- Offer the raw output via a "View Output" action when parse/display fails
- Record diagnostic success only after the panel shows
- Rename display -> parseAndDisplay; align the display-failure message with the log
- Share the View JSON / empty / error DOM builders via @repo/webview-shared
- Drop redundant export/satisfies, extract the latency constant, unify table headers
- Add tests for severity, port mapping, the preferred sentinel, page render,
  shared DOM builders, runDiagnosticCli branches, and the netcheck handler

Author: EhabY

packages/netcheck/src/connectivity.ts

@@ -68,14 +68,23 @@ function boolItem(
 }
 
 function portMappingItem(probe: NetcheckConnectivity): ConnectivityItem {
-	const protocols = [
-		probe.UPnP ? "UPnP" : undefined,
-		probe.PMP ? "NAT-PMP" : undefined,
-		probe.PCP ? "PCP" : undefined,
-	].filter((p): p is string => p !== undefined);
-	return protocols.length > 0
-		? { label: "Port mapping", value: protocols.join(", "), tone: "good" }
-		: { label: "Port mapping", value: "None detected", tone: "neutral" };
+	const fields = [
+		[probe.UPnP, "UPnP"],
+		[probe.PMP, "NAT-PMP"],
+		[probe.PCP, "PCP"],
+	] as const;
+	const detected = fields.filter(([on]) => on).map(([, name]) => name);
+	if (detected.length > 0) {
+		return { label: "Port mapping", value: detected.join(", "), tone: "good" };
+	}
+	// A null field means "could not determine", so report "None detected" only
+	// once a protocol was actually probed.
+	const probed = fields.some(([on]) => typeof on === "boolean");
+	return {
+		label: "Port mapping",
+		value: probed ? "None detected" : "Unknown",
+		tone: "neutral",
+	};
 }
 
 function preferredRegionName(report: NetcheckReport): string | undefined {

packages/netcheck/src/format.ts

@@ -2,6 +2,9 @@ export type TriState = "yes" | "no" | "unknown";
 
 const NANOS_PER_MS = 1_000_000;
 
+/** Below this, show one decimal; at or above, round to whole ms. */
+const DECIMAL_PRECISION_BELOW_MS = 100;
+
 export function nanosToMs(nanos: number): number {
 	return nanos / NANOS_PER_MS;
 }
@@ -13,7 +16,7 @@ export function formatLatency(ms: number | undefined): string {
 	if (ms < 1) {
 		return "<1 ms";
 	}
-	if (ms < 100) {
+	if (ms < DECIMAL_PRECISION_BELOW_MS) {
 		return `${ms.toFixed(1)} ms`;
 	}
 	return `${Math.round(ms)} ms`;

packages/netcheck/src/index.ts

@@ -1,9 +1,13 @@
 import { NetcheckApi, toError, type NetcheckData } from "@repo/shared";
-import { sendCommand, subscribeNotifications } from "@repo/webview-shared";
+import {
+	errorMessage,
+	sendCommand,
+	subscribeNotifications,
+} from "@repo/webview-shared";
 import "@repo/webview-shared/base.css";
 
 import "./index.css";
-import { renderError, renderPage } from "./page";
+import { renderPage } from "./page";
 
 function main(): void {
 	// The extension re-sends `data` on visibility/theme changes; each render
@@ -26,7 +30,7 @@ function render(data: NetcheckData): void {
 		);
 	} catch (err) {
 		root.replaceChildren(
-			renderError(`Failed to render network check: ${toError(err).message}`),
+			errorMessage(`Failed to render network check: ${toError(err).message}`),
 		);
 	}
 }

packages/netcheck/src/page.ts

@@ -5,6 +5,7 @@ import {
 	type NetcheckReport,
 	type NetcheckSeverity,
 } from "@repo/shared";
+import { emptyMessage, viewJsonAction } from "@repo/webview-shared";
 
 import { buildConnectivityItems } from "./connectivity";
 import { formatLatency, formatTriState } from "./format";
@@ -34,7 +35,7 @@ export function renderPage(
 			"Local interfaces",
 			renderInterfaces(report.interfaces.interfaces),
 		),
-		renderActions(onViewJson),
+		viewJsonAction(onViewJson),
 	);
 	return children;
 }
@@ -178,10 +179,10 @@ function renderTable<T>(
 	return table;
 }
 
-function renderTableHead(...labels: string[]): HTMLElement {
+function renderTableHead(...headers: string[]): HTMLElement {
 	const thead = el("thead");
 	const tr = el("tr");
-	tr.append(...labels.map((label) => el("th", undefined, label)));
+	tr.append(...headers.map((header) => el("th", undefined, header)));
 	thead.append(tr);
 	return thead;
 }
@@ -197,19 +198,6 @@ function renderSeverityCell(severity: NetcheckSeverity): HTMLTableCellElement {
 	return td;
 }
 
-function renderActions(onViewJson: () => void): HTMLElement {
-	const actions = el("div", "actions");
-	const viewBtn = el("button", undefined, "View JSON");
-	viewBtn.addEventListener("click", onViewJson);
-	actions.append(viewBtn);
-	return actions;
-}
-
-/** Renders a top-level error message in place of the report. */
-export function renderError(message: string): HTMLElement {
-	return el("p", "error", message);
-}
-
 /** Create an element with an optional class and text content. */
 function el<K extends keyof HTMLElementTagNameMap>(
 	tag: K,
@@ -229,7 +217,3 @@ function el<K extends keyof HTMLElementTagNameMap>(
 function badge(text: string): HTMLElement {
 	return el("span", "badge", text);
 }
-
-function emptyMessage(text: string): HTMLElement {
-	return el("p", "empty", text);
-}

packages/netcheck/src/regions.ts

@@ -54,7 +54,8 @@ function toRegionRow(
 		name: regionName(region, id),
 		severity: region.severity,
 		latencyMs: regionLatencyMs(latencyNanos, relayNodes),
-		preferred: id === preferredId,
+		// 0 is the "undetermined" sentinel, not a real region id.
+		preferred: Boolean(preferredId) && id === preferredId,
 		embeddedRelay: region.region?.EmbeddedRelay ?? false,
 		stun: anyTriState(stunNodes, (n) => n.stun.CanSTUN),
 		relay: anyTriState(relayNodes, (n) => n.can_exchange_messages),

packages/shared/src/netcheck/utils.ts

@@ -6,11 +6,18 @@ const SEVERITY_RANK = {
 	error: 2,
 } as const satisfies Record<NetcheckSeverity, number>;
 
-/** Worst severity across the DERP and interfaces sections. */
+/**
+ * Worst severity across the report. A failed probe (`netcheck_err`) or section
+ * error counts as an error even when the section severity wasn't raised, so the
+ * banner agrees with the Issues list.
+ */
 export function overallNetcheckSeverity(
 	report: NetcheckReport,
 ): NetcheckSeverity {
 	const { derp, interfaces } = report;
+	if (derp.netcheck_err || derp.error || interfaces.error) {
+		return "error";
+	}
 	return SEVERITY_RANK[derp.severity] >= SEVERITY_RANK[interfaces.severity]
 		? derp.severity
 		: interfaces.severity;

packages/speedtest/src/index.ts

@@ -1,5 +1,11 @@
 import { SpeedtestApi, type SpeedtestResult, toError } from "@repo/shared";
-import { sendCommand, subscribeNotifications } from "@repo/webview-shared";
+import {
+	emptyMessage,
+	errorMessage,
+	sendCommand,
+	subscribeNotifications,
+	viewJsonAction,
+} from "@repo/webview-shared";
 import "@repo/webview-shared/base.css";
 
 import { renderLineChart } from "./chart";
@@ -53,14 +59,14 @@ function renderPage(
 
 	const samples = toChartSamples(data.intervals);
 	if (samples.length === 0) {
-		root.appendChild(renderEmptyMessage());
-		root.appendChild(renderActions(onViewJson));
+		root.appendChild(emptyMessage("No samples returned from the speed test."));
+		root.appendChild(viewJsonAction(onViewJson));
 		return () => undefined;
 	}
 
 	const chart = renderChart(samples);
 	root.appendChild(chart.container);
-	root.appendChild(renderActions(onViewJson));
+	root.appendChild(viewJsonAction(onViewJson));
 	return chart.cleanup;
 }
 
@@ -191,32 +197,11 @@ function renderChart(samples: ChartPoint[]): {
 	};
 }
 
-function renderActions(onViewJson: () => void): HTMLElement {
-	const actions = document.createElement("div");
-	actions.className = "actions";
-	const viewBtn = document.createElement("button");
-	viewBtn.textContent = "View JSON";
-	viewBtn.addEventListener("click", onViewJson);
-	actions.appendChild(viewBtn);
-	return actions;
-}
-
-function renderEmptyMessage(): HTMLElement {
-	const p = document.createElement("p");
-	p.className = "empty";
-	p.textContent = "No samples returned from the speed test.";
-	return p;
-}
-
 function showError(message: string): void {
 	const root = document.getElementById("root");
-	if (!root) {
-		return;
+	if (root) {
+		root.replaceChildren(errorMessage(message));
 	}
-	const p = document.createElement("p");
-	p.className = "error";
-	p.textContent = message;
-	root.replaceChildren(p);
 }
 
 main();

packages/webview-shared/src/dom.ts

@@ -0,0 +1,29 @@
+/** DOM builders shared by the vanilla webviews. */
+
+/** An action bar with a "View JSON" button wired to `onClick`. */
+export function viewJsonAction(onClick: () => void): HTMLElement {
+	const actions = document.createElement("div");
+	actions.className = "actions";
+	const button = document.createElement("button");
+	button.textContent = "View JSON";
+	button.addEventListener("click", onClick);
+	actions.append(button);
+	return actions;
+}
+
+/** A message shown in place of a section or result that has no data. */
+export function emptyMessage(text: string): HTMLElement {
+	return paragraph("empty", text);
+}
+
+/** A top-level failure message. */
+export function errorMessage(text: string): HTMLElement {
+	return paragraph("error", text);
+}
+
+function paragraph(className: string, text: string): HTMLElement {
+	const p = document.createElement("p");
+	p.className = className;
+	p.textContent = text;
+	return p;
+}

packages/webview-shared/src/index.ts

@@ -14,3 +14,6 @@ export {
 	sendCommand,
 	subscribeNotifications,
 } from "./ipc";
+
+// DOM builders shared by the vanilla webviews.
+export { viewJsonAction, emptyMessage, errorMessage } from "./dom";

src/command/diagnosticFlow.ts

@@ -15,8 +15,11 @@ export interface DiagnosticCliOptions {
 	progressTitle: string;
 	/** Invoke the CLI under the progress notification; resolves to raw JSON. */
 	exec: (ctx: ProgressContext) => Promise<string>;
-	/** Parse the output, record success telemetry, and show the results. */
-	display: (rawJson: string) => void;
+	/**
+	 * Parse the output, record success telemetry, and show the results. Parsing
+	 * happens here so parse failures map to the `parse_error` telemetry path.
+	 */
+	parseAndDisplay: (rawJson: string) => void;
 }
 
 /**
@@ -48,21 +51,49 @@ export async function runDiagnosticCli(
 		return;
 	}
 
+	// On a display failure, keep the report the user waited for recoverable by
+	// offering its raw output behind a "View Output" action.
+	const offerRawOutput = (message: string) => {
+		void vscode.window
+			.showErrorMessage(message, "View Output")
+			.then((choice) => {
+				if (choice === "View Output") {
+					void openRawOutput(result.value, name, logger);
+				}
+			});
+	};
+
 	try {
-		options.display(result.value);
+		options.parseAndDisplay(result.value);
 	} catch (err) {
 		if (err instanceof ZodError || err instanceof SyntaxError) {
 			telemetry.error("parse_error");
 			logger.error(`Failed to parse ${name} output`, err);
-			vscode.window.showErrorMessage(
+			offerRawOutput(
 				`${name} output did not match the expected format. Check \`Output > Coder\` for details.`,
 			);
 			return;
 		}
 		telemetry.error();
 		logger.error(`Failed to display ${name} results`, err);
-		vscode.window.showErrorMessage(
-			`${name} returned unexpected output: ${toError(err).message}`,
+		offerRawOutput(
+			`${name} could not display its results: ${toError(err).message}`,
 		);
 	}
 }
+
+async function openRawOutput(
+	rawJson: string,
+	name: string,
+	logger: Logger,
+): Promise<void> {
+	try {
+		const doc = await vscode.workspace.openTextDocument({
+			content: rawJson,
+			language: "json",
+		});
+		await vscode.window.showTextDocument(doc, vscode.ViewColumn.Beside);
+	} catch (err) {
+		logger.error(`Failed to open ${name} output`, err);
+	}
+}