fix: Address PR review feedback for usePrevious implementation

- 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 <noreply@anthropic.com>
This commit is contained in:
Jan-Henrik Bruhn 2025-12-27 11:55:32 +01:00
parent 7baf03e4f7
commit 757e0cdd73
2 changed files with 31 additions and 23 deletions

View file

@ -66,6 +66,7 @@ export function AppHeader() {
const [dismissedErrorCode, setDismissedErrorCode] = useState<number | null>( const [dismissedErrorCode, setDismissedErrorCode] = useState<number | null>(
null, null,
); );
const [wasManuallyDismissed, setWasManuallyDismissed] = useState(false);
// Track previous values for comparison // Track previous values for comparison
const prevMachineError = usePrevious(machineError); const prevMachineError = usePrevious(machineError);
@ -99,7 +100,7 @@ export function AppHeader() {
const hadAnyError = const hadAnyError =
prevErrorMessage || prevPyodideError || hasError(prevMachineError); 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 = const isNewMachineError =
hasError(machineError) && hasError(machineError) &&
machineError !== prevMachineError && machineError !== prevMachineError &&
@ -108,7 +109,10 @@ export function AppHeader() {
machineErrorMessage && machineErrorMessage !== prevErrorMessage; machineErrorMessage && machineErrorMessage !== prevErrorMessage;
const isNewPyodideError = pyodideError && pyodideError !== prevPyodideError; const isNewPyodideError = pyodideError && pyodideError !== prevPyodideError;
if (isNewMachineError || isNewErrorMessage || isNewPyodideError) { if (
!wasManuallyDismissed &&
(isNewMachineError || isNewErrorMessage || isNewPyodideError)
) {
setErrorPopoverOpen(true); setErrorPopoverOpen(true);
} }
@ -116,12 +120,14 @@ export function AppHeader() {
if (!hasAnyError && hadAnyError) { if (!hasAnyError && hadAnyError) {
setErrorPopoverOpen(false); setErrorPopoverOpen(false);
setDismissedErrorCode(null); // Reset dismissed tracking setDismissedErrorCode(null); // Reset dismissed tracking
setWasManuallyDismissed(false); // Reset manual dismissal flag
} }
}, [ }, [
machineError, machineError,
machineErrorMessage, machineErrorMessage,
pyodideError, pyodideError,
dismissedErrorCode, dismissedErrorCode,
wasManuallyDismissed,
prevMachineError, prevMachineError,
prevErrorMessage, prevErrorMessage,
prevPyodideError, prevPyodideError,
@ -132,9 +138,16 @@ export function AppHeader() {
const handlePopoverOpenChange = (open: boolean) => { const handlePopoverOpenChange = (open: boolean) => {
setErrorPopoverOpen(open); setErrorPopoverOpen(open);
// If user manually closes it, remember the current error code to prevent reopening // If user manually closes it while any error is present, remember this to prevent reopening
if (!open && hasError(machineError)) { if (
setDismissedErrorCode(machineError); !open &&
(hasError(machineError) || machineErrorMessage || pyodideError)
) {
setWasManuallyDismissed(true);
// Also track the specific machine error code if present
if (hasError(machineError)) {
setDismissedErrorCode(machineError);
}
} }
}; };

View file

@ -4,28 +4,23 @@
* Returns the previous value of a state or prop * Returns the previous value of a state or prop
* Useful for comparing current vs previous values in effects * Useful for comparing current vs previous values in effects
* *
* Note: This uses ref access during render, which is safe for this specific use case. * This implementation updates the ref in an effect instead of during render,
* The pattern is recommended by React for tracking previous values. * 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<T>(value: T): T | undefined { export function usePrevious<T>(value: T): T | undefined {
const ref = useRef<{ value: T; prev: T | undefined }>({ const ref = useRef<T>();
value,
prev: undefined,
});
const current = ref.current.value; useEffect(() => {
ref.current = value;
}, [value]);
if (value !== current) { // eslint-disable-next-line react-hooks/refs
ref.current = { return ref.current;
value,
prev: current,
};
}
return ref.current.prev;
} }
/* eslint-enable react-hooks/refs */