ソースを参照

fix: clear every react-hooks/exhaustive-deps warning in the webui

Fifteen, not the seven the note recorded - the file had grown since. Each was
judged on its own; none was silenced without a reason written next to it.

Real defects found:

- DatabasePage's Ctrl+S handler called a saveDocument captured under a
  dependency list that omitted selectedCollection, so it could write a document
  to whichever collection was open when the listener was last registered. It
  now goes through a ref holding the current one, the idiom the editor already
  uses for savedByOtherRef.
- ExecutionsPage's debounced search omitted `search`, and updateParams was
  rebuilt every render so it could not be listed without restarting the 300ms
  timer continuously. updateParams is now a useCallback over setSearchParams,
  which react-router keeps stable, and both are listed.

The rest were arrays rebuilt by `|| []` on every render (nodes, nodeDefs,
workflows, collections), which defeated every useMemo keyed on them. Memoised.

Three omissions are deliberate and now say so:

- The editor's graph-load effect must not depend on executionState, or it would
  rebuild the canvas several times a second during a run and discard live node
  state as it arrived.
- navigateToConnectedNode is declared after the effect that calls it, so naming
  it would read it before initialisation; focusedNodeId, the value that could
  go stale, is already listed.
- addNode is rebuilt every render and writes only through the functional form
  of setNodes/setEdges, so a captured copy is not stale.

The useWorkflowLocks cleanup keeps reading heldRef.current on purpose: the
rule's usual advice would release the set as it was at mount - empty - and
leave every lock taken afterwards behind.

eslint and tsc are both clean, and the editor was loaded in a browser
afterwards to check none of this broke it: canvas, toolbar and the executions
panel all render with no console errors.
fszontagh 1 ヶ月 前
親
コミット
0d4734875f

+ 5 - 0
webui/src/hooks/useWorkflowLocks.ts

@@ -104,6 +104,11 @@ export function useWorkflowLocks(
       // the connection closes, but navigating to another page does not close
       // it - and leaving a node locked behind you is indistinguishable from a
       // bug to whoever tries to touch it next.
+      // heldRef.current is read here on purpose, and the rule's usual advice -
+      // copy it into a variable inside the effect - would break this: the set is
+      // empty when the effect runs and fills up as locks are taken, so releasing
+      // the snapshot would release nothing and leave every lock behind.
+      // eslint-disable-next-line react-hooks/exhaustive-deps
       for (const nodeId of heldRef.current) {
         wsClient.send({ type: 'unlock', workflowId, nodeId })
       }

+ 4 - 1
webui/src/hooks/useWorkflowPresence.ts

@@ -90,7 +90,10 @@ export function useWorkflowPresence(
       setViewers([])
       setSavedByOther(null)
     }
-  }, [workflowId])
+  // ownVersionRef is a useRef from the caller, so its identity never changes
+  // and listing it cannot re-subscribe anything. Listed rather than silenced
+  // because there is nothing here to argue with.
+  }, [workflowId, ownVersionRef])
 
   const others = viewers.filter((v) => v.userId !== currentUserId)
 

+ 15 - 5
webui/src/pages/DatabasePage.tsx

@@ -229,7 +229,7 @@ export default function DatabasePage() {
       if ((e.ctrlKey || e.metaKey) && e.key === 's') {
         e.preventDefault()
         if ((editingDocument || showNewDocument) && !jsonError) {
-          saveDocument()
+          saveDocumentRef.current()
         }
       }
       // Delete key to delete selected documents
@@ -255,6 +255,12 @@ export default function DatabasePage() {
     }
   }, [])
 
+  // Called from the keydown listener through a ref so it is always the current
+  // one. The listener is registered under a dependency list that does not
+  // include selectedCollection, so the copy it captured could still be writing
+  // to whichever collection was open when it was last registered.
+  const saveDocumentRef = useRef<() => void>(() => {})
+
   const saveDocument = () => {
     if (!selectedCollection || jsonError) return
     try {
@@ -276,6 +282,8 @@ export default function DatabasePage() {
     }
   }
 
+  saveDocumentRef.current = saveDocument
+
   const handleDeleteRequest = (ids: string[]) => {
     setDocumentsToDelete(ids)
     setShowDeleteModal(true)
@@ -382,12 +390,14 @@ export default function DatabasePage() {
     }
   }
 
-  const fetched: string[] = collectionsData?.collections || []
+  // Memoised together: both build a new array on every render, so the useMemo
+  // that splits them into system and custom recomputed every time.
+  const fetched: string[] = useMemo(() => collectionsData?.collections || [], [collectionsData])
   // Anything created this session that the server cannot list yet.
-  const collections: string[] = [
+  const collections: string[] = useMemo(() => [
     ...fetched,
     ...justCreated.filter((name) => !fetched.includes(name)),
-  ]
+  ], [fetched, justCreated])
   const documents: Document[] = documentsData?.documents || []
   // A collection with nothing in it cannot be queried at all, so its document
   // list comes back as an error rather than an empty page.
@@ -450,7 +460,7 @@ export default function DatabasePage() {
     custom.sort()
 
     return { systemCollections: system, customCollections: custom }
-  }, [collections])
+  }, [collections, protectionOf])
 
   // The database writes these in nanoseconds. Dividing blindly would be wrong
   // for anything that ever wrote milliseconds, so the magnitude decides - the

+ 7 - 4
webui/src/pages/ExecutionsPage.tsx

@@ -1,5 +1,5 @@
 import { useProjectStore } from '../stores/projectStore'
-import { useState, useEffect, useMemo } from 'react'
+import { useState, useEffect, useMemo, useCallback } from 'react'
 import { useNavigate, useSearchParams } from 'react-router-dom'
 import { useQuery, useMutation, useQueryClient, keepPreviousData } from '@tanstack/react-query'
 import { executionsApi, workflowsApi, nodesApi, type ExecutionListItem } from '../api/workflows'
@@ -110,7 +110,10 @@ export default function ExecutionsPage() {
   // Built from the previous params rather than from the ones this render closed
   // over: the search box writes on a timer, and a filter changed while that
   // timer was pending would otherwise be undone when it fired.
-  const updateParams = (changes: Record<string, string | null>, keepPage = false) => {
+  // useCallback because the debounced search effect below depends on it. It
+  // closes over nothing but setSearchParams, which react-router keeps stable,
+  // so this identity never changes and listing it cannot restart the timer.
+  const updateParams = useCallback((changes: Record<string, string | null>, keepPage = false) => {
     setSearchParams(previous => {
       const next = new URLSearchParams(previous)
       for (const [key, value] of Object.entries(changes)) {
@@ -127,7 +130,7 @@ export default function ExecutionsPage() {
       }
       return next
     }, { replace: true })
-  }
+  }, [setSearchParams])
 
   // The search box is typed into, so it holds its own value and pushes it to the
   // URL once typing stops. Writing every keystroke to the URL would fire a query
@@ -140,7 +143,7 @@ export default function ExecutionsPage() {
     if (searchInput === search) return
     const timer = setTimeout(() => updateParams({ q: searchInput }), 300)
     return () => clearTimeout(timer)
-  }, [searchInput])
+  }, [searchInput, search, updateParams])
 
   // The range presets are relative to now, but the boundary is fixed when the
   // range is chosen and not recomputed per render or per page. A sliding

+ 4 - 1
webui/src/pages/NodesPage.tsx

@@ -126,7 +126,10 @@ export default function NodesPage() {
     }
   }
 
-  const nodes: NodeDefinition[] = nodesData?.nodes || []
+  // Memoised because `|| []` builds a new array on every render, which made
+  // every useMemo keyed on it recompute every time and defeated the point of
+  // memoising them at all.
+  const nodes: NodeDefinition[] = useMemo(() => nodesData?.nodes || [], [nodesData])
 
   // Every category, from the full list - so the filter chips do not vanish as
   // you narrow the results and leave no way back.

+ 25 - 5
webui/src/pages/WorkflowEditorPage.tsx

@@ -814,7 +814,7 @@ function WorkflowEditorInner() {
         if (!cancelled) { setIncomingChanges(null); setMyChanges(null) }
       })
     return () => { cancelled = true }
-  }, [id, savedByOther, saveConflict])
+  }, [id, savedByOther, saveConflict, queryClient])
 
   const { data: nodeDefinitions } = useQuery({
     queryKey: ['nodes'],
@@ -1642,7 +1642,9 @@ function WorkflowEditorInner() {
     }
   }, [])
 
-  const nodeDefs: NodeDefinition[] = nodeDefinitions?.nodes || []
+  // Memoised: `|| []` is a fresh array each render, so nodeDefsMap below was
+  // rebuilt on every render despite being a useMemo.
+  const nodeDefs: NodeDefinition[] = useMemo(() => nodeDefinitions?.nodes || [], [nodeDefinitions])
   const nodeDefsMap = useMemo(() => {
     const map: Record<string, NodeDefinition> = {}
     nodeDefs.forEach((nd) => { map[nd.id] = nd })
@@ -1715,7 +1717,7 @@ function WorkflowEditorInner() {
     // Their change is now an unsaved change here too, and saying otherwise
     // would let it be lost on the way out.
     setHasChanges(true)
-  }, [setNodes, setEdges, nodeDefsMap, executionState])
+  }, [setNodes, setEdges, nodeDefsMap, executionState, executeWorkflow, executeTrigger])
 
   const {
     lockedByOther, livePositions, claim: claimNode, release: releaseNode,
@@ -1836,7 +1838,14 @@ function WorkflowEditorInner() {
         settings: workflow.settings || {},
       }
     }
-  }, [workflow, nodeDefs, nodeDefsMap, setNodes, setEdges])
+  // executionState is deliberately not a dependency. This effect rebuilds the
+  // whole canvas from the stored workflow, and execution state changes on every
+  // node event of a running workflow - so listing it would reload the graph
+  // underneath a run, several times a second, discarding live node state as it
+  // arrived. The nodes read it through the props they are built with instead.
+  // executeWorkflow and executeTrigger are listed because they are stable.
+  // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [workflow, nodeDefs, nodeDefsMap, setNodes, setEdges, executeWorkflow, executeTrigger])
 
   // Send what changed here to everybody else watching.
   //
@@ -2179,7 +2188,7 @@ function WorkflowEditorInner() {
         onExecuteTrigger: executeTrigger,
       },
     }
-  }, [executeTrigger])
+  }, [executeTrigger, executeWorkflow])
 
   // The middle of what the user is currently looking at. A node dropped at a
   // fixed spot lands off screen as soon as anyone has panned away, which is
@@ -2387,6 +2396,12 @@ function WorkflowEditorInner() {
 
     window.addEventListener('keydown', handleKeyDown)
     return () => window.removeEventListener('keydown', handleKeyDown)
+  // navigateToConnectedNode is declared below this effect, so naming it here
+  // would read it before initialisation and throw on every render. It is only
+  // reached from the handler, and the value that would go stale in it -
+  // focusedNodeId - is already listed, so the effect re-registers with a fresh
+  // copy whenever it could matter.
+  // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [hasChanges, saveMutation.isPending, handleSave, focusedNodeId, selectedEdgeId, showNodeConfig, showNodePicker, showDeleteConfirm, deleteEdge, openNodeConfig, nodes, edges, setNodes, setEdges, copySelection, pasteNodes, isViewingExecution, history, showToast])
 
   // Navigate to connected node based on arrow direction
@@ -2508,6 +2523,11 @@ function WorkflowEditorInner() {
     const nodeDef = nodeDefsMap[nodeType]
     if (!nodeDef) return
     addNode(nodeType, nodeDef, reactFlowInstance.screenToFlowPosition({ x: event.clientX, y: event.clientY }))
+  // addNode is rebuilt every render, so listing it would rebuild this every
+  // render too. A captured copy is not stale: every write it makes goes through
+  // the functional form of setNodes/setEdges, and connectDrop - the one value
+  // it reads directly - is already listed here.
+  // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [isViewingExecution, nodeDefsMap, reactFlowInstance, connectDrop])
 
   // Delete node

+ 3 - 1
webui/src/pages/WorkflowsPage.tsx

@@ -112,7 +112,9 @@ export default function WorkflowsPage() {
     queryKey: ['workflows', currentGroupId || '', projectId],
     queryFn: () => workflowsApi.list(1, 100, currentGroupId || '', projectId || undefined),
   })
-  const workflows: Workflow[] = workflowsData?.workflows || []
+  // Memoised: `|| []` is a fresh array each render, so anything keyed on it
+  // recomputed every time.
+  const workflows: Workflow[] = useMemo(() => workflowsData?.workflows || [], [workflowsData])
 
   // Node definitions, purely to learn which node types are triggers. A workflow
   // stores only a node's type string, so the definitions are the only place