Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 2 additions & 26 deletions src/components/CardGeneratorModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SettingsTab } from "./SettingsTab";
import { ActionButtons } from "./ActionButtons";
import { useCardSettings } from "@/hooks/useCardSettings";
import { useCardPreview } from "@/hooks/useCardPreview";
import { useModalFocus } from "@/hooks/useModalFocus";

interface CardGeneratorModalProps {
isOpen: boolean;
Expand All @@ -27,8 +28,6 @@ export default function CardGeneratorModal({

const cardRef = useRef<HTMLDivElement>(null);
const modalRef = useRef<HTMLDivElement>(null);
const previousFocusRef = useRef<HTMLElement | null>(null);

useEffect(() => {
const timer = setTimeout(() => setMounted(true), 0);
return () => clearTimeout(timer);
Expand Down Expand Up @@ -60,30 +59,7 @@ export default function CardGeneratorModal({
setPreviewSize(null);
}, [onClose, setPreviewSize, setPreviewUrl]);

useEffect(() => {
if (isOpen) {
previousFocusRef.current = document.activeElement as HTMLElement;

if (modalRef.current) {
modalRef.current.focus();
}

const handleKeyDown = (e: globalThis.KeyboardEvent) => {
if (e.key === "Escape") {
handleClose();
}
};

document.addEventListener("keydown", handleKeyDown);

return () => {
document.removeEventListener("keydown", handleKeyDown);
if (previousFocusRef.current) {
previousFocusRef.current.focus();
}
};
}
}, [isOpen, handleClose]);
useModalFocus(isOpen, modalRef, handleClose);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The modal is only rendered when mounted is true (line 64), but mounted is set asynchronously via setTimeout (line 32). If isOpen becomes true while mounted is still false, the useModalFocus hook will run its effect, but modalRef.current will be null, so the modal container will not be focused. Since mounted is not a dependency of the hook's effect, it won't re-run when the modal actually renders. You should pass isOpen && mounted to the hook to ensure it only attempts to manage focus when the element is actually in the DOM. Avoid using fixed setTimeout delays for rendering logic.

Suggested change
useModalFocus(isOpen, modalRef, handleClose);
useModalFocus(isOpen && mounted, modalRef, handleClose);
References
  1. Avoid using fixed setTimeout delays to prevent rendering issues and race conditions.


if (!isOpen || !mounted) return null;

Expand Down
60 changes: 60 additions & 0 deletions src/hooks/__tests__/useModalFocus.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { renderHook } from "@testing-library/react";
import { useModalFocus } from "../useModalFocus";
import { expect, test, vi, describe, afterEach, beforeEach } from "vitest";

describe("useModalFocus", () => {
let modalRef: React.RefObject<HTMLElement | null>;
let onClose: ReturnType<typeof vi.fn>;
let addEventListenerSpy: ReturnType<typeof vi.spyOn>;
let removeEventListenerSpy: ReturnType<typeof vi.spyOn>;

beforeEach(() => {
modalRef = { current: document.createElement("div") };
onClose = vi.fn();
addEventListenerSpy = vi.spyOn(document, "addEventListener");
removeEventListenerSpy = vi.spyOn(document, "removeEventListener");
});

afterEach(() => {
vi.restoreAllMocks();
});

test("focuses modalRef when isOpen is true", () => {
const focusSpy = vi.spyOn(modalRef.current!, "focus");
renderHook(() => useModalFocus(true, modalRef, onClose as unknown as () => void));
expect(focusSpy).toHaveBeenCalledTimes(1);
});

test("does not focus modalRef when isOpen is false", () => {
const focusSpy = vi.spyOn(modalRef.current!, "focus");
renderHook(() => useModalFocus(false, modalRef, onClose as unknown as () => void));
expect(focusSpy).not.toHaveBeenCalled();
});

test("adds and removes keydown event listener based on isOpen", () => {
const { unmount } = renderHook(
({ isOpen }) => useModalFocus(isOpen, modalRef, onClose as unknown as () => void),
{ initialProps: { isOpen: true } }
);

expect(addEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function));

unmount();
expect(removeEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function));
});
Comment on lines +34 to +44
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 クリーンアップ時に「直前にフォーカスされていた要素へフォーカスを戻す」動作が明示的にアサートされていません。document.activeElement をモックしてクリーンアップ後に previousElement.focus() が呼ばれることを検証するテストケースを追加すると、フォーカス管理のリグレッションを防ぎやすくなります。

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/__tests__/useModalFocus.test.ts
Line: 34-44

Comment:
クリーンアップ時に「直前にフォーカスされていた要素へフォーカスを戻す」動作が明示的にアサートされていません。`document.activeElement` をモックしてクリーンアップ後に `previousElement.focus()` が呼ばれることを検証するテストケースを追加すると、フォーカス管理のリグレッションを防ぎやすくなります。

How can I resolve this? If you propose a fix, please make it concise.


test("calls onClose when Escape key is pressed", () => {
renderHook(() => useModalFocus(true, modalRef, onClose as unknown as () => void));

const eventListenerCall = addEventListenerSpy.mock.calls.find(
(call: unknown[]) => call[0] === "keydown"
);
const handler = eventListenerCall![1] as EventListener;

handler(new KeyboardEvent("keydown", { key: "Escape" }));
expect(onClose).toHaveBeenCalledTimes(1);

handler(new KeyboardEvent("keydown", { key: "Enter" }));
expect(onClose).toHaveBeenCalledTimes(1); // Should not increase
});
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test suite is missing a case to verify that focus is correctly restored to the previously active element when the modal is closed or the hook is unmounted. This is a core functionality of the hook and should be explicitly tested.

34 changes: 34 additions & 0 deletions src/hooks/useModalFocus.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { useEffect, useRef } from "react";

export function useModalFocus(
isOpen: boolean,
modalRef: React.RefObject<HTMLElement | null>,
onClose: () => void
) {
const previousFocusRef = useRef<HTMLElement | null>(null);

useEffect(() => {
if (isOpen) {
previousFocusRef.current = document.activeElement as HTMLElement;

if (modalRef.current) {
modalRef.current.focus();
}

const handleKeyDown = (e: globalThis.KeyboardEvent) => {
if (e.key === "Escape") {
onClose();
}
};

document.addEventListener("keydown", handleKeyDown);

return () => {
document.removeEventListener("keydown", handleKeyDown);
if (previousFocusRef.current) {
previousFocusRef.current.focus();
}
};
}
}, [isOpen, onClose, modalRef]);
Comment on lines +8 to +33
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The useEffect currently depends on onClose. If the onClose callback provided by the caller is not memoized (e.g., an anonymous function passed directly in props), this effect will re-run on every render of the parent component. This causes the cleanup function to restore focus to the background element and the effect body to immediately focus the modal container again. This "focus stealing" resets the user's focus within the modal (e.g., if they were typing in an input field), which is a significant UX issue for a reusable hook.

To fix this, use a useRef to store the onClose callback so it can be accessed inside the effect without being a dependency. Additionally, ensure all functions have explicit return types to maintain type safety.

  const previousFocusRef = useRef<HTMLElement | null>(null);
  const onCloseRef = useRef(onClose);

  useEffect((): void => {
    onCloseRef.current = onClose;
  }, [onClose]);

  useEffect((): (() => void) | void => {
    if (isOpen) {
      previousFocusRef.current = document.activeElement as HTMLElement;

      if (modalRef.current) {
        modalRef.current.focus();
      }

      const handleKeyDown = (e: globalThis.KeyboardEvent): void => {
        if (e.key === "Escape") {
          onCloseRef.current();
        }
      };

      document.addEventListener("keydown", handleKeyDown);

      return (): void => {
        document.removeEventListener("keydown", handleKeyDown);
        if (previousFocusRef.current) {
          previousFocusRef.current.focus();
        }
      };
    }
  }, [isOpen, modalRef]);
References
  1. Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 React の useRef が返す ref オブジェクト自体は、コンポーネントのライフサイクル全体を通じて同一参照を維持するため、modalRefuseEffect の依存配列に含める必要はありません。依存に含めると、将来的に親コンポーネントが異なる ref を渡すケースで意図しない再実行が起きるリスクがあります。

Suggested change
}, [isOpen, onClose, modalRef]);
}, [isOpen, onClose]);
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/useModalFocus.ts
Line: 33

Comment:
React の `useRef` が返す ref オブジェクト自体は、コンポーネントのライフサイクル全体を通じて同一参照を維持するため、`modalRef``useEffect` の依存配列に含める必要はありません。依存に含めると、将来的に親コンポーネントが異なる ref を渡すケースで意図しない再実行が起きるリスクがあります。

```suggestion
  }, [isOpen, onClose]);
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}
1 change: 1 addition & 0 deletions vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default defineConfig({
"src/components/LanguageChart.tsx",
"src/components/SkillsCard.tsx",
"src/components/LayoutEditor.tsx",
"src/hooks/useModalFocus.ts",
],
Comment on lines 23 to 25
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 src/hooks/useModalFocus.ts の追加は不要です。カバレッジの include リストにはすでに src/hooks/**/*.ts というグロブパターンが含まれており、useModalFocus.ts は自動的に対象になります。重複したエントリを追加するとメンテナンス時に混乱を招く可能性があります。

Suggested change
"src/components/LayoutEditor.tsx",
"src/hooks/useModalFocus.ts",
],
"src/components/LayoutEditor.tsx",
],
Prompt To Fix With AI
This is a comment left during a code review.
Path: vitest.config.ts
Line: 23-25

Comment:
`src/hooks/useModalFocus.ts` の追加は不要です。カバレッジの `include` リストにはすでに `src/hooks/**/*.ts` というグロブパターンが含まれており、`useModalFocus.ts` は自動的に対象になります。重複したエントリを追加するとメンテナンス時に混乱を招く可能性があります。

```suggestion
        "src/components/LayoutEditor.tsx",
      ],
```

How can I resolve this? If you propose a fix, please make it concise.

thresholds: {
lines: 80,
Expand Down
Loading