Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions apps/netscli-gui/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { useReleaseNotifications } from './hooks/useReleaseNotifications';
import { useTauriRuntimeState } from './hooks/useTauriRuntimeState';
import { appWindowAction, type AppWindowAction } from './services/appWindow';
import type { ResultCellContext } from './tools/types';
import { packetCaptureUnavailableMessage } from './workspace/packetCapture';
import { guardForOperation, type OperationGuard } from './workspace/operationGuards';
import { useWorkspace } from './workspace/useWorkspace';

Expand Down Expand Up @@ -155,10 +156,15 @@ function App() {
});
}

const pcapUnavailableReason =
activeTab?.kind === 'pcap' && !pcapCapability.available
? packetCaptureUnavailableMessage(pcapCapability.message)
: null;
// Two different questions. The menu asks "can packet capture run at all",
// which is true or false regardless of which tab is open; Run asks "can
// THIS tab run", which is only about pcap when a pcap tab is active.
// Conflating them greyed the menu entry only once you had already opened
// the thing the entry is for.
const packetCaptureUnavailable = !pcapCapability.available
? packetCaptureUnavailableMessage(pcapCapability.message)
: null;
const pcapUnavailableReason = activeTab?.kind === 'pcap' ? packetCaptureUnavailable : null;

return (
<div className={`container ${darkMode ? 'theme-dark' : 'theme-light'}`} data-testid="app-shell">
Expand All @@ -170,6 +176,7 @@ function App() {
setOpenMenu={setOpenMenu}
tabCount={workspace.tabs.length}
toolCapabilities={toolCapabilities}
toolDisabledReasons={packetCaptureUnavailable ? { pcap: packetCaptureUnavailable } : undefined}
onAddTab={workspace.addTab}
onAbout={() => setAboutOpen(true)}
onOpenSettings={() => setSettingsOpen(true)}
Expand Down Expand Up @@ -308,11 +315,5 @@ function App() {
);
}

function packetCaptureUnavailableMessage(message?: string | null): string {
if (message?.includes("built without feature 'pcap'")) {
return 'Packet Capture is not included in this build.';
}
return 'Packet Capture needs Npcap/libpcap before it can run.';
}

export default App;
15 changes: 14 additions & 1 deletion apps/netscli-gui/src/components/results/ResultTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import { StatusPill } from './StatusPill';
interface ResultTableProps {
activeTab: WorkspaceTab;
columns: ResultColumn[];
/** Needed to tell "the filter hid everything" from "the run found
* nothing". Both produce zero rows and they are not the same news. */
filterText: string;
pcapCapability?: PcapCapability;
rows: ResultRow[];
onContentContextMenu: (event: MouseEvent<HTMLElement>, cell?: ResultCellContext) => void;
Expand All @@ -29,6 +32,7 @@ interface ResultTableProps {
export function ResultTable({
activeTab,
columns,
filterText,
pcapCapability,
rows,
onContentContextMenu,
Expand Down Expand Up @@ -214,7 +218,16 @@ export function ResultTable({
})}
</tbody>
</table>
{rows.length === 0 && <div className="empty-filter">No rows match the current filter.</div>}
{rows.length === 0 && (
<div className="empty-filter">
{/* This said "No rows match the current filter" whatever the reason,
so a scan that legitimately found nothing sent people hunting
for a filter they had never set. */}
{filterText
? 'No rows match the current filter.'
: 'This run completed and found nothing.'}
</div>
)}
</div>
);
}
30 changes: 9 additions & 21 deletions apps/netscli-gui/src/components/shell/MenuBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,15 @@ import {
SquareX,
Trash2,
X,
type LucideIcon,
} from 'lucide-react';
import { useEffect, useRef } from 'react';

import { availableToolKinds, LOOKUP_TOOL_KINDS, SCAN_TOOL_KINDS, TOOL_CONFIG } from '../../tools/registry';
import { availableToolKinds, LOOKUP_TOOL_KINDS, SCAN_TOOL_KINDS } from '../../tools/registry';
import type { HistoryEntry, ToolCapabilityMap, ToolKind, WorkspaceTab } from '../../tools/types';
import { useRovingFocus } from '../primitives/focus';
import { useAnchoredPopoverPosition, useOverlayDismiss } from '../primitives/overlay';
import { NetsCliMenuMark } from './NetsCliMenuMark';

interface MenuItem {
label: string;
Icon: LucideIcon;
action?: () => void;
disabled?: boolean;
selected?: boolean;
variant?: 'danger';
}

interface MenuSection {
label?: string;
items: MenuItem[];
}
import { toolItemFactory, type MenuSection } from './menuItems';

const MENU_POPOVER_LABELS = new Set(['File', 'Edit', 'Scan', 'Tools', 'History', 'Help']);

Expand All @@ -50,6 +36,8 @@ interface MenuBarProps {
setOpenMenu: (menu: string | null) => void;
tabCount: number;
toolCapabilities: ToolCapabilityMap;
/** Why a tool cannot run, keyed by kind. Present => shown greyed. */
toolDisabledReasons?: Partial<Record<ToolKind, string>>;
onAddTab: (kind: ToolKind) => void;
onAbout: () => void;
onOpenSettings: () => void;
Expand Down Expand Up @@ -80,6 +68,7 @@ export function MenuBar({
setOpenMenu,
tabCount,
toolCapabilities,
toolDisabledReasons,
onAddTab,
onAbout,
onOpenSettings,
Expand Down Expand Up @@ -148,11 +137,7 @@ export function MenuBar({
window.setTimeout(() => lastTriggerRef.current?.focus({ preventScroll: true }), 50);
}, [menuPopoverOpen]);

const makeToolItem = (kind: ToolKind): MenuItem => ({
label: TOOL_CONFIG[kind].label,
Icon: TOOL_CONFIG[kind].Icon,
action: () => onAddTab(kind),
});
const makeToolItem = toolItemFactory(onAddTab, toolDisabledReasons);

const groups: Array<{ label: string; sections: MenuSection[] }> = [
{
Expand Down Expand Up @@ -361,7 +346,10 @@ export function MenuBar({
'menu-popover-item',
item.selected ? 'selected' : '',
item.variant === 'danger' ? 'danger' : '',
item.muted ? 'muted' : '',
].filter(Boolean).join(' ')}
data-tooltip={item.tooltip}
data-tooltip-placement="right"
disabled={item.disabled}
// Index-qualified: repeated history commands share a
// label, and React drops the duplicate key (M-1).
Expand Down
1 change: 1 addition & 0 deletions apps/netscli-gui/src/components/shell/WorkspaceView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function WorkspaceView({
<ResultTable
activeTab={activeTab}
columns={workspace.columns}
filterText={workspace.filterText}
pcapCapability={pcapCapability}
rows={workspace.rows}
onContentContextMenu={onContentContextMenu}
Expand Down
42 changes: 42 additions & 0 deletions apps/netscli-gui/src/components/shell/menuItems.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { describe, expect, it, vi } from 'vitest';

import { toolItemFactory } from './menuItems';

describe('tool menu items', () => {
it('is a plain entry when the tool can run', () => {
const item = toolItemFactory(vi.fn())('pcap');

expect(item.muted).toBe(false);
expect(item.tooltip).toBeUndefined();
});

it('is greyed and says why when the tool cannot run', () => {
const item = toolItemFactory(vi.fn(), {
pcap: 'Packet Capture needs Npcap/libpcap before it can run.',
})('pcap');

expect(item.muted).toBe(true);
expect(item.tooltip).toContain('Npcap');
});

it('stays clickable while greyed', () => {
// Not an oversight. The pane behind this entry carries the setup
// instructions and the link to them, so a genuinely inert entry would
// remove the only place that explains how to fix what it is greyed for
// -- which is the state the app was in when people reported finding no
// trace of packet capture and no explanation.
const onAddTab = vi.fn();
const item = toolItemFactory(onAddTab, { pcap: 'Needs Npcap.' })('pcap');

expect(item.disabled).toBeUndefined();
item.action?.();
expect(onAddTab).toHaveBeenCalledWith('pcap');
});

it('leaves other tools alone when one is unavailable', () => {
const make = toolItemFactory(vi.fn(), { pcap: 'Needs Npcap.' });

expect(make('scan').muted).toBe(false);
expect(make('mdns').tooltip).toBeUndefined();
});
});
48 changes: 48 additions & 0 deletions apps/netscli-gui/src/components/shell/menuItems.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import type { LucideIcon } from 'lucide-react';

import { TOOL_CONFIG } from '../../tools/registry';
import type { ToolKind } from '../../tools/types';

export interface MenuItem {
label: string;
Icon: LucideIcon;
action?: () => void;
disabled?: boolean;
selected?: boolean;
variant?: 'danger';
/** Rendered as unavailable, but still clickable. See `toolItemFactory`. */
muted?: boolean;
tooltip?: string;
}

export interface MenuSection {
label?: string;
items: MenuItem[];
}

/**
* Build a menu entry for a tool, greyed with the reason when it cannot run.
*
* Greyed rather than removed: removing it is what the app used to do, and
* people went looking for packet capture, found no trace of it, and no
* explanation either.
*
* Greyed but still clickable, deliberately. The pane behind it carries the
* setup instructions and the link to them, so a genuinely inert entry would
* take away the one place that says how to fix the thing it is greyed for.
*/
export function toolItemFactory(
onAddTab: (kind: ToolKind) => void,
reasons?: Partial<Record<ToolKind, string>>
): (kind: ToolKind) => MenuItem {
return (kind) => {
const reason = reasons?.[kind];
return {
label: TOOL_CONFIG[kind].label,
Icon: TOOL_CONFIG[kind].Icon,
action: () => onAddTab(kind),
muted: Boolean(reason),
tooltip: reason ? `${reason} Open for setup instructions.` : undefined,
};
};
}
24 changes: 23 additions & 1 deletion apps/netscli-gui/src/styles/shell/menu.css
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,11 @@
}

.menu-popover-item.danger:hover:not(:disabled) {
background: color-mix(in srgb, var(--red), transparent 82%);
/* The same neutral highlight every other item uses. A red wash under the
cursor reads as "this went wrong" rather than "this is destructive", and
it fires on nothing more than passing over Exit. The red icon below
still marks the item as the destructive one. */
background: var(--bg-selected);
color: var(--text-primary);
}

Expand Down Expand Up @@ -199,3 +203,21 @@
.menu-popover-item.selected {
color: var(--mint);
}

/* A tool that cannot run right now: shown, explained, and still openable.
Removing it is what the app used to do, and people went looking for
packet capture, found no trace of it and no explanation. Greyed says
"not available"; the pane behind it says why and how to fix it. */
.menu-popover-item.muted {
color: var(--text-muted);
}

.menu-popover-item.muted svg {
color: var(--text-muted);
opacity: 0.7;
}

.menu-popover-item.muted:hover:not(:disabled) {
background: var(--bg-selected);
color: var(--text-secondary);
}
1 change: 1 addition & 0 deletions apps/netscli-gui/src/tools/presentation/columns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export function columnsFor(
? { key: 'vendor', label: 'Vendor', grow: true }
: { key: 'vendor', label: 'Vendor', width: 180 },
{ key: 'rtt', label: 'RTT', mono: true, width: 100 },
{ key: 'found', label: 'Found by', width: 130 },
];
case 'dns':
return [
Expand Down
62 changes: 62 additions & 0 deletions apps/netscli-gui/src/tools/presentation/discoverTruth.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// A discovered host must not be described as having answered when it did not.
//
// The GUI called every discover row a "Responded host". The core has always
// distinguished the two cases -- a probe reply means the host is there now, a
// neighbour-table entry means the OS spoke to it recently and it may since
// have left -- and the raw tab showed `"found_by": "neighbor"` one click
// away, while the primary view stated the opposite. On an ordinary LAN that
// was 5 rows in 24 confidently mislabelled.
//
// PRODUCT.md: "No wrong answers. The scanner never silently reports
// something false." design-direction.md: never "present a result as certain
// when the underlying probe failed."

import { describe, expect, it } from 'vitest';

import { columnsFor } from './columns';
import { detailLinesForRow } from './rowDetails';
import { buildRows } from './rows';
import type { ToolResult } from '../../types/app';

const discoverResult = (): ToolResult =>
({
kind: 'discover',
data: [
{ ip: '192.168.1.10', hostname: 'desk', mac: 'AA:BB:CC:DD:EE:FF', vendor: 'Acme', rtt_ms: 3, found_by: 'probe' },
{ ip: '192.168.1.42', hostname: null, mac: 'C0:35:32:1A:19:EF', vendor: 'Liteon', rtt_ms: null, found_by: 'neighbor' },
],
}) as ToolResult;

describe('a discovered host describes how it was found', () => {
it('does not claim a neighbour-table entry responded', () => {
const rows = buildRows(discoverResult());
const summary = detailLinesForRow(rows[1]).find((l) => l.label === 'Summary');

expect(summary?.value).not.toMatch(/responded to a probe/i);
expect(summary?.value).toMatch(/neighbour table/i);
});

it('still says so when the host did answer', () => {
const rows = buildRows(discoverResult());
const summary = detailLinesForRow(rows[0]).find((l) => l.label === 'Summary');

expect(summary?.value).toMatch(/responded to a probe/i);
});

it('carries found_by onto the row instead of dropping it', () => {
const rows = buildRows(discoverResult());

expect(rows[0].data.found_by).toBe('probe');
expect(rows[1].data.found_by).toBe('neighbor');
});

it('shows it in the table, not only in the detail pane', () => {
// The detail pane needs a selected row. A column is what makes a stale
// entry visible while scanning the list, which is where someone decides
// whether a device is really there.
const columns = columnsFor('discover', discoverResult(), buildRows(discoverResult()));

expect(columns.map((c) => c.key)).toContain('found');
expect(buildRows(discoverResult())[1].data.found).toMatch(/neighbour/i);
});
});
12 changes: 11 additions & 1 deletion apps/netscli-gui/src/tools/presentation/rowDetails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,17 @@ export function detailLinesForRow(row: ResultRow): DetailLine[] {
...lines,
{
label: 'Summary',
value: `Responded host${row.data.vendor ? ', known vendor' : ', vendor unknown'}`,
// "Responded host" was hardcoded, so a host that answered nothing
// and is only remembered by the OS neighbour table was reported as
// having replied. That is the one thing this product must never
// do, and it was doing it on the primary view while the raw tab
// one click away said `"found_by": "neighbor"`.
value: [
row.data.found_by === 'neighbor'
? 'No probe reply; listed in the neighbour table, so it may have left'
: 'Responded to a probe',
row.data.vendor ? 'known vendor' : 'vendor unknown',
].join(', '),
},
];
case 'sweep':
Expand Down
4 changes: 4 additions & 0 deletions apps/netscli-gui/src/tools/presentation/rows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ export function buildRows(result: ToolResult | null): ResultRow[] {
mac: host.mac ?? '',
vendor: host.vendor ?? '',
rtt: host.rtt_ms == null ? '' : `${host.rtt_ms} ms`,
// Carried through, not dropped. The table said "Responded host"
// for every row, including the ones that answered nothing.
found_by: host.found_by ?? '',
found: host.found_by === 'neighbor' ? 'neighbour table' : host.found_by ? 'probe reply' : '',
};
return {
id: `discover-${host.ip}-${index}`,
Expand Down
Loading