🧹 [Code Health] Extract useModalFocus hook from CardGeneratorModal#300
🧹 [Code Health] Extract useModalFocus hook from CardGeneratorModal#300is0692vs wants to merge 2 commits into
Conversation
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors focus management in CardGeneratorModal by introducing a new useModalFocus hook and corresponding unit tests. Feedback focuses on preventing 'focus stealing' by removing the onClose dependency from the effect, addressing a race condition where the hook might run before the modal is mounted, and adding a test case to verify that focus is correctly restored to the background element upon closing.
| 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]); |
There was a problem hiding this comment.
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
- Maintain explicit return types for functions in TypeScript to ensure type safety and API clarity.
| }; | ||
| } | ||
| }, [isOpen, handleClose]); | ||
| useModalFocus(isOpen, modalRef, handleClose); |
There was a problem hiding this comment.
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.
| useModalFocus(isOpen, modalRef, handleClose); | |
| useModalFocus(isOpen && mounted, modalRef, handleClose); |
References
- Avoid using fixed setTimeout delays to prevent rendering issues and race conditions.
| handler(new KeyboardEvent("keydown", { key: "Enter" })); | ||
| expect(onClose).toHaveBeenCalledTimes(1); // Should not increase | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "src/components/LayoutEditor.tsx", | ||
| "src/hooks/useModalFocus.ts", | ||
| ], |
There was a problem hiding this comment.
src/hooks/useModalFocus.ts の追加は不要です。カバレッジの include リストにはすでに src/hooks/**/*.ts というグロブパターンが含まれており、useModalFocus.ts は自動的に対象になります。重複したエントリを追加するとメンテナンス時に混乱を招く可能性があります。
| "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.| } | ||
| }; | ||
| } | ||
| }, [isOpen, onClose, modalRef]); |
There was a problem hiding this comment.
useRef が返す ref オブジェクト自体は、コンポーネントのライフサイクル全体を通じて同一参照を維持するため、modalRef を useEffect の依存配列に含める必要はありません。依存に含めると、将来的に親コンポーネントが異なる ref を渡すケースで意図しない再実行が起きるリスクがあります。
| }, [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!
| test("adds and removes keydown event listener based on isOpen", () => { | ||
| const { unmount, rerender } = renderHook( | ||
| ({ isOpen }) => useModalFocus(isOpen, modalRef, onClose), | ||
| { initialProps: { isOpen: true } } | ||
| ); | ||
|
|
||
| expect(addEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function)); | ||
|
|
||
| unmount(); | ||
| expect(removeEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function)); | ||
| }); |
There was a problem hiding this comment.
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.Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
1 participant
🎯 What: Extracted the modal focus management and Escape key handling logic from
src/components/CardGeneratorModal.tsxinto a reusable custom hooksrc/hooks/useModalFocus.ts.💡 Why:
CardGeneratorModal.tsxwas a large file with mixed concerns (UI rendering, layout editing, card settings management, and simple DOM event bindings). By separating out the raw DOM side effects (focus management and keydown listeners) into an isolated and testable hook, the modal component becomes much cleaner and easier to read.✅ Verification:
CardGeneratorModalto use the hook correctly.npm run lintandnpm run test, and confirmed everything passed including coverage thresholds.✨ Result: Improved separation of concerns, reduced file size and complexity in
CardGeneratorModal, and better testability for the focus and keyboard handling logic.PR created automatically by Jules for task 7008740017028856622 started by @is0692vs
Greptile Summary
CardGeneratorModal.tsxからモーダルのフォーカス管理・EscapeキーハンドリングのロジックをuseModalFocusカスタムフックとして切り出したリファクタリングPRです。元のロジックをほぼそのまま移植しており、動作の変化はありません。src/hooks/useModalFocus.tsを新規作成し、フォーカス保存・復元とキーダウンリスナーの登録・解除をカプセル化。CardGeneratorModal.tsxのインラインのuseEffectをuseModalFocus(isOpen, modalRef, handleClose)の1行に置き換えることでコンポーネントの責務を明確化。useModalFocus.tsを明示的に追加（ただし既存のグロブパターンと重複）。Confidence Score: 4/5
既存ロジックの純粋な切り出しであり、振る舞いの変化はない。安全にマージ可能。
フォーカス管理ロジックの移植は正確で、元の動作と等価。
modalRefが依存配列に含まれている点とvitest.config.tsの重複エントリは小さなクリーンアップ項目に留まり、機能上の問題はない。フォーカス復元のアサーションがテストに欠けている点は動作には影響しないが、リグレッション検知力を下げる。特に注意が必要なファイルはないが、
src/hooks/useModalFocus.tsの依存配列とvitest.config.tsの重複エントリは軽微な修正で改善できる。Important Files Changed
modalRefを不要に依存配列に含めている。useModalFocusフックへの差し替えのみ。元のインラインロジックと振る舞いは等価で、問題なし。src/hooks/useModalFocus.tsを明示的に追加しているが、既存のsrc/hooks/**/*.tsグロブで既にカバーされているため重複。Sequence Diagram
sequenceDiagram participant User participant CardGeneratorModal participant useModalFocus participant document User->>CardGeneratorModal: "モーダルを開く (isOpen=true)" CardGeneratorModal->>useModalFocus: useModalFocus(true, modalRef, handleClose) useModalFocus->>document: activeElement を保存 (previousFocusRef) useModalFocus->>CardGeneratorModal: modalRef.current.focus() useModalFocus->>document: addEventListener(keydown, handleKeyDown) User->>document: Escapeキー押下 document->>useModalFocus: handleKeyDown イベント useModalFocus->>CardGeneratorModal: onClose() 呼び出し CardGeneratorModal->>useModalFocus: useModalFocus(false, modalRef, handleClose) Note over useModalFocus: クリーンアップ実行 useModalFocus->>document: removeEventListener(keydown, handleKeyDown) useModalFocus->>User: previousFocusRef.current.focus() でフォーカス復元Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "🧹 Extract useModalFocus hook from CardG..." | Re-trigger Greptile