feat: add network check command with results webview#1002
Conversation
EhabY
force-pushed
the
feat/netcheck-webview
branch
2 times, most recently
from
75527f3 to
c7240ae
Compare
EhabY
changed the title
EhabY
force-pushed
the
feat/netcheck-webview
branch
from
108c43e to
355e16c
Compare
|
/coder-agents-review model:claude-opus-4-8 thinking:high |
|
Chat: Review posted | View chat Review history
deep-review v0.7.1 | Round 3 | Last posted: Round 3, 28 findings (4 P2, 9 P3, 8 Nit, 7 Note), COMMENT. Review Finding inventoryFinding inventory — PR #1002 (vscode-coder)Law analysis
Findings
Contested and acknowledgedCRF-11 (P3, pnpm-lock.yaml) - lockfile peer re-keying
CRF-14 (Nit, page.ts) - DOM duplication
Round logRound 3Churn guard: PROCEED. All 6 R2 findings addressed in e9a1b2b ("fix: surface region faults and dedupe JSON-open in netcheck"), 0 silent. Headline fix: Pariston's structural remedy implemented, error/netcheck_err and region severities folded into section severity at the Zod parse boundary (types.ts), so all consumers read one honest severity. Law not re-run (effective LOC +3129, grew only +72). Reviewed against 9a8dfaf..e9a1b2b. Round 2Churn guard: PROCEED. 15 addressed, 3 acknowledged (CRF-14, CRF-19, CRF-20), 1 contested (CRF-11), 0 silent. Status cells updated to author claims; panel verifies fixes against the new code. Netero + Law re-run (effective LOC grew 2477 to 3057, +580). Reviewed against 9a8dfaf..e20388b. Round 1Netero first-pass: 1 P3, 1 Nit, 2 Notes (all below panel gate). Law: advisory split. Panel of 17 (5 always-on + 10 trigger-matched + 2 wildcards: meruem, zoro). Convergence: CRF-5 (banner severity) raised P2 by four independent reviewers and traced to coderd/healthcheck/derphealth/derp.go (NetcheckErr set without bumping Severity) plus the in-repo health.test.ts fixture. Kurapika and Luffy: no findings. 1 P2, 7 P3, 6 Nits, 11 Notes (5 posted inline, 6 folded into body). Reviewed against 451491d..355e16c. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strong feature work. The duplication that a second one-shot CLI panel would have created was pulled into showResultPanel and runDiagnosticCli and the existing speedtest command migrated onto them, so the PR adds a feature while leaving less scaffolding than a copy-paste would. The shared-mechanism tests moved verbatim into resultPanel.test.ts rather than being dropped, the domain suites assert real derived values, every report value reaches the DOM through textContent/createTextNode (no injection surface), the CLI runs via execFile with a fixed arg array, and the deployment-scoped wiring (view/title overflow plus palette, single command id) is handled rather than copied from the workspace-scoped pattern. Kurapika (security) and Luffy (product/over-engineering) both came back with no findings. Hisoka put it well after probing the parser: "The trap was set and the author already disarmed it."
Counts: 1 P2, 7 P3, 6 Nits, and several Notes.
The one must-fix is CRF-5: the health banner and the severity telemetry are derived only from the two section severities, but coder netcheck records a failed connectivity probe in derp.netcheck_err without raising derp.severity. Four reviewers independently traced this to coderd/healthcheck/derphealth/derp.go and to the repo's own health.test.ts fixture. The result: a green "Network is healthy" headline can sit directly above an Error in the Issues list, in exactly the degraded scenario the tool exists to diagnose, and the run is recorded as severity: ok. No P0/P1, so this is a COMMENT, but CRF-5 should be addressed before merge.
Law flagged an advisory split (land the shared-scaffolding extraction plus speedtest migration as a behavior-preserving precursor, then the netcheck feature on top). It is advisory only; the bundling is defensible because the abstraction is validated by both consumers landing together.
A few low-value notes are folded here rather than inline: runNetcheck inlines a third copy of the baseURL / "You are not logged in" pattern (requireExtensionBaseUrl/requireRemoteBaseUrl already exist, though they throw, so reuse here would read worse); the telemetry command value netcheck is not snake_cased while its sibling is speed_test; the literal U+2014 placeholder glyph is fine here since vscode-coder has no em-dash lint; and two more render branches lack tests beyond CRF-8 (the two-unmeasured-region name-order sort tiebreak in regions.ts, and the section-level error field that collectIssues surfaces in health.ts).
🤖 This review was automatically generated with Coder Agents.
EhabY
force-pushed
the
feat/netcheck-webview
branch
2 times, most recently
from
5d082bd to
61695af
Compare
|
/coder-agents-review model:claude-opus-4-8 thinking:high |
Render report sections as bordered cards, replace opacity-based dimming with VS Code theme tokens (descriptionForeground, widget/panel borders, keybindingTable header background, focusBorder), tint the status banner by severity, and make the page responsive: drop the body min-width, let tables scroll inside their cards, and tighten spacing on narrow panels.
…ommand flow - Replace the monolithic render.ts/index.ts in packages/netcheck with focused modules: format, health, connectivity, regions, dom, page - Extract the shared CLI-diagnostic command flow (progress, telemetry, error handling) from runSpeedTest/runNetcheck into src/command/diagnosticFlow.ts - Add an emptyIfNull schema helper and remove an Object.fromEntries cast in the netcheck report schema - Simplify NetcheckPanelFactory.show to take NetcheckData directly - Split render.test.ts into per-module test files with a shared fixture Rendering is unchanged: harness screenshots are byte-identical before and after across themes, states, and widths.
Make the netcheck webview a leaner renderer that is easy to port to React, and remove duplication across the webview, shared API, and tests. - Fold dom.ts helpers into page.ts and move table-flush styling to a CSS :has() rule so page.ts holds rendering only, not layout logic. - Replace connectivity's twin-object triItem with a labeled-tuple boolItem; document the tone -> CSS-class mapping. - Drop formatTriState's always-identical labels param. - Unify health severity lookups as readonly maps; sectionSummary returns the status and the caller composes the label. - Trim RegionRow.id and node-level severity/uses_websocket that were modeled but never rendered, in both the shared type and the Zod schema. - Cover the shared showResultPanel mechanism once in resultPanel.test.ts and slim the netcheck/speedtest factory tests to their feature-specific wiring.
- Split shared netcheck into types.ts / api.ts (Api only) / utils.ts, trim the exported surface, and document provenance. - Add a compile-time drift guard: the parser fails to build if the coder SDK renames or removes a field it reads. - Extract shared base styles into @repo/webview-shared/base.css, consumed by both vanilla webviews; each index.css keeps only page-specific rules. - Simplify connectivity (keyed cases, no nested ternary) and regions (map -> toSorted pipeline); render missing IPv4 as a warning, not an error. - Move the large parser payloads into golden fixtures/ JSON files.
- 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
EhabY
force-pushed
the
feat/netcheck-webview
branch
from
61695af to
e20388b
Compare
There was a problem hiding this comment.
Strong round. Of the 19 posted R1 findings, 15 are addressed and verified against the code: CRF-5's section-level fix lands with a real severity.test.ts, CRF-6 (port-mapping tri-state) and CRF-9 (PreferredDERP===0 sentinel) are correctly narrowed and tested, the diagnostic-flow coverage (CRF-1) and handler test (CRF-4) are genuine, parse failures now keep the raw report reachable via View Output (CRF-7), telemetry is recorded after the panel shows (CRF-17), the parse-vs-display messages are split (CRF-18), and the renames/constants (CRF-10/12/13/15/16) are fixed at the root. CRF-11 (lockfile) was contested and is now closed: the current diff is 21 additive lines with 0 resolution: changes, exactly as the author argued. CRF-14 over-delivered: despite the reply saying "left as is," the DOM builders were actually deduped into a new shared dom.ts. The +580 LOC is almost all test code answering R1, which is proportional.
The headline this round is that the CRF-5 fix, while correct, is incomplete. The upstream derp.go decoupling CRF-5 exposed (an error set without raising severity) exists at three layers (section, region, node), and the fix only taught the section layer. Hisoka put it well: "You came dressed as a routine webview and hid a three-layer severity rollup mismatch underneath. The banner, the Issues list, and the region row each trust a different slice of the same Go report, and the slices disagree exactly where a relay actually breaks." Three surviving siblings: CRF-26 (P2, a panicked region still banners green "healthy"), CRF-27 (P2, region/node errors never reach the Issues list and region warnings are stripped at parse), and CRF-25 (P3, sectionSummary and the region status cell still read raw severity, so the banner title contradicts the rows). Pariston's structural fix resolves CRF-25/26 in one place: fold the error into severity at the Zod parse boundary (severity = error ? "error" : severity, plus netcheck_err/region error), so every consumer reads a corrected value and the class cannot recur per-site.
None are P0/P1, so this stays a COMMENT, but CRF-26/27 reproduce the original CRF-5 harm class and are worth closing before merge. Smaller new items: CRF-28 (a vacuous warning.count assertion), CRF-29 (the new View Output helper duplicates the panel's and fails silently), and CRF-30 (an empty-state message points at an Issues section that isn't rendered).
Still on record and untouched: CRF-19/CRF-20 accepted by the author with rationale; CRF-22 (telemetry netcheck not snake_cased), CRF-23/CRF-24 (two render branches still untested after the R2 additions), and CRF-8's index.ts renderError catch (defensive against an in-practice-impossible throw, low value).
🤖 This review was automatically generated with Coder Agents.
Address second-round review feedback (CRF-25..30): - Fold error/netcheck_err into severity once at the Zod parse boundary, rolling region errors up into the DERP section, so the banner, section summary, region cell, and telemetry all read one honest severity. overallNetcheckSeverity is now just the worst of the two sections. - collectIssues walks regions, surfacing per-region errors and warnings (name-prefixed) in the Issues list instead of only a hover tooltip; region warnings now survive parsing. - Extract openJsonBeside shared by resultPanel and the diagnostic flow, which also fixes the silent-failure divergence in the parse-failure recovery. - Drop the dead "See Issues above" reference from the connectivity empty state. - Give the diagnostics fixture a DERP warning so warning.count proves both terms. CRF-11 (lockfile peer churn) verified clean: the diff is the new package only, regenerated under the pinned pnpm@11.6.0.
|
/coder-agents-review model:claude-opus-4-8 thinking:high |
There was a problem hiding this comment.
This round is the strongest signal in the PR's history that the author fixes classes, not instances. The R2 CRF-5 family is resolved at the source: error/netcheck_err and region severities are folded into section severity once at the Zod parse boundary, so overallNetcheckSeverity, sectionSummary, the region status cell, and the telemetry all read one honest field. Knov, Pariston, Meruem, Kite, and Mafu-san each traced the fold and confirmed it does not over- or under-escalate, the CRF-29 silent-failure path was deleted via a shared openJsonBeside, the CRF-28 fixture now makes both warning.count summands fire, and CRF-30's dead reference is gone. Coverage was relocated to types.test.ts, not lost.
The one structural theme that survives is the same layered decoupling, one level deeper. The fix folds the section and region layers, but the node layer is still dropped at parse, so a single-node DERP region whose node cannot exchange messages (the common embedded-relay case) banners red with an empty Issues list and no tooltip. This is the node half of CRF-27, which named "region/node errors"; I confirmed against coderd/healthcheck/derphealth/derp.go that a single-node region inherits the node's error severity (line 230) while leaving region.error nil and region.warnings empty, and Nami reproduced it empirically (overallNetcheckSeverity "error", collectIssues []). Hisoka: "It just stopped at the region wall and the node fault walked right through." Re-raised on the CRF-27 thread.
One regression the region fix introduced: CRF-32 (P2). The CLI already aggregates every region and node warning into the DERP section warnings array (derp.go:112 and :209), which collectIssues already emits, so the new region-warning walk now lists each region/node warning twice (once bare, once region-prefixed). The region-error walk is correct and necessary and should stay; only the warning half duplicates.
That same aggregation led me to drop CRF-31 (the R3 first-pass note that warning.count excludes region warnings). Four reviewers reasoned from the webview code that region warnings are separate from the section count, but Chopper checked the source: because the CLI folds region/node warnings into derp.warnings, warning.count already counts them, and the undercount only arises from hand-built reports the CLI never emits. The real warning bug is the opposite, the double-listing above.
Smaller items: CRF-33 (P3, the derp.error fold operand has no isolating test, so dropping it would silently reintroduce the false-green), CRF-34 (Nit, the page-header builder is the one DOM duplicate that stayed behind after the dom.ts dedup), and CRF-35 (Note, the error-into-severity fold lives at three sites with no shared helper and section.severity is now a destructive rollup the shared type does not document). None are P0/P1, so this stays a COMMENT.
🤖 This review was automatically generated with Coder Agents.
CRF-27 (node half): a single-node region whose node is unhealthy (e.g. the embedded relay that can't exchange messages) gets severity error from the CLI with no error string or warning, so the banner was red over an empty Issues list. The region severity already reflects the node, so collectIssues now synthesizes a region-prefixed message when a region is non-ok but carries no message of its own.
Address review round 4 (CRF-32..35): - CRF-32: the CLI folds region/node warnings into the DERP section `warnings`, so the region walk listed them twice. Drop the region-warning walk (keep region errors, which are not aggregated) and remove the now-redundant region `warnings` field from the schema and shared type. - CRF-33: add a test for the `derp.error` severity fold (the existing case set `netcheck_err`, short-circuiting the `|| d.error` operand). - CRF-34: extract `pageHeader` into @repo/webview-shared/dom.ts, used by both vanilla webviews (the last header duplicate after the dom.ts dedup). - CRF-35: document that section `severity` is a parse-time rollup. Kept the error fold inlined per earlier maintainer preference (trivial ternary).
1 participant
Closes #348.
Adds a Coder: Network Check command that runs
coder netcheckand renders the report in a webview panel. Available from the command palette and the My Workspaces view overflow menu when authenticated.How it works
nullwarning lists normalized), and the parsed report is pushed to acoder.netcheckPanelwebview.Code reuse
src/webviews/resultPanel.ts#showResultPanel. Both factories use it while keepingbuildCommandHandlers/buildRequestHandlerson their concrete APIs so the compile-time exhaustiveness guarantee is preserved.src/command/diagnosticFlow.ts#runDiagnosticCli; each command supplies only its CLI invocation and result display.@repo/webview-shared/base.css, consumed by both netcheck and speedtest; eachindex.csskeeps only page-specific rules.Types & drift
Shared report types live in
packages/shared/src/netcheck/—types.ts(a normalized subset of the CLI report, only the fields the webview renders),api.ts(theNetcheckApiIPC contract), andutils.ts(overallNetcheckSeverity, used by both telemetry and the webview banner). A compile-time guard in the parser fails the build if the coder SDK renames or removes a field we read (the DERP section mirrors codersdkDERPHealthReport); leaf type changes are caught at runtime by Zod.Telemetry
command.diagnostic.completedgainscommand: netcheckwith aseverityproperty (worst of DERP/interfaces) and boundedregion.count/warning.countmeasurements; documented insrc/instrumentation/EVENTS.md. No raw report content is recorded.Testing
test/unit/webviews/resultPanel.test.ts— the shared panel mechanism (ready handshake, visibility/theme re-send, viewJson, disposal), covered once; the netcheck and speedtest factory tests now assert only their own wiring.test/unit/webviews/netcheck/types.test.ts— Zod parsing against a goldenfixtures/netcheck-report.jsontrimmed from a real run (null warnings, null region entries, missing probe section, error cases).test/unit/core/cliExec.test.ts— netcheck arg building, stderr surfacing, abort.test/unit/instrumentation/diagnostics.test.ts— netcheck telemetry properties/measurements.test/webview/netcheck/{format,health,connectivity,regions}.test.ts— formatting, issue collection, connectivity tones, and region row derivation (latency fallback, preferred-first sort, STUN-only handling).Size
Production: +1,662 / −216 (~350 of which is webview CSS). Tests: +1,456 / −82 (tests now roughly match production by line count). The rest is config churn. Net of the speedtest panel/test dedup, the change is a new webview package plus the shared scaffolding.
Follow-up
The issue also suggests running diagnostics automatically on connection trouble, launch, or periodically. This PR is manual-only; automatic triggering is left as a follow-up.
Implementation plan
packages/shared/src/netcheck/— report types (types.ts), theNetcheckApiIPC contract (api.ts), andoverallNetcheckSeverity(utils.ts) used by both telemetry and the webview banner.packages/netcheck/— vanilla TS webview package (mirrorspackages/speedtest/); pure domain modules (format,health,connectivity,regions) for testability, withpage.tsfor DOM construction.src/webviews/resultPanel.ts— shared one-shot CLI result panel scaffolding.src/command/diagnosticFlow.ts— shared cancellable-progress/telemetry/error flow for CLI diagnostic commands (speedtest + netcheck).src/webviews/netcheck/— Zod schema, SDK drift guard, and panel factory.src/core/cliExec.ts#netcheck—coder netcheckwith global flags and abort signal.src/commands.ts#netcheck— client/host resolution, thenrunDiagnosticCliwith speedtest-equivalent error handling.Current UI