Merge pull request #54 from jhbruhn/refactor/36-improve-useeffect-with-useprevious

refactor: Improve useEffect patterns in AppHeader with usePrevious hook

src/components/AppHeader.tsx

@@ -1,7 +1,8 @@
-import { useState, useEffect, useRef } from "react";
+import { useState, useEffect } from "react";
 import { useShallow } from "zustand/react/shallow";
 import { useMachineStore } from "../stores/useMachineStore";
 import { useUIStore } from "../stores/useUIStore";
+import { usePrevious } from "../hooks/usePrevious";
 import { WorkflowStepper } from "./WorkflowStepper";
 import { ErrorPopoverContent } from "./ErrorPopover";
 import {
@@ -65,9 +66,12 @@ export function AppHeader() {
   const [dismissedErrorCode, setDismissedErrorCode] = useState<number | null>(
     null,
   );
-  const prevMachineErrorRef = useRef<number | undefined>(undefined);
-  const prevErrorMessageRef = useRef<string | null>(null);
-  const prevPyodideErrorRef = useRef<string | null>(null);
+  const [wasManuallyDismissed, setWasManuallyDismissed] = useState(false);
+
+  // Track previous values for comparison
+  const prevMachineError = usePrevious(machineError);
+  const prevErrorMessage = usePrevious(machineErrorMessage);
+  const prevPyodideError = usePrevious(pyodideError);
 
   // Get state visual info for header status badge
   const stateVisual = getStateVisualInfo(machineStatus);
@@ -89,31 +93,26 @@ export function AppHeader() {
   // Auto-open/close error popover based on error state changes
   /* eslint-disable react-hooks/set-state-in-effect */
   useEffect(() => {
-    const currentError = machineError;
-    const prevError = prevMachineErrorRef.current;
-    const currentErrorMessage = machineErrorMessage;
-    const prevErrorMessage = prevErrorMessageRef.current;
-    const currentPyodideError = pyodideError;
-    const prevPyodideError = prevPyodideErrorRef.current;
-
     // Check if there's any error now
     const hasAnyError =
-      machineErrorMessage || pyodideError || hasError(currentError);
+      machineErrorMessage || pyodideError || hasError(machineError);
     // Check if there was any error before
     const hadAnyError =
-      prevErrorMessage || prevPyodideError || hasError(prevError);
+      prevErrorMessage || prevPyodideError || hasError(prevMachineError);
 
-    // Auto-open popover when new error appears
+    // Auto-open popover when new error appears (but not if user manually dismissed)
     const isNewMachineError =
-      hasError(currentError) &&
-      currentError !== prevError &&
-      currentError !== dismissedErrorCode;
+      hasError(machineError) &&
+      machineError !== prevMachineError &&
+      machineError !== dismissedErrorCode;
     const isNewErrorMessage =
-      currentErrorMessage && currentErrorMessage !== prevErrorMessage;
-    const isNewPyodideError =
-      currentPyodideError && currentPyodideError !== prevPyodideError;
+      machineErrorMessage && machineErrorMessage !== prevErrorMessage;
+    const isNewPyodideError = pyodideError && pyodideError !== prevPyodideError;
 
-    if (isNewMachineError || isNewErrorMessage || isNewPyodideError) {
+    if (
+      !wasManuallyDismissed &&
+      (isNewMachineError || isNewErrorMessage || isNewPyodideError)
+    ) {
       setErrorPopoverOpen(true);
     }
 
@@ -121,28 +120,34 @@ export function AppHeader() {
     if (!hasAnyError && hadAnyError) {
       setErrorPopoverOpen(false);
       setDismissedErrorCode(null); // Reset dismissed tracking
+      setWasManuallyDismissed(false); // Reset manual dismissal flag
     }
-
-    // Update refs for next comparison
-    prevMachineErrorRef.current = currentError;
-    prevErrorMessageRef.current = currentErrorMessage;
-    prevPyodideErrorRef.current = currentPyodideError;
-  }, [machineError, machineErrorMessage, pyodideError, dismissedErrorCode]);
+  }, [
+    machineError,
+    machineErrorMessage,
+    pyodideError,
+    dismissedErrorCode,
+    wasManuallyDismissed,
+    prevMachineError,
+    prevErrorMessage,
+    prevPyodideError,
+  ]);
   /* eslint-enable react-hooks/set-state-in-effect */
 
   // Handle manual popover dismiss
   const handlePopoverOpenChange = (open: boolean) => {
     setErrorPopoverOpen(open);
 
-    // If user manually closes it, remember the current error state to prevent reopening
-    if (!open) {
-      // For machine errors, track the error code
+    // If user manually closes it while any error is present, remember this to prevent reopening
+    if (
+      !open &&
+      (hasError(machineError) || machineErrorMessage || pyodideError)
+    ) {
+      setWasManuallyDismissed(true);
+      // Also track the specific machine error code if present
       if (hasError(machineError)) {
         setDismissedErrorCode(machineError);
       }
-      // Update refs so we don't reopen for the same error message/pyodide error
-      prevErrorMessageRef.current = machineErrorMessage;
-      prevPyodideErrorRef.current = pyodideError;
     }
   };
 

src/hooks/usePrevious.ts

@@ -0,0 +1,26 @@
+/**
+ * usePrevious Hook
+ *
+ * Returns the previous value of a state or prop
+ * Useful for comparing current vs previous values in effects
+ *
+ * This implementation updates the ref in an effect instead of during render,
+ * which is compatible with React Concurrent Mode and Strict Mode.
+ *
+ * Note: The ref read on return is safe because it's only returning the previous value
+ * (from the last render), not the current value being passed in. This is the standard
+ * pattern for usePrevious hooks.
+ */
+
+import { useEffect, useRef } from "react";
+
+export function usePrevious<T>(value: T): T | undefined {
+  const ref = useRef<T | undefined>(undefined);
+
+  useEffect(() => {
+    ref.current = value;
+  }, [value]);
+
+  // eslint-disable-next-line react-hooks/refs
+  return ref.current;
+}