[FE-Feat] useEffectOnce 훅 구현하기 #428
Conversation
WalkthroughA new custom React hook, Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useEffectOnce
participant Callback
Component->>useEffectOnce: Pass { condition, callback }
useEffectOnce->>useEffectOnce: Check if condition is true and not yet called
alt First time and condition met
useEffectOnce->>Callback: Invoke callback
Callback-->>useEffectOnce: Complete
else Already called or condition false
useEffectOnce-->>Component: Do nothing
end
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/apps/client/src/routes/oauth.redirect/calendar/index.tsx (1)
12-17: Consider simplifying the async callback pattern.The current implementation works but wrapping async operations in an IIFE is unnecessarily complex.
Consider this cleaner approach:
-const clearCalendarCache = useCallback(() => { - (async () => { - await queryClient.invalidateQueries({ queryKey: calendarKeys.all }); - navigate({ to: '/my-calendar' }); - })(); -}, [queryClient, navigate]); +const clearCalendarCache = useCallback(async () => { + await queryClient.invalidateQueries({ queryKey: calendarKeys.all }); + navigate({ to: '/my-calendar' }); +}, [queryClient, navigate]);However, ensure that the
useEffectOncehook properly handles async callbacks if you make this change.frontend/packages/core/src/hooks/useEffectOnce.ts (2)
3-6: Consider improving the TypeScript interface.The current interface restricts callbacks to
voidreturn types, but many real-world use cases involve async operations.Consider this more flexible interface:
interface UseEffectOnceOptions { condition?: boolean; - callback: () => void; + callback: () => void | Promise<void>; }This would better support the async patterns used in the codebase (like in the calendar component).
8-13: Enhance JSDoc documentation.The documentation could be more comprehensive about the hook's behavior and StrictMode compatibility.
/** - * + * A hook that ensures a callback function runs only once, even in React StrictMode. + * * @param options * @param options.condition - 콜백을 실행할 조건 (기본값: true) * @param options.callback - 한 번만 실행할 콜백 함수. useCallback을 사용하여 메모이제이션하는 것이 좋습니다. + * + * @example + * ```tsx + * const memoizedCallback = useCallback(() => { + * // Your side effect here + * }, [dependencies]); + * + * useEffectOnce({ + * condition: someCondition, + * callback: memoizedCallback + * }); + * ``` */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/.gitignore(1 hunks)frontend/apps/client/src/routes/oauth.redirect/calendar/index.tsx(1 hunks)frontend/apps/client/src/routes/oauth.redirect/login/index.tsx(2 hunks)frontend/packages/core/src/hooks/index.ts(1 hunks)frontend/packages/core/src/hooks/useEffectOnce.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: frontend
🔇 Additional comments (4)
frontend/.gitignore (1)
14-14: LGTM! Standard environment file exclusion.The addition of
.env*pattern properly excludes all environment configuration files from version control, which is a security best practice.frontend/packages/core/src/hooks/index.ts (1)
2-2: LGTM! Proper hook export.The export statement correctly follows the existing pattern and makes the new hook available for consumption.
frontend/apps/client/src/routes/oauth.redirect/calendar/index.tsx (1)
19-19: LGTM! Correct usage of useEffectOnce.The hook is used appropriately with a memoized callback to ensure the effect runs only once on mount.
frontend/apps/client/src/routes/oauth.redirect/login/index.tsx (1)
14-21: LGTM! Excellent usage pattern.The implementation correctly:
- Memoizes the callback with proper dependencies
- Uses a condition to control when the effect should run
- Follows the recommended pattern for the useEffectOnce hook
| useEffect(() => { | ||
| if (condition && !isCalled.current) { | ||
| callback(); | ||
| } | ||
|
|
||
| return () => { | ||
| if (!isCalled.current) { | ||
| isCalled.current = true; | ||
| } | ||
| }; | ||
| }, [condition, callback]); |
There was a problem hiding this comment.
Fix critical logic error in the hook implementation.
The current implementation has a fundamental flaw in the cleanup function logic that breaks the hook's intended behavior.
Issues:
isCalled.currentis not set totrueimmediately after calling the callback- The cleanup function incorrectly sets the flag when the callback wasn't called
- This prevents the hook from working correctly with conditional execution
Example of the bug:
If condition is initially false, the effect runs but doesn't call the callback. The cleanup then sets isCalled.current = true, preventing future execution even when condition becomes true.
Apply this fix:
useEffect(() => {
if (condition && !isCalled.current) {
+ isCalled.current = true;
callback();
}
-
- return () => {
- if (!isCalled.current) {
- isCalled.current = true;
- }
- };
}, [condition, callback]);This ensures the callback runs only once when the condition is met, which aligns with the hook's purpose and StrictMode compatibility goals.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (condition && !isCalled.current) { | |
| callback(); | |
| } | |
| return () => { | |
| if (!isCalled.current) { | |
| isCalled.current = true; | |
| } | |
| }; | |
| }, [condition, callback]); | |
| useEffect(() => { | |
| if (condition && !isCalled.current) { | |
| isCalled.current = true; | |
| callback(); | |
| } | |
| }, [condition, callback]); |
🤖 Prompt for AI Agents
In frontend/packages/core/src/hooks/useEffectOnce.ts between lines 17 and 27,
the hook's logic incorrectly sets isCalled.current to true in the cleanup
function even if the callback was not called, causing the callback to never run
when the condition later becomes true. To fix this, set isCalled.current to true
immediately after calling the callback inside the effect, and remove or adjust
the cleanup function so it does not set isCalled.current when the callback was
not executed. This ensures the callback runs only once when the condition is met
as intended.
|
기존 StrictMode의 문제를 해결하고자 만든 훅인데, 리액트의 의도와 맞지 않는 것을 발견하여 브랜치를 변경하여 다른 방법으로 해결했습니다. |
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
StrictMode에서도 한번만 동작하는useEffectOnce훅을 구현합니다.🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
Refactor
Chores