From c4fea5a4ae13b807ccf8fb1e9629868b2e3d378b Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Tue, 18 Aug 2026 10:28:16 +0100 Subject: [PATCH] Make the GUI say what it actually did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three result-correctness defects from the independent review, all of the same shape: the interface reported something the backend did not do. 1. The command preview claimed five ports and scanned three. `buildCommand` substituted the GUI's DEFAULT_PORTS placeholder (22,80,443,8080,8443) whenever the Ports field was empty, while execution sent no port list at all and the core fell back to its own default of 22,80,443. That wrong string is what Ctrl+Shift+C copied, what the History menu recorded, and what was stored with the saved result bundle. `-p` is now omitted when the field is empty -- which is both accurate and what `inspect` and `sweep` in the same function already did. 2. Truncated captures were presented as complete. The core sets `packets_truncated` on a capture and `truncated` on a parse. Neither was read anywhere in the GUI. Opening a 20,000-packet file with the default 1,000 limit showed 1,000 rows under a status bar reading "20000 packets", with nothing saying the view was cut off -- and Export CSV then wrote only the loaded rows. The summary now reads "1000 of 20000 packets shown" when either flag is set. 3. Three call sites disagreed on which ports an inspect checked. The row builder used `ports?.length ? ports : open_ports`; the column set and the status-bar summary used `ports ?? open_ports`. A backend returning `ports: []` with a non-empty `open_ports` rendered rows from `open_ports` while the columns came from an empty list, so every cell was blank. All three now call one `inspectPorts` helper, keeping the length check -- `ports` is optional for older backends and an empty list carries no more information than a missing one. Two smaller preview fixes in the same function: the pcap capture branch omitted `--json` while every other command includes it, and interpolated the BPF filter raw, so a filter containing a quote produced a preview that would not parse if pasted. Every new test was confirmed to fail against the previous behaviour: "expected 'netscli scan router.local -p 22,80,44…' to be 'netscli scan router.local --json'" and "expected '20000 packets' to be '1000 of 20000 packets shown'". The pcap summary cases moved to their own file rather than pushing presentation.test.ts past its size cap -- which is the split its transition exception already asked for. 105 GUI tests pass, lint 0 errors, build and file-size guard clean. --- .../src/tools/presentation.test.ts | 20 +++++++++- .../src/tools/presentation/columns.ts | 5 ++- .../src/tools/presentation/commands.ts | 17 +++++++-- .../src/tools/presentation/ports.ts | 20 +++++++++- .../src/tools/presentation/rows.ts | 4 +- .../src/tools/presentation/summaries.ts | 24 ++++++++++-- apps/netscli-gui/src/tools/summaries.test.ts | 38 +++++++++++++++++++ 7 files changed, 116 insertions(+), 12 deletions(-) create mode 100644 apps/netscli-gui/src/tools/summaries.test.ts diff --git a/apps/netscli-gui/src/tools/presentation.test.ts b/apps/netscli-gui/src/tools/presentation.test.ts index ce03c144..d1a49907 100644 --- a/apps/netscli-gui/src/tools/presentation.test.ts +++ b/apps/netscli-gui/src/tools/presentation.test.ts @@ -76,9 +76,27 @@ describe('buildCommand', () => { pcap.form.filter = 'tcp port 443'; pcap.form.max_packets = '50'; expect(buildCommand(pcap)).toBe( - 'netscli pcap --interface Ethernet --duration 10 --filter "tcp port 443" --max-packets 50', + 'netscli pcap --interface Ethernet --duration 10 --filter "tcp port 443" --max-packets 50 --json', ); }); + + it('omits -p when the ports field is empty, because that is what runs', () => { + // The preview used to substitute the GUI's five-port placeholder while + // execution sent no port list and the core used its own default of three. + // The copied command, the History entry and the saved bundle all carried + // the wrong string. + const scan = createTab('scan'); + scan.form.host = 'router.local'; + scan.form.ports = ''; + expect(buildCommand(scan)).toBe('netscli scan router.local --json'); + }); + + it('escapes quotes in a capture filter so the preview stays paste-able', () => { + const pcap = createTab('pcap'); + pcap.form.interface = 'eth0'; + pcap.form.filter = 'host "example"'; + expect(buildCommand(pcap)).toContain('--filter "host \\"example\\""'); + }); }); describe('tabIdentity', () => { diff --git a/apps/netscli-gui/src/tools/presentation/columns.ts b/apps/netscli-gui/src/tools/presentation/columns.ts index 91e69ee7..26072100 100644 --- a/apps/netscli-gui/src/tools/presentation/columns.ts +++ b/apps/netscli-gui/src/tools/presentation/columns.ts @@ -1,6 +1,7 @@ import type { ToolResult } from '../../types/app'; import type { ResultColumn, ResultRow, ToolKind } from '../types'; import { buildRows } from './rows'; +import { inspectPorts } from './ports'; export function columnsFor( kind: ToolKind, @@ -13,8 +14,8 @@ export function columnsFor( const hasPorts = hasColumnData(rows, 'ports'); const hasMac = hasColumnData(rows, 'mac'); - const inspectPorts = result?.kind === 'inspect' ? (result.data.ports ?? result.data.open_ports) : []; - if (kind === 'inspect' && result?.kind === 'inspect' && inspectPorts.length === 0) { + const inspectPortList = result?.kind === 'inspect' ? inspectPorts(result.data) : []; + if (kind === 'inspect' && result?.kind === 'inspect' && inspectPortList.length === 0) { return [ { key: 'host', label: 'Host', mono: true, width: 220 }, { key: 'ip', label: 'IP', mono: true, width: 220 }, diff --git a/apps/netscli-gui/src/tools/presentation/commands.ts b/apps/netscli-gui/src/tools/presentation/commands.ts index ce968186..d61a6df9 100644 --- a/apps/netscli-gui/src/tools/presentation/commands.ts +++ b/apps/netscli-gui/src/tools/presentation/commands.ts @@ -1,11 +1,17 @@ -import { DEFAULT_PORTS } from '../registry'; import type { WorkspaceTab } from '../types'; export function buildCommand(tab: WorkspaceTab): string { const form = tab.form; switch (tab.kind) { case 'scan': - return `netscli scan ${form.host || ''} -p ${form.ports || DEFAULT_PORTS} --json`; + // `-p` is omitted when the field is empty, exactly as `inspect` and + // `sweep` below already do. It used to substitute the GUI's + // DEFAULT_PORTS placeholder (22,80,443,8080,8443) while execution sent + // no port list at all and the core fell back to its own default of + // 22,80,443 — so the preview claimed five ports, three were scanned, + // and that wrong string was what Ctrl+Shift+C copied, what the History + // menu recorded, and what got stored with the saved result. + return `netscli scan ${form.host || ''}${form.ports ? ` -p ${form.ports}` : ''} --json`; case 'ping': return `netscli ping ${form.host || ''}${form.count ? ` --count ${form.count}` : ''} --json`; case 'trace': { @@ -45,8 +51,13 @@ export function buildCommand(tab: WorkspaceTab): string { const parts = ['netscli pcap']; if (form.interface) parts.push(`--interface ${form.interface}`); if (form.duration) parts.push(`--duration ${form.duration}`); - if (form.filter) parts.push(`--filter "${form.filter}"`); + // Escape quotes rather than interpolating raw: a BPF filter containing + // a double quote produced a preview that would not parse if pasted. + if (form.filter) parts.push(`--filter "${form.filter.replace(/"/g, '\\"')}"`); if (form.max_packets) parts.push(`--max-packets ${form.max_packets}`); + // The capture branch omitted --json while every other command here + // includes it, so this one preview did not match what the app runs. + parts.push('--json'); return parts.join(' '); } } diff --git a/apps/netscli-gui/src/tools/presentation/ports.ts b/apps/netscli-gui/src/tools/presentation/ports.ts index 77d31a1d..c9e0c296 100644 --- a/apps/netscli-gui/src/tools/presentation/ports.ts +++ b/apps/netscli-gui/src/tools/presentation/ports.ts @@ -1,4 +1,22 @@ -import type { PortResult } from '../../types/netscli'; +import type { InspectResult, PortResult } from '../../types/netscli'; + +/** + * Which ports an inspect actually checked, everywhere. + * + * Three call sites had drifted into two different answers: the row builder + * used `ports?.length ? ports : open_ports`, while the column set and the + * status-bar summary used `ports ?? open_ports`. A backend returning + * `ports: []` alongside a non-empty `open_ports` therefore rendered rows + * from `open_ports` while the columns were built from an empty list, so + * every cell came out blank. + * + * The length check is the right one to keep: `ports` is optional for older + * backends, and an empty list carries no more information than a missing + * one. + */ +export function inspectPorts(result: InspectResult): PortResult[] { + return result.ports?.length ? result.ports : result.open_ports; +} export function statusOf(port: PortResult): string { return port.status ?? (port.open ? 'open' : 'closed'); diff --git a/apps/netscli-gui/src/tools/presentation/rows.ts b/apps/netscli-gui/src/tools/presentation/rows.ts index d57a6d22..2531e173 100644 --- a/apps/netscli-gui/src/tools/presentation/rows.ts +++ b/apps/netscli-gui/src/tools/presentation/rows.ts @@ -1,7 +1,7 @@ import type { ToolResult } from '../../types/app'; import type { PortResult } from '../../types/netscli'; import type { ResultRow, ToolKind } from '../types'; -import { latencyOf, statusOf } from './ports'; +import { inspectPorts, latencyOf, statusOf } from './ports'; import { parseTraceLine, type TraceHopRow } from './traceLine'; function portRow(port: PortResult, index: number, kind: ToolKind): ResultRow { @@ -128,7 +128,7 @@ export function buildRows(result: ToolResult | null): ResultRow[] { ]; } case 'inspect': { - const ports = result.data.ports?.length ? result.data.ports : result.data.open_ports; + const ports = inspectPorts(result.data); const rows = ports.map((port, index) => portRow(port, index, 'inspect')); if (rows.length > 0) return rows; const ping = result.data.ping; diff --git a/apps/netscli-gui/src/tools/presentation/summaries.ts b/apps/netscli-gui/src/tools/presentation/summaries.ts index 34ffc273..10439bca 100644 --- a/apps/netscli-gui/src/tools/presentation/summaries.ts +++ b/apps/netscli-gui/src/tools/presentation/summaries.ts @@ -1,4 +1,5 @@ import type { ToolResult } from '../../types/app'; +import { inspectPorts } from './ports'; export function resultSummary(result: ToolResult | null): string { if (!result) return '0 results'; @@ -18,7 +19,7 @@ export function resultSummary(result: ToolResult | null): string { case 'reverse': return result.data.hostname ? 'reverse name found' : 'no reverse name'; case 'inspect': { - const ports = result.data.ports ?? result.data.open_ports; + const ports = inspectPorts(result.data); if (ports.length > 0) { const open = ports.filter((port) => port.open).length; return `${ports.length} ${ports.length === 1 ? 'port' : 'ports'} checked - ${open} open`; @@ -37,8 +38,25 @@ export function resultSummary(result: ToolResult | null): string { return `${result.data.length} interfaces`; case 'arp': return `${result.data.length} ARP entries`; - case 'pcap': - return `${'packets_captured' in result.data ? result.data.packets_captured : result.data.total_packets} packets`; + case 'pcap': { + // Say when the view is cut off. The core sets `packets_truncated` on a + // capture and `truncated` on a parse, and neither was read anywhere in + // the GUI: opening a 20,000-packet file with the default 1,000 limit + // showed 1,000 rows under a status bar reading "20000 packets", with + // nothing indicating the rest was missing — and Export CSV then wrote + // only the loaded rows. + // `in` has to narrow inline; hoisting the discriminant into a boolean + // loses the narrowing and the union members have no fields in common. + const data = result.data; + if ('packets_captured' in data) { + return data.packets_truncated + ? `${formatNumber(data.packets.length)} of ${formatNumber(data.packets_captured)} packets shown` + : `${formatNumber(data.packets_captured)} packets`; + } + return data.truncated + ? `${formatNumber(data.packets.length)} of ${formatNumber(data.total_packets)} packets shown` + : `${formatNumber(data.total_packets)} packets`; + } } } diff --git a/apps/netscli-gui/src/tools/summaries.test.ts b/apps/netscli-gui/src/tools/summaries.test.ts new file mode 100644 index 00000000..d1e9f8c5 --- /dev/null +++ b/apps/netscli-gui/src/tools/summaries.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest'; + +import { resultSummary } from './presentation'; + +describe('resultSummary — packet capture truncation', () => { + /** + * The core sets `packets_truncated` on a capture and `truncated` on a + * parse, and neither was read anywhere in the GUI. Opening a + * 20,000-packet file with the default 1,000 limit showed 1,000 rows under + * a status bar reading "20000 packets" — no indication the view was cut + * off, while Export CSV wrote only the loaded rows. + */ + const packets = (n: number) => Array.from({ length: n }, (_, i) => ({ index: i })) as never[]; + + it('says how many of how many when a parsed capture is truncated', () => { + const summary = resultSummary({ + kind: 'pcap', + data: { file_path: 'x.pcap', link_type: 1, total_packets: 20000, packets: packets(1000), truncated: true }, + } as never); + expect(summary).toBe('1000 of 20000 packets shown'); + }); + + it('says how many of how many when a live capture is truncated', () => { + const summary = resultSummary({ + kind: 'pcap', + data: { file_path: 'x.pcap', duration: { secs: 5, nanos: 0 }, packets_captured: 8000, packets: packets(1000), packets_truncated: true }, + } as never); + expect(summary).toBe('1000 of 8000 packets shown'); + }); + + it('reports a plain total when nothing was dropped', () => { + const summary = resultSummary({ + kind: 'pcap', + data: { file_path: 'x.pcap', link_type: 1, total_packets: 42, packets: packets(42), truncated: false }, + } as never); + expect(summary).toBe('42 packets'); + }); +});