From dca5fb227dc86e07ddb65fedf6b903061b2b8a9e Mon Sep 17 00:00:00 2001 From: Hacks4Snacks Date: Mon, 27 Jul 2026 15:40:57 -0500 Subject: [PATCH] studio bulk edit support --- docs/studio-guide.md | 26 +- .../src/dfd/Editor.test.ts | 54 ++++ .../src/dfd/Editor.tsx | 129 +++++--- .../src/dfd/Inspector.test.tsx | 214 ++++++++++++- .../src/dfd/Inspector.tsx | 288 +++++++++++++----- 5 files changed, 588 insertions(+), 123 deletions(-) diff --git a/docs/studio-guide.md b/docs/studio-guide.md index 8711967..9a176ea 100644 --- a/docs/studio-guide.md +++ b/docs/studio-guide.md @@ -51,7 +51,8 @@ connection, not the geometry. | Action | How | | --- | --- | | Rename a node or flow | Double-click it and type. | -| Delete the selection | `Delete` key. | +| Select several objects | Hold `Cmd` (`Ctrl` on Windows/Linux) and click, or drag a selection box with `Shift`. | +| Delete the selection | `Delete` key. Deleting an element takes its flows with it, and the whole deletion is a single undo step. | | Resize a trust boundary | Drag its handles (it's a resizable region). | | Tidy the diagram | Click **Tidy** to fit labels, separate overlapping shapes and peer trust boundaries, route flows, and deconflict flow labels. Nested boundaries remain nested, and each boundary moves with its members. | | Pan / zoom | Drag the canvas / scroll; use the minimap and **fit** control to navigate. | @@ -75,7 +76,7 @@ the CLI or API into `tmforge-json`) shows each source diagram on its own page. ### The inspector -The right-hand panel edits the selected element or flow. It is **schema-driven**: it lists a typed +The right-hand panel edits the selected elements or flows. It is **schema-driven**: it lists a typed control for every property the engine declares for that primitive, so every property an analysis rule can read is reachable, and you can clear any finding without leaving the canvas. A data flow, for example, exposes **Protocol**, **Port**, **Channel**, **DataType**, **Algorithm**, **Identity**, @@ -90,6 +91,27 @@ and the control is not there. `Unknown` does **not** clear a finding: the rule s the property is not evidenced rather than that the control is absent. See [`Unknown` and the three states of a control](analysis-rules.md#unknown-and-the-three-states-of-a-control). +#### Editing several objects at once + +Select any number of elements of the same kind, or any number of data flows, and the inspector edits +all of them. Each control writes the whole selection in one step, so one change is one undo. + +A property the selection disagrees about shows as **(mixed)** rather than picking one object's value +to display. `(mixed)` cannot be chosen, so simply opening the inspector on a mixed selection never +flattens it — only a property you actually change is written, and the rest are left as they are. +Clearing a property to **(none)** removes it from every selected object. + +Names are not offered across a selection: a name identifies one thing, so renaming several objects to +the same string is not an edit worth making. Delete still applies to the whole selection. + +Two cases deliberately offer delete but no property editing: + +- **Mixed kinds** (say processes and data stores together). The same property name does not accept + the same values on every kind — `AuthenticationScheme` allows `Token` and `PublicKey` on an + external entity but not on a process — so one shared control would offer values the schema rejects + for part of the selection. Narrow the selection to one kind. +- **Elements and flows together.** They have no properties in common at all. + ## Validating against the engine Click **Analyze** to send the whole model (every page) to the live `/v1` engine. Findings come back diff --git a/src/ThreatModelForge.Studio/src/dfd/Editor.test.ts b/src/ThreatModelForge.Studio/src/dfd/Editor.test.ts index 5ad2f12..15db8cc 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Editor.test.ts +++ b/src/ThreatModelForge.Studio/src/dfd/Editor.test.ts @@ -10,6 +10,8 @@ import { buildAnalysis, applyCanvasEdgeChanges, applyFlags, + deleteFromGraph, + sameIds, initialTheme, STORAGE_KEY, LEGACY_MODEL_KEY, @@ -369,3 +371,55 @@ describe('Editor — initialTheme', () => { expect(initialTheme()).toBe('light'); }); }); + +describe('Editor — deleting a selection', () => { + const a: DfdNode = { id: 'a', type: 'process', position: { x: 0, y: 0 }, data: { label: 'A' } }; + const b: DfdNode = { id: 'b', type: 'process', position: { x: 0, y: 0 }, data: { label: 'B' } }; + const c: DfdNode = { id: 'c', type: 'datastore', position: { x: 0, y: 0 }, data: { label: 'C' } }; + const ab: DfdEdge = { id: 'ab', source: 'a', target: 'b' }; + const bc: DfdEdge = { id: 'bc', source: 'b', target: 'c' }; + + it('deletes every selected element, not just the first', () => { + const result = deleteFromGraph([a, b, c], [], ['a', 'b'], []); + expect(result.nodes.map((n) => n.id)).toEqual(['c']); + }); + + it('takes the flows connected to a deleted element with it, at either end', () => { + const result = deleteFromGraph([a, b, c], [ab, bc], ['b'], []); + // 'b' is the target of ab and the source of bc; both go. + expect(result.nodes.map((n) => n.id)).toEqual(['a', 'c']); + expect(result.edges).toEqual([]); + }); + + it('deletes a selected flow without touching its endpoints', () => { + const result = deleteFromGraph([a, b, c], [ab, bc], [], ['ab']); + expect(result.nodes.map((n) => n.id)).toEqual(['a', 'b', 'c']); + expect(result.edges.map((e) => e.id)).toEqual(['bc']); + }); + + it('handles a selection holding both elements and flows', () => { + const result = deleteFromGraph([a, b, c], [ab, bc], ['a'], ['bc']); + expect(result.nodes.map((n) => n.id)).toEqual(['b', 'c']); + // 'ab' went with 'a' even though only 'bc' was selected. + expect(result.edges).toEqual([]); + }); + + it('leaves the graph alone when nothing is selected', () => { + const result = deleteFromGraph([a, b], [ab], [], []); + expect(result.nodes).toEqual([a, b]); + expect(result.edges).toEqual([ab]); + }); +}); + +describe('Editor — sameIds', () => { + it('recognizes an unchanged selection so the Inspector is not remounted mid-edit', () => { + expect(sameIds([], [])).toBe(true); + expect(sameIds(['a', 'b'], ['a', 'b'])).toBe(true); + }); + + it('sees a changed selection', () => { + expect(sameIds(['a'], ['a', 'b'])).toBe(false); + expect(sameIds(['a', 'b'], ['b', 'a'])).toBe(false); + expect(sameIds(['a'], ['b'])).toBe(false); + }); +}); diff --git a/src/ThreatModelForge.Studio/src/dfd/Editor.tsx b/src/ThreatModelForge.Studio/src/dfd/Editor.tsx index 751fa36..7db672c 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Editor.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/Editor.tsx @@ -342,6 +342,32 @@ export function applyCanvasEdgeChanges(changes: EdgeChange[], current: return applyEdgeChangesToGraph(geometrySafe, current); } +/** True when two id lists hold the same ids in the same order. */ +export function sameIds(a: readonly string[], b: readonly string[]): boolean { + return a.length === b.length && a.every((id, index) => id === b[index]); +} + +/** + * The graph left after deleting a selection. Deleting an element takes its flows with it, because a + * flow with a missing endpoint is not a model — so a selection of elements removes more edges than + * the ones the author selected. + */ +export function deleteFromGraph( + nodes: DfdNode[], + edges: DfdEdge[], + doomedNodeIds: readonly string[], + doomedEdgeIds: readonly string[], +): { nodes: DfdNode[]; edges: DfdEdge[] } { + const doomedNodes = new Set(doomedNodeIds); + const doomedEdges = new Set(doomedEdgeIds); + return { + nodes: nodes.filter((n) => !doomedNodes.has(n.id)), + edges: edges.filter( + (e) => !doomedEdges.has(e.id) && !doomedNodes.has(e.source) && !doomedNodes.has(e.target), + ), + }; +} + export function Editor() { const [pages, setPages] = useState(INITIAL_WORKSPACE.pages); const [activePageId, setActivePageId] = useState(INITIAL_ACTIVE.id); @@ -354,7 +380,9 @@ export function Editor() { const [engine, setEngine] = useState(offlineEngine); const [engineOnline, setEngineOnline] = useState(false); const [formats, setFormats] = useState([]); - const [selection, setSelection] = useState<{ node: string | null; edge: string | null }>({ node: null, edge: null }); + // The whole selection, not just its first member: the Inspector edits every selected element, and + // a bulk delete has to take the connected flows with it. + const [selection, setSelection] = useState<{ nodes: string[]; edges: string[] }>({ nodes: [], edges: [] }); const [stencils, setStencils] = useState(FALLBACK_STENCILS); const [recentStencilIds, setRecentStencilIds] = useState(loadRecentStencilIds); const [packs, setPacks] = useState(FALLBACK_PACKS); @@ -604,7 +632,7 @@ export function Editor() { : { nodes: target.nodes, edges: target.edges }; setNodes(applied.nodes); setEdges(applied.edges); - setSelection({ node: null, edge: null }); + setSelection({ nodes: [], edges: [] }); reset(); window.setTimeout(() => fitView({ padding: 0.25, maxZoom: 1.15, duration: 200 }), 0); }, @@ -617,7 +645,7 @@ export function Editor() { setActivePageId(id); setNodes([]); setEdges([]); - setSelection({ node: null, edge: null }); + setSelection({ nodes: [], edges: [] }); reset(); }, [allPages, pages.length, setNodes, setEdges, reset]); @@ -650,7 +678,7 @@ export function Editor() { : { nodes: next.nodes, edges: next.edges }; setNodes(applied.nodes); setEdges(applied.edges); - setSelection({ node: null, edge: null }); + setSelection({ nodes: [], edges: [] }); reset(); } }, @@ -784,10 +812,7 @@ export function Editor() { takeSnapshot(); setNodes((nds) => [...nds.map((n) => (n.selected ? { ...n, selected: false } : n)), ...clone.nodes]); setEdges((eds) => [...eds.map((e) => (e.selected ? { ...e, selected: false } : e)), ...clone.edges]); - setSelection({ - node: clone.nodes[0]?.id ?? null, - edge: clone.nodes.length === 0 ? clone.edges[0]?.id ?? null : null, - }); + setSelection({ nodes: clone.nodes.map((n) => n.id), edges: clone.edges.map((e) => e.id) }); }, [setNodes, setEdges, takeSnapshot], ); @@ -1010,7 +1035,15 @@ export function Editor() { const onSelectionChange = useCallback( ({ nodes: selectedNodes, edges: selectedEdges }: { nodes: DfdNode[]; edges: DfdEdge[] }) => { - setSelection({ node: selectedNodes[0]?.id ?? null, edge: selectedEdges[0]?.id ?? null }); + setSelection((prev) => { + const nextNodes = selectedNodes.map((n) => n.id); + const nextEdges = selectedEdges.map((e) => e.id); + // React Flow re-emits the selection on every graph change. Returning the previous object when + // nothing moved keeps the Inspector from remounting mid-edit (which would drop focus). + return sameIds(prev.nodes, nextNodes) && sameIds(prev.edges, nextEdges) + ? prev + : { nodes: nextNodes, edges: nextEdges }; + }); }, [], ); @@ -1037,12 +1070,18 @@ export function Editor() { [setEdges], ); + // The property writers take a set of ids so a bulk edit is one state update under one snapshot. + // A single selection is just the one-element case, so there is only ever one code path. const setEdgeProperty = useCallback( - (id: string, key: string, value: string) => { + (ids: string[], key: string, value: string) => { + if (ids.length === 0) { + return; + } takeSnapshot(); + const targets = new Set(ids); setEdges((eds) => eds.map((e) => { - if (e.id !== id) { + if (!targets.has(e.id)) { return e; } const properties = { ...(e.data?.properties ?? {}) }; @@ -1059,11 +1098,15 @@ export function Editor() { ); const setNodeProperty = useCallback( - (id: string, key: string, value: string) => { + (ids: string[], key: string, value: string) => { + if (ids.length === 0) { + return; + } takeSnapshot(); + const targets = new Set(ids); setNodes((nds) => nds.map((n) => { - if (n.id !== id) { + if (!targets.has(n.id)) { return n; } const properties = { ...(n.data.properties ?? {}) }; @@ -1076,11 +1119,15 @@ export function Editor() { ); const removeNodeProperty = useCallback( - (id: string, key: string) => { + (ids: string[], key: string) => { + if (ids.length === 0) { + return; + } takeSnapshot(); + const targets = new Set(ids); setNodes((nds) => nds.map((n) => { - if (n.id !== id) { + if (!targets.has(n.id)) { return n; } const properties = { ...(n.data.properties ?? {}) }; @@ -1093,21 +1140,28 @@ export function Editor() { ); const deleteSelected = useCallback(() => { - if (!selection.node && !selection.edge) { + if (selection.nodes.length === 0 && selection.edges.length === 0) { return; } takeSnapshot(); - if (selection.node) { - const nodeId = selection.node; - setNodes((nds) => nds.filter((n) => n.id !== nodeId)); - setEdges((eds) => eds.filter((e) => e.source !== nodeId && e.target !== nodeId)); - } else if (selection.edge) { - const edgeId = selection.edge; - setEdges((eds) => eds.filter((e) => e.id !== edgeId)); - } - setSelection({ node: null, edge: null }); + setNodes((nds) => deleteFromGraph(nds, [], selection.nodes, selection.edges).nodes); + setEdges((eds) => deleteFromGraph([], eds, selection.nodes, selection.edges).edges); + setSelection({ nodes: [], edges: [] }); }, [selection, setNodes, setEdges, takeSnapshot]); + // React Flow fires onNodesDelete AND onEdgesDelete for a single deletion (removing an element takes + // its flows with it), so snapshotting in both would cost two undos for one Backspace. onBeforeDelete + // fires exactly once, before anything is removed, with the connected flows already resolved. + const beforeDelete = useCallback( + ({ nodes: doomedNodes, edges: doomedEdges }: { nodes: DfdNode[]; edges: DfdEdge[] }) => { + if (doomedNodes.length > 0 || doomedEdges.length > 0) { + takeSnapshot(); + } + return Promise.resolve(true); + }, + [takeSnapshot], + ); + const clearFlags = useCallback(() => { setNodes((nds) => nds.map((n) => (n.className ? { ...n, className: undefined } : n))); setEdges((eds) => eds.map((e) => (e.className ? { ...e, className: undefined } : e))); @@ -1316,7 +1370,7 @@ export function Editor() { if (item.kind === 'flow') { setNodes((nds) => nds.map((n) => (n.selected ? { ...n, selected: false } : n))); setEdges((eds) => eds.map((e) => (e.selected !== (e.id === item.id) ? { ...e, selected: e.id === item.id } : e))); - setSelection({ node: null, edge: item.id }); + setSelection({ nodes: [], edges: [item.id] }); const edge = allPages.find((p) => p.id === item.pageId)?.edges.find((e) => e.id === item.id); const ends = [edge?.source, edge?.target].filter((id): id is string => Boolean(id)).map((id) => ({ id })); if (ends.length > 0) { @@ -1325,7 +1379,7 @@ export function Editor() { } else { setEdges((eds) => eds.map((e) => (e.selected ? { ...e, selected: false } : e))); setNodes((nds) => nds.map((n) => (n.selected !== (n.id === item.id) ? { ...n, selected: n.id === item.id } : n))); - setSelection({ node: item.id, edge: null }); + setSelection({ nodes: [item.id], edges: [] }); fitView({ nodes: [{ id: item.id }], padding: 0.6, duration: 400, maxZoom: 1.4 }); } }; @@ -1400,7 +1454,7 @@ export function Editor() { setThreatTriage(model.threats ?? []); flaggedIdsRef.current = new Set(); analysisActiveRef.current = false; - setSelection({ node: null, edge: null }); + setSelection({ nodes: [], edges: [] }); reset(); // A freshly loaded model is the new saved baseline, so it does not read as dirty. setSavedJson( @@ -1477,7 +1531,7 @@ export function Editor() { setThreatTriage([]); flaggedIdsRef.current = new Set(); analysisActiveRef.current = false; - setSelection({ node: null, edge: null }); + setSelection({ nodes: [], edges: [] }); reset(); }, [setNodes, setEdges, reset]); @@ -1501,8 +1555,14 @@ export function Editor() { [takeSnapshot, renameNode, renameEdge, setEdgeLabelOffset], ); - const selectedNode = nodes.find((n) => n.id === selection.node) ?? null; - const selectedEdge = edges.find((e) => e.id === selection.edge) ?? null; + const selectedNodes = useMemo(() => { + const ids = new Set(selection.nodes); + return nodes.filter((n) => ids.has(n.id)); + }, [nodes, selection.nodes]); + const selectedEdges = useMemo(() => { + const ids = new Set(selection.edges); + return edges.filter((e) => ids.has(e.id)); + }, [edges, selection.edges]); return ( @@ -1550,8 +1610,7 @@ export function Editor() { onConnect={onConnect} onReconnect={onReconnect} onNodeDragStart={takeSnapshot} - onNodesDelete={takeSnapshot} - onEdgesDelete={takeSnapshot} + onBeforeDelete={beforeDelete} onSelectionChange={onSelectionChange} onPaneClick={findings.length || threats.length ? clearFlags : undefined} nodeTypes={nodeTypes} @@ -1684,8 +1743,8 @@ export function Editor() { /> = {}): DfdNode { return { id: 'n1', type: 'process', position: { x: 0, y: 0 }, data: { label: 'API', properties } }; } +/** A second process, so a multi-selection can be built without repeating the literal. */ +function otherProcess(id: string, properties: Record = {}): DfdNode { + return { id, type: 'process', position: { x: 0, y: 0 }, data: { label: id, properties } }; +} + describe('Inspector — data flow', () => { it('renders a typed control for every flow schema property (incl. the previously-missing Port/Algorithm)', () => { const h = handlers(); - render(); + render(); // Every flow property is now reachable — this is the core regression guard. for (const name of ['Protocol', 'Port', 'DataType', 'Algorithm', 'Cached']) { @@ -54,7 +64,7 @@ describe('Inspector — data flow', () => { it('renders enum properties as dropdowns of the schema values and strings as text inputs', () => { const h = handlers(); - render(); + render(); const protocol = screen.getByLabelText('Protocol'); expect(protocol.tagName).toBe('SELECT'); @@ -66,38 +76,38 @@ describe('Inspector — data flow', () => { it('setting a flow property emits the canonical value via onSetEdgeProperty', () => { const h = handlers(); - render(); + render(); fireEvent.change(screen.getByLabelText('Protocol'), { target: { value: 'HTTPS' } }); - expect(h.onSetEdgeProperty).toHaveBeenCalledWith('f1', 'Protocol', 'HTTPS'); + expect(h.onSetEdgeProperty).toHaveBeenCalledWith(['f1'], 'Protocol', 'HTTPS'); fireEvent.change(screen.getByLabelText('Port'), { target: { value: '443' } }); - expect(h.onSetEdgeProperty).toHaveBeenCalledWith('f1', 'Port', '443'); + expect(h.onSetEdgeProperty).toHaveBeenCalledWith(['f1'], 'Port', '443'); }); it('clearing a flow property to "(none)" removes it', () => { const h = handlers(); - render(); + render(); fireEvent.change(screen.getByLabelText('Protocol'), { target: { value: '' } }); - // The edge's remove path is onSetEdgeProperty(id, key, '') — empty clears the property. - expect(h.onSetEdgeProperty).toHaveBeenCalledWith('f1', 'Protocol', ''); + // The edge's remove path is onSetEdgeProperty(ids, key, '') — empty clears the property. + expect(h.onSetEdgeProperty).toHaveBeenCalledWith(['f1'], 'Protocol', ''); }); it('shows a non-schema property under Custom properties with a remove control', () => { const h = handlers(); - render(); + render(); expect(screen.getByText('Custom properties')).toBeInTheDocument(); fireEvent.click(screen.getByTitle('Remove Legacy')); - expect(h.onSetEdgeProperty).toHaveBeenCalledWith('f1', 'Legacy', ''); + expect(h.onSetEdgeProperty).toHaveBeenCalledWith(['f1'], 'Legacy', ''); }); }); describe('Inspector — element', () => { it('renders the selected kind\'s schema controls and writes via onSetNodeProperty', () => { const h = handlers(); - render(); + render(); const scheme = screen.getByLabelText('AuthenticationScheme'); expect(scheme).toBeInTheDocument(); @@ -105,15 +115,193 @@ describe('Inspector — element', () => { expect(screen.queryByLabelText('Protocol')).not.toBeInTheDocument(); fireEvent.change(scheme, { target: { value: 'OAuth' } }); - expect(h.onSetNodeProperty).toHaveBeenCalledWith('n1', 'AuthenticationScheme', 'OAuth'); + expect(h.onSetNodeProperty).toHaveBeenCalledWith(['n1'], 'AuthenticationScheme', 'OAuth'); }); it('falls back to a free-text add row when the engine schema is unavailable (offline)', () => { const h = handlers(); - render(); + render(); // No typed controls, but the author can still add properties by name. expect(screen.queryByLabelText('AuthenticationScheme')).not.toBeInTheDocument(); expect(screen.getByPlaceholderText('key')).toBeInTheDocument(); }); }); + +describe('Inspector — multi-selection', () => { + it('shows the shared value when every selected element agrees', () => { + const h = handlers(); + render( + , + ); + + expect((screen.getByLabelText('AuthenticationScheme') as HTMLSelectElement).value).toBe('OAuth'); + expect(screen.queryByText('(mixed)')).not.toBeInTheDocument(); + }); + + it('shows a disagreed property as (mixed) and writes nothing just by rendering', () => { + const h = handlers(); + render( + , + ); + + const scheme = screen.getByLabelText('AuthenticationScheme'); + const mixed = within(scheme).getByRole('option', { name: '(mixed)' }); + expect(mixed).toBeInTheDocument(); + // Disabled, so the placeholder can never be committed back over the real values. + expect(mixed).toBeDisabled(); + // Rendering a mixed selection must not flatten it. + expect(h.onSetNodeProperty).not.toHaveBeenCalled(); + expect(h.onRemoveNodeProperty).not.toHaveBeenCalled(); + }); + + it('a property set on only some of the selection counts as mixed, not as the value one of them has', () => { + const h = handlers(); + render( + , + ); + + expect(within(screen.getByLabelText('AuthenticationScheme')).getByRole('option', { name: '(mixed)' })).toBeInTheDocument(); + }); + + it('changing a property writes it to every selected element at once', () => { + const h = handlers(); + render( + , + ); + + fireEvent.change(screen.getByLabelText('AuthenticationScheme'), { target: { value: 'OAuth' } }); + expect(h.onSetNodeProperty).toHaveBeenCalledWith(['n1', 'n2'], 'AuthenticationScheme', 'OAuth'); + }); + + it('writes only the field the author changed, leaving the other mixed fields alone', () => { + const h = handlers(); + render( + , + ); + + fireEvent.change(screen.getByLabelText('SanitizesInput'), { target: { value: 'No' } }); + + // Exactly one write, for exactly the field that was touched. The two mixed fields stay mixed. + expect(h.onSetNodeProperty).toHaveBeenCalledTimes(1); + expect(h.onSetNodeProperty).toHaveBeenCalledWith(['n1', 'n2'], 'SanitizesInput', 'No'); + }); + + it('clearing a property to "(none)" removes it from every selected element', () => { + const h = handlers(); + render( + , + ); + + fireEvent.change(screen.getByLabelText('AuthenticationScheme'), { target: { value: '' } }); + expect(h.onRemoveNodeProperty).toHaveBeenCalledWith(['n1', 'n2'], 'AuthenticationScheme'); + }); + + it('lists a custom property held by any of the selection once, and removes it from all of them', () => { + const h = handlers(); + render( + , + ); + + // One row, showing that the selection disagrees rather than showing one element's value. + expect(screen.getByPlaceholderText('(mixed)')).toBeInTheDocument(); + fireEvent.click(screen.getByTitle('Remove Owner')); + expect(h.onRemoveNodeProperty).toHaveBeenCalledWith(['n1', 'n2'], 'Owner'); + }); + + it('offers no Name field for a multi-selection, and counts the elements it will delete', () => { + const h = handlers(); + render( + , + ); + + // Renaming three elements to one name is not an edit anyone wants. + expect(screen.queryByLabelText('Name')).not.toBeInTheDocument(); + expect(screen.getByText('3 processes')).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'Delete 3 elements' })); + expect(h.onDelete).toHaveBeenCalled(); + }); + + it('bulk-edits data flows the same way', () => { + const h = handlers(); + const second: DfdEdge = { id: 'f2', source: 'n2', target: 'n3', label: 'reply', data: { properties: {} } }; + render(); + + // A flow label is per-flow, so it is not offered across a selection. + expect(screen.queryByLabelText('Label')).not.toBeInTheDocument(); + fireEvent.change(screen.getByLabelText('Protocol'), { target: { value: 'HTTPS' } }); + expect(h.onSetEdgeProperty).toHaveBeenCalledWith(['f1', 'f2'], 'Protocol', 'HTTPS'); + }); + + it('refuses typed editing across kinds, because a shared property name does not share its values', () => { + const h = handlers(); + const store: DfdNode = { id: 'd1', type: 'datastore', position: { x: 0, y: 0 }, data: { label: 'DB' } }; + render(); + + expect(screen.queryByLabelText('AuthenticationScheme')).not.toBeInTheDocument(); + expect(screen.getByText(/different kinds/i)).toBeInTheDocument(); + // Deleting is still unambiguous, so it stays on offer. + expect(screen.getByRole('button', { name: 'Delete 2 elements' })).toBeInTheDocument(); + }); + + it('offers only delete when the selection mixes elements and flows', () => { + const h = handlers(); + render(); + + expect(screen.queryByLabelText('AuthenticationScheme')).not.toBeInTheDocument(); + expect(screen.queryByLabelText('Protocol')).not.toBeInTheDocument(); + expect(screen.getByText(/no properties in common/i)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Delete 2 items' })).toBeInTheDocument(); + }); +}); diff --git a/src/ThreatModelForge.Studio/src/dfd/Inspector.tsx b/src/ThreatModelForge.Studio/src/dfd/Inspector.tsx index 6e24bfd..f13b391 100644 --- a/src/ThreatModelForge.Studio/src/dfd/Inspector.tsx +++ b/src/ThreatModelForge.Studio/src/dfd/Inspector.tsx @@ -9,9 +9,58 @@ const KIND_LABEL: Record = { boundary: 'Trust boundary', }; +const KIND_PLURAL: Record = { + process: 'processes', + datastore: 'data stores', + external: 'external entities', + boundary: 'trust boundaries', +}; + +/** + * The value shown for a property the selection disagrees about. It is only ever rendered as a + * disabled option, so it can never be committed; the NUL prefix keeps it from colliding with a real + * schema value. + */ +const MIXED = '\u0000mixed'; +const MIXED_LABEL = '(mixed)'; + +/** The shared value of one property across a selection, and whether the selection disagrees. */ +export interface PropertySummary { + /** The common value, or empty when the selection disagrees. */ + value: string; + /** True when at least two selected items hold different values for the property. */ + mixed: boolean; +} + +/** + * Reduces one property across the selection. An item that does not carry the property counts as + * empty, so a value set on some-but-not-all of the selection reads as mixed rather than as the value + * one of them happens to have. + */ +export function summarizeProperty(bags: readonly Record[], key: string): PropertySummary { + const first = bags[0]?.[key] ?? ''; + const mixed = bags.some((bag) => (bag[key] ?? '') !== first); + return { value: mixed ? '' : first, mixed }; +} + +/** The distinct custom (non-schema) property keys across the selection, in first-seen order. */ +export function customKeys(bags: readonly Record[], schemaNames: ReadonlySet): string[] { + const keys: string[] = []; + for (const bag of bags) { + for (const key of Object.keys(bag)) { + if (!schemaNames.has(key) && !keys.includes(key)) { + keys.push(key); + } + } + } + return keys; +} + interface InspectorProps { - node: DfdNode | null; - edge: DfdEdge | null; + /** Every selected element: none, one, or many. */ + nodes: DfdNode[]; + /** Every selected data flow: none, one, or many. */ + edges: DfdEdge[]; /** The stencil catalog, so a specialized node can show its stencil identity. */ stencils: StencilInfo[]; /** The typed property schema, so element properties render as dropdowns/checkboxes with canonical values. */ @@ -21,11 +70,11 @@ interface InspectorProps { onRenameNode: (id: string, label: string) => void; onRenameEdge: (id: string, label: string) => void; /** Sets (or, when value is empty, clears) a flow custom property such as Protocol or DataType. */ - onSetEdgeProperty: (id: string, key: string, value: string) => void; + onSetEdgeProperty: (ids: string[], key: string, value: string) => void; /** Adds or updates an element custom property. */ - onSetNodeProperty: (id: string, key: string, value: string) => void; + onSetNodeProperty: (ids: string[], key: string, value: string) => void; /** Removes an element custom property. */ - onRemoveNodeProperty: (id: string, key: string) => void; + onRemoveNodeProperty: (ids: string[], key: string) => void; onDelete: () => void; } @@ -35,35 +84,44 @@ interface InspectorProps { * analysis rule can read is reachable — followed by any custom (non-schema) properties and a * free-form add row. Enum and boolean properties render as dropdowns of canonical values; string * properties render as text inputs. Selecting "(none)" or clearing a value removes the property. + * + * Over a multi-selection every control writes the whole selection. A property the selection disagrees + * about renders as "(mixed)" and is left alone unless the author changes it, so opening the Inspector + * on a mixed selection never flattens the values it is showing. */ function PropertyFields(props: { /** The DFD primitive whose schema to render: 'process' | 'datastore' | 'external' | 'flow'. */ appliesTo: string; - /** The custom properties currently set on the element or flow. */ - properties: Record; + /** The custom properties of each selected element or flow, one bag per item. */ + selection: Record[]; /** The full typed property schema; filtered here by the primitive it applies to. */ schema: PropertyDescriptorInfo[]; - /** Adds or updates a property. */ + /** Adds or updates a property across the whole selection. */ onSet: (key: string, value: string) => void; - /** Removes a property. */ + /** Removes a property across the whole selection. */ onRemove: (key: string) => void; /** Called when a free-text edit begins, so one undo step covers the whole edit. */ onBeginEdit: () => void; }) { - const { appliesTo, properties, schema, onSet, onRemove, onBeginEdit } = props; + const { appliesTo, selection, schema, onSet, onRemove, onBeginEdit } = props; const [newKey, setNewKey] = useState(''); const [newValue, setNewValue] = useState(''); const schemaFor = schema.filter((descriptor) => descriptor.appliesTo === appliesTo); const schemaNames = new Set(schemaFor.map((descriptor) => descriptor.name)); - const custom = Object.entries(properties).filter(([key]) => !schemaNames.has(key)); + const custom = customKeys(selection, schemaNames); const typedControl = (descriptor: PropertyDescriptorInfo) => { - const value = properties[descriptor.name] ?? ''; + const { value, mixed } = summarizeProperty(selection, descriptor.name); const commit = (next: string) => (next ? onSet(descriptor.name, next) : onRemove(descriptor.name)); if (descriptor.kind === 'enum' || descriptor.kind === 'bool') { return ( - commit(event.target.value)}> + {mixed && ( + + )} {descriptor.values.map((option) => (