fix(canvas): stop non-edits from marking the canvas unsaved
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
This commit is contained in:
@@ -7,7 +7,7 @@ import { useAuthStore } from '@/stores/authStore'
|
|||||||
vi.mock('@/stores/canvasStore')
|
vi.mock('@/stores/canvasStore')
|
||||||
vi.mock('@/stores/authStore')
|
vi.mock('@/stores/authStore')
|
||||||
|
|
||||||
const mockUpdateNode = vi.fn()
|
const mockSetNodeStatus = vi.fn()
|
||||||
const mockNotifyScanDeviceFound = vi.fn()
|
const mockNotifyScanDeviceFound = vi.fn()
|
||||||
const mockSetServiceStatuses = vi.fn()
|
const mockSetServiceStatuses = vi.fn()
|
||||||
|
|
||||||
@@ -32,7 +32,7 @@ describe('useStatusPolling', () => {
|
|||||||
vi.stubGlobal('WebSocket', MockWebSocket)
|
vi.stubGlobal('WebSocket', MockWebSocket)
|
||||||
|
|
||||||
vi.mocked(useCanvasStore).mockReturnValue({
|
vi.mocked(useCanvasStore).mockReturnValue({
|
||||||
updateNode: mockUpdateNode,
|
setNodeStatus: mockSetNodeStatus,
|
||||||
notifyScanDeviceFound: mockNotifyScanDeviceFound,
|
notifyScanDeviceFound: mockNotifyScanDeviceFound,
|
||||||
setServiceStatuses: mockSetServiceStatuses,
|
setServiceStatuses: mockSetServiceStatuses,
|
||||||
} as ReturnType<typeof useCanvasStore>)
|
} as ReturnType<typeof useCanvasStore>)
|
||||||
@@ -50,7 +50,7 @@ describe('useStatusPolling', () => {
|
|||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
vi.restoreAllMocks()
|
vi.restoreAllMocks()
|
||||||
mockUpdateNode.mockClear()
|
mockSetNodeStatus.mockClear()
|
||||||
mockNotifyScanDeviceFound.mockClear()
|
mockNotifyScanDeviceFound.mockClear()
|
||||||
mockSetServiceStatuses.mockClear()
|
mockSetServiceStatuses.mockClear()
|
||||||
})
|
})
|
||||||
@@ -95,7 +95,7 @@ describe('useStatusPolling', () => {
|
|||||||
expect(ws.send).toHaveBeenCalledWith(JSON.stringify({ token: 'test-token' }))
|
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())
|
renderHook(() => useStatusPolling())
|
||||||
const ws = MockWebSocket.instances[0]
|
const ws = MockWebSocket.instances[0]
|
||||||
ws.onmessage?.({
|
ws.onmessage?.({
|
||||||
@@ -106,7 +106,7 @@ describe('useStatusPolling', () => {
|
|||||||
response_time_ms: 42,
|
response_time_ms: 42,
|
||||||
}),
|
}),
|
||||||
})
|
})
|
||||||
expect(mockUpdateNode).toHaveBeenCalledWith('node-1', {
|
expect(mockSetNodeStatus).toHaveBeenCalledWith('node-1', {
|
||||||
status: 'online',
|
status: 'online',
|
||||||
response_time_ms: 42,
|
response_time_ms: 42,
|
||||||
last_seen: '2024-01-01T12:00:00Z',
|
last_seen: '2024-01-01T12:00:00Z',
|
||||||
@@ -123,7 +123,7 @@ describe('useStatusPolling', () => {
|
|||||||
checked_at: '2024-01-01T12:00:00Z',
|
checked_at: '2024-01-01T12:00:00Z',
|
||||||
}),
|
}),
|
||||||
})
|
})
|
||||||
expect(mockUpdateNode).toHaveBeenCalledWith('node-1', {
|
expect(mockSetNodeStatus).toHaveBeenCalledWith('node-1', {
|
||||||
status: 'offline',
|
status: 'offline',
|
||||||
response_time_ms: undefined,
|
response_time_ms: undefined,
|
||||||
last_seen: undefined,
|
last_seen: undefined,
|
||||||
@@ -136,7 +136,7 @@ describe('useStatusPolling', () => {
|
|||||||
ws.onmessage?.({
|
ws.onmessage?.({
|
||||||
data: JSON.stringify({ node_id: 'node-1', status: 'online', response_time_ms: null }),
|
data: JSON.stringify({ node_id: 'node-1', status: 'online', response_time_ms: null }),
|
||||||
})
|
})
|
||||||
expect(mockUpdateNode).toHaveBeenCalledWith(
|
expect(mockSetNodeStatus).toHaveBeenCalledWith(
|
||||||
'node-1',
|
'node-1',
|
||||||
expect.objectContaining({ response_time_ms: undefined }),
|
expect.objectContaining({ response_time_ms: undefined }),
|
||||||
)
|
)
|
||||||
@@ -147,7 +147,7 @@ describe('useStatusPolling', () => {
|
|||||||
const ws = MockWebSocket.instances[0]
|
const ws = MockWebSocket.instances[0]
|
||||||
ws.onmessage?.({ data: JSON.stringify({ type: 'scan_device_found' }) })
|
ws.onmessage?.({ data: JSON.stringify({ type: 'scan_device_found' }) })
|
||||||
expect(mockNotifyScanDeviceFound).toHaveBeenCalledOnce()
|
expect(mockNotifyScanDeviceFound).toHaveBeenCalledOnce()
|
||||||
expect(mockUpdateNode).not.toHaveBeenCalled()
|
expect(mockSetNodeStatus).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('routes service_status messages to setServiceStatuses', () => {
|
it('routes service_status messages to setServiceStatuses', () => {
|
||||||
@@ -158,21 +158,21 @@ describe('useStatusPolling', () => {
|
|||||||
data: JSON.stringify({ type: 'service_status', node_id: 'node-9', services }),
|
data: JSON.stringify({ type: 'service_status', node_id: 'node-9', services }),
|
||||||
})
|
})
|
||||||
expect(mockSetServiceStatuses).toHaveBeenCalledWith('node-9', services)
|
expect(mockSetServiceStatuses).toHaveBeenCalledWith('node-9', services)
|
||||||
expect(mockUpdateNode).not.toHaveBeenCalled()
|
expect(mockSetNodeStatus).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('ignores malformed JSON without throwing', () => {
|
it('ignores malformed JSON without throwing', () => {
|
||||||
renderHook(() => useStatusPolling())
|
renderHook(() => useStatusPolling())
|
||||||
const ws = MockWebSocket.instances[0]
|
const ws = MockWebSocket.instances[0]
|
||||||
expect(() => ws.onmessage?.({ data: 'not-valid-json{{' })).not.toThrow()
|
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', () => {
|
it('ignores messages with no node_id or status', () => {
|
||||||
renderHook(() => useStatusPolling())
|
renderHook(() => useStatusPolling())
|
||||||
const ws = MockWebSocket.instances[0]
|
const ws = MockWebSocket.instances[0]
|
||||||
ws.onmessage?.({ data: JSON.stringify({ some: 'unknown-field' }) })
|
ws.onmessage?.({ data: JSON.stringify({ some: 'unknown-field' }) })
|
||||||
expect(mockUpdateNode).not.toHaveBeenCalled()
|
expect(mockSetNodeStatus).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('closes WebSocket on unmount', () => {
|
it('closes WebSocket on unmount', () => {
|
||||||
|
|||||||
@@ -24,7 +24,7 @@ const STANDALONE = import.meta.env.VITE_STANDALONE === 'true'
|
|||||||
|
|
||||||
export function useStatusPolling() {
|
export function useStatusPolling() {
|
||||||
const wsRef = useRef<WebSocket | null>(null)
|
const wsRef = useRef<WebSocket | null>(null)
|
||||||
const { updateNode, notifyScanDeviceFound, setServiceStatuses } = useCanvasStore()
|
const { setNodeStatus, notifyScanDeviceFound, setServiceStatuses } = useCanvasStore()
|
||||||
const { isAuthenticated, token } = useAuthStore()
|
const { isAuthenticated, token } = useAuthStore()
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
@@ -50,7 +50,9 @@ export function useStatusPolling() {
|
|||||||
} else if (msg.type === 'service_status' && msg.node_id && msg.services) {
|
} else if (msg.type === 'service_status' && msg.node_id && msg.services) {
|
||||||
setServiceStatuses(msg.node_id, msg.services)
|
setServiceStatuses(msg.node_id, msg.services)
|
||||||
} else if (msg.node_id && msg.status) {
|
} 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,
|
status: msg.status,
|
||||||
response_time_ms: msg.response_time_ms ?? undefined,
|
response_time_ms: msg.response_time_ms ?? undefined,
|
||||||
last_seen: msg.status === 'online' ? msg.checked_at : undefined,
|
last_seen: msg.status === 'online' ? msg.checked_at : undefined,
|
||||||
@@ -69,5 +71,5 @@ export function useStatusPolling() {
|
|||||||
ws.close()
|
ws.close()
|
||||||
wsRef.current = null
|
wsRef.current = null
|
||||||
}
|
}
|
||||||
}, [isAuthenticated, token, updateNode, notifyScanDeviceFound, setServiceStatuses])
|
}, [isAuthenticated, token, setNodeStatus, notifyScanDeviceFound, setServiceStatuses])
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -52,6 +52,51 @@ describe('canvasStore — nodes', () => {
|
|||||||
expect(useCanvasStore.getState().hasUnsavedChanges).toBe(false)
|
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', () => {
|
it('setEditingTextId sets and clears editing text id', () => {
|
||||||
const { setEditingTextId } = useCanvasStore.getState()
|
const { setEditingTextId } = useCanvasStore.getState()
|
||||||
setEditingTextId('t1')
|
setEditingTextId('t1')
|
||||||
|
|||||||
@@ -22,6 +22,22 @@ type Clipboard = { nodes: Node<NodeData>[]; edges: Edge<EdgeData>[] }
|
|||||||
/** Resolve a node's effective parent id from either the RF field or domain data. */
|
/** Resolve a node's effective parent id from either the RF field or domain data. */
|
||||||
const parentIdOf = (n: Node<NodeData>): string | undefined => n.parentId ?? n.data.parent_id ?? undefined
|
const parentIdOf = (n: Node<NodeData>): 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<Node<NodeData>>): 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).
|
* Keep manually-routed edge waypoints attached to their nodes on drag (#279).
|
||||||
*
|
*
|
||||||
@@ -127,6 +143,14 @@ interface CanvasState {
|
|||||||
setSelectedNode: (id: string | null) => void
|
setSelectedNode: (id: string | null) => void
|
||||||
addNode: (node: Node<NodeData>) => void
|
addNode: (node: Node<NodeData>) => void
|
||||||
updateNode: (id: string, data: Partial<NodeData>) => void
|
updateNode: (id: string, data: Partial<NodeData>) => 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<NodeData, 'status' | 'response_time_ms' | 'last_seen'>) => void
|
||||||
deleteNode: (id: string) => void
|
deleteNode: (id: string) => void
|
||||||
updateEdge: (id: string, data: Partial<EdgeData>) => void
|
updateEdge: (id: string, data: Partial<EdgeData>) => void
|
||||||
reconnectEdge: (id: string, connection: Connection) => void
|
reconnectEdge: (id: string, connection: Connection) => void
|
||||||
@@ -327,7 +351,7 @@ export const useCanvasStore = create<CanvasState>((set) => ({
|
|||||||
nodes,
|
nodes,
|
||||||
edges,
|
edges,
|
||||||
selectedNodeIds,
|
selectedNodeIds,
|
||||||
hasUnsavedChanges: state.hasUnsavedChanges || changes.some((c) => c.type !== 'select'),
|
hasUnsavedChanges: state.hasUnsavedChanges || changes.some(isUserNodeEdit),
|
||||||
}
|
}
|
||||||
}),
|
}),
|
||||||
|
|
||||||
@@ -468,6 +492,18 @@ export const useCanvasStore = create<CanvasState>((set) => ({
|
|||||||
return { nodes, edges, hasUnsavedChanges: true }
|
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) =>
|
deleteNode: (id) =>
|
||||||
set((state) => {
|
set((state) => {
|
||||||
const idsToRemove = new Set<string>()
|
const idsToRemove = new Set<string>()
|
||||||
|
|||||||
Reference in New Issue
Block a user