From 7baf03e4f71689298458dd31bbe62e5003c9c02f Mon Sep 17 00:00:00 2001 From: Jan-Henrik Bruhn Date: Sat, 27 Dec 2025 11:46:45 +0100 Subject: [PATCH 1/3] refactor: Improve useEffect patterns in AppHeader with usePrevious hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace manual ref tracking with usePrevious custom hook for cleaner, more maintainable code. Simplifies error state change detection by eliminating manual ref updates and reducing complexity. Resolves #36 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- src/components/AppHeader.tsx | 60 ++++++++++++++++-------------------- src/hooks/usePrevious.ts | 31 +++++++++++++++++++ 2 files changed, 57 insertions(+), 34 deletions(-) create mode 100644 src/hooks/usePrevious.ts diff --git a/src/components/AppHeader.tsx b/src/components/AppHeader.tsx index 0ec0976..20d9de2 100644 --- a/src/components/AppHeader.tsx +++ b/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,11 @@ export function AppHeader() { const [dismissedErrorCode, setDismissedErrorCode] = useState( null, ); - const prevMachineErrorRef = useRef(undefined); - const prevErrorMessageRef = useRef(null); - const prevPyodideErrorRef = useRef(null); + + // 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,29 +92,21 @@ 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 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) { setErrorPopoverOpen(true); @@ -122,27 +117,24 @@ export function AppHeader() { setErrorPopoverOpen(false); setDismissedErrorCode(null); // Reset dismissed tracking } - - // Update refs for next comparison - prevMachineErrorRef.current = currentError; - prevErrorMessageRef.current = currentErrorMessage; - prevPyodideErrorRef.current = currentPyodideError; - }, [machineError, machineErrorMessage, pyodideError, dismissedErrorCode]); + }, [ + machineError, + machineErrorMessage, + pyodideError, + dismissedErrorCode, + 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 (hasError(machineError)) { - setDismissedErrorCode(machineError); - } - // Update refs so we don't reopen for the same error message/pyodide error - prevErrorMessageRef.current = machineErrorMessage; - prevPyodideErrorRef.current = pyodideError; + // If user manually closes it, remember the current error code to prevent reopening + if (!open && hasError(machineError)) { + setDismissedErrorCode(machineError); } }; diff --git a/src/hooks/usePrevious.ts b/src/hooks/usePrevious.ts new file mode 100644 index 0000000..d959bb3 --- /dev/null +++ b/src/hooks/usePrevious.ts @@ -0,0 +1,31 @@ +/** + * usePrevious Hook + * + * Returns the previous value of a state or prop + * Useful for comparing current vs previous values in effects + * + * Note: This uses ref access during render, which is safe for this specific use case. + * The pattern is recommended by React for tracking previous values. + */ + +import { useRef } from "react"; + +/* eslint-disable react-hooks/refs */ +export function usePrevious(value: T): T | undefined { + const ref = useRef<{ value: T; prev: T | undefined }>({ + value, + prev: undefined, + }); + + const current = ref.current.value; + + if (value !== current) { + ref.current = { + value, + prev: current, + }; + } + + return ref.current.prev; +} +/* eslint-enable react-hooks/refs */ From 757e0cdd73102ce777cf51dc577f68988946f86b Mon Sep 17 00:00:00 2001 From: Jan-Henrik Bruhn Date: Sat, 27 Dec 2025 11:55:32 +0100 Subject: [PATCH 2/3] fix: Address PR review feedback for usePrevious implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update usePrevious hook to use useEffect pattern instead of mutating refs during render (addresses Concurrent Mode compatibility) - Add wasManuallyDismissed flag to properly track dismissal of all error types (machineError, machineErrorMessage, and pyodideError) - Add proper eslint-disable comment with explanation for ref access - Update handlePopoverOpenChange to handle dismissal of all error types These changes address all feedback from PR review #54 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- src/components/AppHeader.tsx | 23 ++++++++++++++++++----- src/hooks/usePrevious.ts | 31 +++++++++++++------------------ 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/src/components/AppHeader.tsx b/src/components/AppHeader.tsx index 20d9de2..c100d06 100644 --- a/src/components/AppHeader.tsx +++ b/src/components/AppHeader.tsx @@ -66,6 +66,7 @@ export function AppHeader() { const [dismissedErrorCode, setDismissedErrorCode] = useState( null, ); + const [wasManuallyDismissed, setWasManuallyDismissed] = useState(false); // Track previous values for comparison const prevMachineError = usePrevious(machineError); @@ -99,7 +100,7 @@ export function AppHeader() { const hadAnyError = 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(machineError) && machineError !== prevMachineError && @@ -108,7 +109,10 @@ export function AppHeader() { machineErrorMessage && machineErrorMessage !== prevErrorMessage; const isNewPyodideError = pyodideError && pyodideError !== prevPyodideError; - if (isNewMachineError || isNewErrorMessage || isNewPyodideError) { + if ( + !wasManuallyDismissed && + (isNewMachineError || isNewErrorMessage || isNewPyodideError) + ) { setErrorPopoverOpen(true); } @@ -116,12 +120,14 @@ export function AppHeader() { if (!hasAnyError && hadAnyError) { setErrorPopoverOpen(false); setDismissedErrorCode(null); // Reset dismissed tracking + setWasManuallyDismissed(false); // Reset manual dismissal flag } }, [ machineError, machineErrorMessage, pyodideError, dismissedErrorCode, + wasManuallyDismissed, prevMachineError, prevErrorMessage, prevPyodideError, @@ -132,9 +138,16 @@ export function AppHeader() { const handlePopoverOpenChange = (open: boolean) => { setErrorPopoverOpen(open); - // If user manually closes it, remember the current error code to prevent reopening - if (!open && hasError(machineError)) { - setDismissedErrorCode(machineError); + // 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); + } } }; diff --git a/src/hooks/usePrevious.ts b/src/hooks/usePrevious.ts index d959bb3..26b8021 100644 --- a/src/hooks/usePrevious.ts +++ b/src/hooks/usePrevious.ts @@ -4,28 +4,23 @@ * Returns the previous value of a state or prop * Useful for comparing current vs previous values in effects * - * Note: This uses ref access during render, which is safe for this specific use case. - * The pattern is recommended by React for tracking previous values. + * 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 { useRef } from "react"; +import { useEffect, useRef } from "react"; -/* eslint-disable react-hooks/refs */ export function usePrevious(value: T): T | undefined { - const ref = useRef<{ value: T; prev: T | undefined }>({ - value, - prev: undefined, - }); + const ref = useRef(); - const current = ref.current.value; + useEffect(() => { + ref.current = value; + }, [value]); - if (value !== current) { - ref.current = { - value, - prev: current, - }; - } - - return ref.current.prev; + // eslint-disable-next-line react-hooks/refs + return ref.current; } -/* eslint-enable react-hooks/refs */ From 705815a8fcdb1679a54adfe9f76e098f6b4e31d0 Mon Sep 17 00:00:00 2001 From: Jan-Henrik Bruhn Date: Sat, 27 Dec 2025 11:59:42 +0100 Subject: [PATCH 3/3] fix: Provide initial value to useRef in usePrevious hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TypeScript requires an initial value argument when calling useRef. Changed useRef() to useRef(undefined) to fix build error and properly type the ref for the first render. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- src/hooks/usePrevious.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hooks/usePrevious.ts b/src/hooks/usePrevious.ts index 26b8021..f081f95 100644 --- a/src/hooks/usePrevious.ts +++ b/src/hooks/usePrevious.ts @@ -15,7 +15,7 @@ import { useEffect, useRef } from "react"; export function usePrevious(value: T): T | undefined { - const ref = useRef(); + const ref = useRef(undefined); useEffect(() => { ref.current = value;