From 71ab3bf3917bc8713bb5e97933b04a8f201b6ba2 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Fri, 17 Jul 2026 13:02:14 +0200 Subject: [PATCH] fix(canvas): stop non-edits from marking the canvas unsaved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Autosave saves whenever hasUnsavedChanges is set, so any source that dirties the canvas without a user edit turns into a spurious silent save. Two such sources: 1. Live status updates. useStatusPolling routed backend status pushes through updateNode, which always sets hasUnsavedChanges. With the 60s status cycle, an idle opted-in tab saved itself every cycle and could clobber edits made elsewhere with its stale in-memory canvas. Add setNodeStatus, a live-overlay action that updates a node's status/response_time/last_seen without dirtying (mirrors setServiceStatuses), and route polling through it. 2. Initial dimensions measure. onNodesChange treated React Flow's first-measure `dimensions` change as an edit, marking a freshly loaded canvas dirty before any interaction — autosave then saved on every load/switch. Only count a `dimensions` change as an edit when it is a real resize (resizing === true). Add regression tests: setNodeStatus updates status without dirtying; onNodesChange ignores initial-measure dimensions and select-only changes but still dirties on a user resize. ha-relevant: maybe --- .../hooks/__tests__/useStatusPolling.test.ts | 22 ++++----- frontend/src/hooks/useStatusPolling.ts | 8 ++-- .../__tests__/canvasStore/nodes.test.ts | 45 +++++++++++++++++++ frontend/src/stores/canvasStore.ts | 38 +++++++++++++++- 4 files changed, 98 insertions(+), 15 deletions(-) diff --git a/frontend/src/hooks/__tests__/useStatusPolling.test.ts b/frontend/src/hooks/__tests__/useStatusPolling.test.ts index e8df523..1a89280 100644 --- a/frontend/src/hooks/__tests__/useStatusPolling.test.ts +++ b/frontend/src/hooks/__tests__/useStatusPolling.test.ts @@ -7,7 +7,7 @@ import { useAuthStore } from '@/stores/authStore' vi.mock('@/stores/canvasStore') vi.mock('@/stores/authStore') -const mockUpdateNode = vi.fn() +const mockSetNodeStatus = vi.fn() const mockNotifyScanDeviceFound = vi.fn() const mockSetServiceStatuses = vi.fn() @@ -32,7 +32,7 @@ describe('useStatusPolling', () => { vi.stubGlobal('WebSocket', MockWebSocket) vi.mocked(useCanvasStore).mockReturnValue({ - updateNode: mockUpdateNode, + setNodeStatus: mockSetNodeStatus, notifyScanDeviceFound: mockNotifyScanDeviceFound, setServiceStatuses: mockSetServiceStatuses, } as ReturnType) @@ -50,7 +50,7 @@ describe('useStatusPolling', () => { afterEach(() => { vi.restoreAllMocks() - mockUpdateNode.mockClear() + mockSetNodeStatus.mockClear() mockNotifyScanDeviceFound.mockClear() mockSetServiceStatuses.mockClear() }) @@ -95,7 +95,7 @@ describe('useStatusPolling', () => { expect(ws.send).toHaveBeenCalledWith(JSON.stringify({ token: 'test-token' })) }) - it('calls updateNode with correct data on status message', () => { + it('calls setNodeStatus with correct data on status message', () => { renderHook(() => useStatusPolling()) const ws = MockWebSocket.instances[0] ws.onmessage?.({ @@ -106,7 +106,7 @@ describe('useStatusPolling', () => { response_time_ms: 42, }), }) - expect(mockUpdateNode).toHaveBeenCalledWith('node-1', { + expect(mockSetNodeStatus).toHaveBeenCalledWith('node-1', { status: 'online', response_time_ms: 42, last_seen: '2024-01-01T12:00:00Z', @@ -123,7 +123,7 @@ describe('useStatusPolling', () => { checked_at: '2024-01-01T12:00:00Z', }), }) - expect(mockUpdateNode).toHaveBeenCalledWith('node-1', { + expect(mockSetNodeStatus).toHaveBeenCalledWith('node-1', { status: 'offline', response_time_ms: undefined, last_seen: undefined, @@ -136,7 +136,7 @@ describe('useStatusPolling', () => { ws.onmessage?.({ data: JSON.stringify({ node_id: 'node-1', status: 'online', response_time_ms: null }), }) - expect(mockUpdateNode).toHaveBeenCalledWith( + expect(mockSetNodeStatus).toHaveBeenCalledWith( 'node-1', expect.objectContaining({ response_time_ms: undefined }), ) @@ -147,7 +147,7 @@ describe('useStatusPolling', () => { const ws = MockWebSocket.instances[0] ws.onmessage?.({ data: JSON.stringify({ type: 'scan_device_found' }) }) expect(mockNotifyScanDeviceFound).toHaveBeenCalledOnce() - expect(mockUpdateNode).not.toHaveBeenCalled() + expect(mockSetNodeStatus).not.toHaveBeenCalled() }) it('routes service_status messages to setServiceStatuses', () => { @@ -158,21 +158,21 @@ describe('useStatusPolling', () => { data: JSON.stringify({ type: 'service_status', node_id: 'node-9', services }), }) expect(mockSetServiceStatuses).toHaveBeenCalledWith('node-9', services) - expect(mockUpdateNode).not.toHaveBeenCalled() + expect(mockSetNodeStatus).not.toHaveBeenCalled() }) it('ignores malformed JSON without throwing', () => { renderHook(() => useStatusPolling()) const ws = MockWebSocket.instances[0] expect(() => ws.onmessage?.({ data: 'not-valid-json{{' })).not.toThrow() - expect(mockUpdateNode).not.toHaveBeenCalled() + expect(mockSetNodeStatus).not.toHaveBeenCalled() }) it('ignores messages with no node_id or status', () => { renderHook(() => useStatusPolling()) const ws = MockWebSocket.instances[0] ws.onmessage?.({ data: JSON.stringify({ some: 'unknown-field' }) }) - expect(mockUpdateNode).not.toHaveBeenCalled() + expect(mockSetNodeStatus).not.toHaveBeenCalled() }) it('closes WebSocket on unmount', () => { diff --git a/frontend/src/hooks/useStatusPolling.ts b/frontend/src/hooks/useStatusPolling.ts index defd2bc..b21b340 100644 --- a/frontend/src/hooks/useStatusPolling.ts +++ b/frontend/src/hooks/useStatusPolling.ts @@ -24,7 +24,7 @@ const STANDALONE = import.meta.env.VITE_STANDALONE === 'true' export function useStatusPolling() { const wsRef = useRef(null) - const { updateNode, notifyScanDeviceFound, setServiceStatuses } = useCanvasStore() + const { setNodeStatus, notifyScanDeviceFound, setServiceStatuses } = useCanvasStore() const { isAuthenticated, token } = useAuthStore() useEffect(() => { @@ -50,7 +50,9 @@ export function useStatusPolling() { } else if (msg.type === 'service_status' && msg.node_id && msg.services) { setServiceStatuses(msg.node_id, msg.services) } else if (msg.node_id && msg.status) { - updateNode(msg.node_id, { + // Live status is monitoring data, not a user edit — must not dirty the + // canvas (otherwise autosave rewrites an untouched canvas every cycle). + setNodeStatus(msg.node_id, { status: msg.status, response_time_ms: msg.response_time_ms ?? undefined, last_seen: msg.status === 'online' ? msg.checked_at : undefined, @@ -69,5 +71,5 @@ export function useStatusPolling() { ws.close() wsRef.current = null } - }, [isAuthenticated, token, updateNode, notifyScanDeviceFound, setServiceStatuses]) + }, [isAuthenticated, token, setNodeStatus, notifyScanDeviceFound, setServiceStatuses]) } diff --git a/frontend/src/stores/__tests__/canvasStore/nodes.test.ts b/frontend/src/stores/__tests__/canvasStore/nodes.test.ts index 50d99c7..7bc3412 100644 --- a/frontend/src/stores/__tests__/canvasStore/nodes.test.ts +++ b/frontend/src/stores/__tests__/canvasStore/nodes.test.ts @@ -52,6 +52,51 @@ describe('canvasStore — nodes', () => { expect(useCanvasStore.getState().hasUnsavedChanges).toBe(false) }) + it('setNodeStatus updates node status fields', () => { + useCanvasStore.getState().addNode(makeNode('n1')) + useCanvasStore.getState().setNodeStatus('n1', { + status: 'online', + response_time_ms: 42, + last_seen: '2024-01-01T12:00:00Z', + }) + const node = useCanvasStore.getState().nodes.find((n) => n.id === 'n1') + expect(node?.data.status).toBe('online') + expect(node?.data.response_time_ms).toBe(42) + expect(node?.data.last_seen).toBe('2024-01-01T12:00:00Z') + }) + + it('setNodeStatus does NOT mark the canvas unsaved (live status is not a user edit)', () => { + useCanvasStore.getState().addNode(makeNode('n1')) + useCanvasStore.setState({ hasUnsavedChanges: false }) + useCanvasStore.getState().setNodeStatus('n1', { status: 'offline' }) + expect(useCanvasStore.getState().hasUnsavedChanges).toBe(false) + }) + + it('onNodesChange does NOT dirty the canvas on an initial dimensions measure', () => { + useCanvasStore.getState().addNode(makeNode({ id: 'n1' })) + useCanvasStore.setState({ hasUnsavedChanges: false }) + useCanvasStore.getState().onNodesChange([ + { id: 'n1', type: 'dimensions', dimensions: { width: 120, height: 40 } }, + ]) + expect(useCanvasStore.getState().hasUnsavedChanges).toBe(false) + }) + + it('onNodesChange DOES dirty the canvas on a user resize (resizing: true)', () => { + useCanvasStore.getState().addNode(makeNode({ id: 'n1' })) + useCanvasStore.setState({ hasUnsavedChanges: false }) + useCanvasStore.getState().onNodesChange([ + { id: 'n1', type: 'dimensions', dimensions: { width: 200, height: 80 }, resizing: true }, + ]) + expect(useCanvasStore.getState().hasUnsavedChanges).toBe(true) + }) + + it('onNodesChange does NOT dirty the canvas on a select-only change', () => { + useCanvasStore.getState().addNode(makeNode({ id: 'n1' })) + useCanvasStore.setState({ hasUnsavedChanges: false }) + useCanvasStore.getState().onNodesChange([{ id: 'n1', type: 'select', selected: true }]) + expect(useCanvasStore.getState().hasUnsavedChanges).toBe(false) + }) + it('setEditingTextId sets and clears editing text id', () => { const { setEditingTextId } = useCanvasStore.getState() setEditingTextId('t1') diff --git a/frontend/src/stores/canvasStore.ts b/frontend/src/stores/canvasStore.ts index 419c3b1..c8c8c91 100644 --- a/frontend/src/stores/canvasStore.ts +++ b/frontend/src/stores/canvasStore.ts @@ -22,6 +22,22 @@ type Clipboard = { nodes: Node[]; edges: Edge[] } /** Resolve a node's effective parent id from either the RF field or domain data. */ const parentIdOf = (n: Node): string | undefined => n.parentId ?? n.data.parent_id ?? undefined +/** + * Whether a node change represents a real user edit that should dirty the canvas. + * Excludes: + * - 'select': selecting a node changes nothing persisted. + * - 'dimensions' without resizing: React Flow emits these when it first measures + * a node's size after mount/load. Counting them as edits marks a freshly + * loaded canvas dirty before the user touches anything (autosave would then + * save on every load). A user-driven resize sets `resizing === true` and still + * dirties. + */ +function isUserNodeEdit(c: NodeChange>): boolean { + if (c.type === 'select') return false + if (c.type === 'dimensions' && c.resizing !== true) return false + return true +} + /** * Keep manually-routed edge waypoints attached to their nodes on drag (#279). * @@ -127,6 +143,14 @@ interface CanvasState { setSelectedNode: (id: string | null) => void addNode: (node: Node) => void updateNode: (id: string, data: Partial) => void + /** + * Apply a live status update to a node WITHOUT marking the canvas unsaved. + * Status (online/offline, response time, last seen) is transient monitoring + * data pushed by the backend, not a user edit — dirtying the canvas here would + * make autosave rewrite an untouched canvas on every status cycle and could + * clobber edits made elsewhere. Mirrors setServiceStatuses' live-overlay rule. + */ + setNodeStatus: (id: string, status: Pick) => void deleteNode: (id: string) => void updateEdge: (id: string, data: Partial) => void reconnectEdge: (id: string, connection: Connection) => void @@ -327,7 +351,7 @@ export const useCanvasStore = create((set) => ({ nodes, edges, selectedNodeIds, - hasUnsavedChanges: state.hasUnsavedChanges || changes.some((c) => c.type !== 'select'), + hasUnsavedChanges: state.hasUnsavedChanges || changes.some(isUserNodeEdit), } }), @@ -468,6 +492,18 @@ export const useCanvasStore = create((set) => ({ return { nodes, edges, hasUnsavedChanges: true } }), + setNodeStatus: (id, status) => + set((state) => { + let changed = false + const nodes = state.nodes.map((n) => { + if (n.id !== id) return n + changed = true + return { ...n, data: { ...n.data, ...status } } + }) + // No hasUnsavedChanges: live status is monitoring data, not a user edit. + return changed ? { nodes } : {} + }), + deleteNode: (id) => set((state) => { const idsToRemove = new Set()