[refactor] isPending을 이용한 연속 클릭 방지 추가#157
Merged
Conversation
Closed
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (1)
Walkthrough컴포넌트 및 훅의 로딩 상태(isPending) 노출, disabled 상태 처리 체계화, 그리고 UI 피드백 개선을 통해 비동기 작업 중 중복 제출을 방지하고 사용자 경험을 개선하는 리팩토링입니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/common/ToggleSwitch.tsx (1)
46-57: 접근성 개선 제안:aria-disabled추가를 고려해주세요.
disabled속성만으로도 동작하지만, 스크린 리더 사용자를 위해aria-disabled를 명시적으로 추가하면 더 좋습니다.♻️ 제안
<button type="button" aria-label={label} aria-pressed={enabled} + aria-disabled={disabled} disabled={disabled} onClick={() => onChange(!enabled)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/ToggleSwitch.tsx` around lines 46 - 57, The ToggleSwitch button currently relies on the native disabled prop but doesn't expose the explicit accessibility state; update the ToggleSwitch component's button to include aria-disabled={disabled} (alongside existing aria-pressed={enabled}) and ensure the onClick handler respects disabled (e.g., early return if disabled) so the props onChange and aria attributes remain consistent; locate the button element in ToggleSwitch.tsx and add aria-disabled={disabled} and a disabled-guard around the onClick (referenced symbols: ToggleSwitch, enabled, disabled, onChange, aria-pressed).src/components/layout/Header/components/ChatHeader.tsx (1)
57-70: 접근성 개선: 알림 토글 버튼에aria-label추가를 권장합니다.
void toggleMute()패턴으로 unhandled promise 경고를 방지한 점은 좋습니다. 다만 스크린 리더 사용자를 위해 토글 버튼에 접근성 속성을 추가해주세요.♻️ 제안
<button type="button" disabled={isTogglingMute} onClick={() => void toggleMute()} + aria-label={isMuted ? '알림 켜기' : '알림 끄기'} + aria-pressed={!isMuted} className={`relative inline-flex h-6 w-11 items-center rounded-full transition-colors ${ isMuted ? 'bg-gray-300' : 'bg-primary' } disabled:cursor-not-allowed disabled:opacity-60`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/Header/components/ChatHeader.tsx` around lines 57 - 70, The toggle button in ChatHeader.tsx lacks accessibility attributes; update the button rendered inside the ChatHeader component (the element using isMuted, isTogglingMute, and calling toggleMute()) to include an appropriate accessible name and state — e.g., add a dynamic aria-label that changes based on isMuted (like "Unmute notifications" when isMuted is true and "Mute notifications" when false) and/or add aria-pressed={isMuted} so screen readers know the current state; keep the existing disabled handling and onClick (void toggleMute()) but ensure the aria attributes update inline with isMuted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/Chat/hooks/useChat.ts`:
- Around line 20-23: The createChatRoomMutation (mutationFn: postChatRooms)
lacks onSuccess/onError handlers and should mirror other mutations: add an
onSuccess that invalidates the chatRoomList cache (use your query
client/invalidation util used elsewhere) and add an onError to surface errors
(e.g., show toast or propagate). Also update ClubIntro.tsx's handleInquireClick
to wrap the createChatRoom call in try/catch and handle failures (display error
feedback) or await the mutation's promise and use its error handler so request
failures don't silently fail.
In `@src/pages/Club/Application/clubFeePage.tsx`:
- Around line 50-54: Extract the submission orchestration from the page into a
custom hook (e.g., create useClubApplication) so handleSubmit only calls the
hook-provided submit handler; specifically move the logic that checks imageFile,
calls uploadImage(imageFile) and then applyToClub({ answers, feePaymentImageUrl:
fileUrl }) into the hook, accept inputs (answers and imageFile or provide
setters) and expose a submit function, loading/error state and any setters
needed; update the page to import useClubApplication and replace the inline
handleSubmit with the hook's submit handler to keep business logic out of the
route component.
- Around line 50-54: Add a reentrancy guard at the start of handleSubmit to
prevent duplicate submissions: introduce a local or component-level boolean
(e.g., isSubmitting) that handleSubmit checks immediately and returns if true,
then set isSubmitting = true before calling uploadImage/applyToClub and reset it
in a finally block (isSubmitting = false) so failures still clear the guard;
reference handleSubmit, imageFile, uploadImage, and applyToClub to locate where
to add the early-return check and the try/finally around the async calls.
In `@src/pages/Club/ClubDetail/components/ClubIntro.tsx`:
- Around line 18-21: handleInquireClick currently calls
createChatRoom(clubDetail.presidentUserId) without error handling so any failure
from mutateAsync will bubble up; wrap the call in a try/catch inside
handleInquireClick, await createChatRoom(...) in the try,
navigate(`/chats/${response.chatRoomId}`) on success, and handle failures in the
catch by logging the error (or showing a user-facing message/toast) and avoiding
an unhandled rejection.
---
Nitpick comments:
In `@src/components/common/ToggleSwitch.tsx`:
- Around line 46-57: The ToggleSwitch button currently relies on the native
disabled prop but doesn't expose the explicit accessibility state; update the
ToggleSwitch component's button to include aria-disabled={disabled} (alongside
existing aria-pressed={enabled}) and ensure the onClick handler respects
disabled (e.g., early return if disabled) so the props onChange and aria
attributes remain consistent; locate the button element in ToggleSwitch.tsx and
add aria-disabled={disabled} and a disabled-guard around the onClick (referenced
symbols: ToggleSwitch, enabled, disabled, onChange, aria-pressed).
In `@src/components/layout/Header/components/ChatHeader.tsx`:
- Around line 57-70: The toggle button in ChatHeader.tsx lacks accessibility
attributes; update the button rendered inside the ChatHeader component (the
element using isMuted, isTogglingMute, and calling toggleMute()) to include an
appropriate accessible name and state — e.g., add a dynamic aria-label that
changes based on isMuted (like "Unmute notifications" when isMuted is true and
"Mute notifications" when false) and/or add aria-pressed={isMuted} so screen
readers know the current state; keep the existing disabled handling and onClick
(void toggleMute()) but ensure the aria attributes update inline with isMuted.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/common/ToggleSwitch.tsxsrc/components/layout/Header/components/ChatHeader.tsxsrc/pages/Chat/ChatRoom.tsxsrc/pages/Chat/hooks/useChat.tssrc/pages/Club/Application/clubFeePage.tsxsrc/pages/Club/ClubDetail/components/ClubIntro.tsxsrc/pages/Manager/ManagedClubProfile/index.tsxsrc/pages/Manager/ManagedRecruitment/index.tsxsrc/pages/User/MyPage/index.tsxsrc/pages/User/Profile/index.tsx
Comment on lines
+20
to
23
| const createChatRoomMutation = useMutation({ | ||
| mutationKey: ['createChatRoom'], | ||
| mutationFn: (userId: number) => postChatRooms(userId), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/pages/Chat/hooks/useChat.tsRepository: BCSDLab/KONECT_FRONT_END
Length of output: 3867
🏁 Script executed:
rg -n "createChatRoom" --type=ts --type=tsx -A 5 -B 2Repository: BCSDLab/KONECT_FRONT_END
Length of output: 95
🏁 Script executed:
fd "useChat" --type=ts --type=tsxRepository: BCSDLab/KONECT_FRONT_END
Length of output: 237
🏁 Script executed:
rg "createChatRoom" -A 5 -B 2Repository: BCSDLab/KONECT_FRONT_END
Length of output: 1994
🏁 Script executed:
rg "useChat" -lRepository: BCSDLab/KONECT_FRONT_END
Length of output: 419
에러 처리 및 캐시 무효화 누락 처리 필요
createChatRoomMutation에 에러 핸들링이 없을 뿐만 아니라, 성공 시 캐시 무효화도 누락되어 있습니다. ClubIntro.tsx의 handleInquireClick에서도 try-catch가 없어 요청 실패 시 사용자 피드백이 전혀 없습니다.
다른 mutation들(sendMessage, toggleMute)과 일관되게 다음을 추가하세요:
onSuccess에서 chatRoomList 캐시 무효화onError핸들러 또는 컴포넌트에서 에러 처리
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Chat/hooks/useChat.ts` around lines 20 - 23, The
createChatRoomMutation (mutationFn: postChatRooms) lacks onSuccess/onError
handlers and should mirror other mutations: add an onSuccess that invalidates
the chatRoomList cache (use your query client/invalidation util used elsewhere)
and add an onError to surface errors (e.g., show toast or propagate). Also
update ClubIntro.tsx's handleInquireClick to wrap the createChatRoom call in
try/catch and handle failures (display error feedback) or await the mutation's
promise and use its error handler so request failures don't silently fail.
Comment on lines
50
to
54
| const handleSubmit = async () => { | ||
| if (!imageFile) return; | ||
| setIsSubmitting(true); | ||
|
|
||
| try { | ||
| const { fileUrl } = await uploadImage(imageFile); | ||
| await applyToClub({ answers, feePaymentImageUrl: fileUrl }); | ||
| } finally { | ||
| setIsSubmitting(false); | ||
| } | ||
| const { fileUrl } = await uploadImage(imageFile); | ||
| await applyToClub({ answers, feePaymentImageUrl: fileUrl }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
페이지의 제출 비즈니스 로직은 커스텀 훅으로 분리해 주세요.
Line 50-54의 업로드+지원 orchestration은 라우트 엔트리 페이지보다 훅으로 옮기는 쪽이 유지보수에 유리합니다.
As per coding guidelines "비즈니스 로직은 커스텀 훅으로 분리되어 있는지".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Club/Application/clubFeePage.tsx` around lines 50 - 54, Extract the
submission orchestration from the page into a custom hook (e.g., create
useClubApplication) so handleSubmit only calls the hook-provided submit handler;
specifically move the logic that checks imageFile, calls uploadImage(imageFile)
and then applyToClub({ answers, feePaymentImageUrl: fileUrl }) into the hook,
accept inputs (answers and imageFile or provide setters) and expose a submit
function, loading/error state and any setters needed; update the page to import
useClubApplication and replace the inline handleSubmit with the hook's submit
handler to keep business logic out of the route component.
중복 제출 방지를 위해 handleSubmit 내부 가드를 추가해 주세요.
현재는 버튼 disabled에 의존하고 있어, 재진입 케이스에서 중복 호출 가능성을 완전히 막지 못합니다. Line 50에서 조기 반환 가드를 두는 편이 안전합니다.
🔧 제안 수정
const handleSubmit = async () => {
- if (!imageFile) return;
+ if (!imageFile || isSubmitting) return;
const { fileUrl } = await uploadImage(imageFile);
await applyToClub({ answers, feePaymentImageUrl: fileUrl });
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Club/Application/clubFeePage.tsx` around lines 50 - 54, Add a
reentrancy guard at the start of handleSubmit to prevent duplicate submissions:
introduce a local or component-level boolean (e.g., isSubmitting) that
handleSubmit checks immediately and returns if true, then set isSubmitting =
true before calling uploadImage/applyToClub and reset it in a finally block
(isSubmitting = false) so failures still clear the guard; reference
handleSubmit, imageFile, uploadImage, and applyToClub to locate where to add the
early-return check and the try/finally around the async calls.
Comment on lines
18
to
21
| const handleInquireClick = async () => { | ||
| if (isSubmitting) return; | ||
|
|
||
| try { | ||
| setIsSubmitting(true); | ||
| const response = await createChatRoom(clubDetail.presidentUserId); | ||
| navigate(`/chats/${response.chatRoomId}`); | ||
| } finally { | ||
| setIsSubmitting(false); | ||
| } | ||
| const response = await createChatRoom(clubDetail.presidentUserId); | ||
| navigate(`/chats/${response.chatRoomId}`); | ||
| }; |
There was a problem hiding this comment.
에러 처리 부재: createChatRoom 실패 시 예외가 전파됩니다.
mutateAsync는 실패 시 예외를 throw하므로 try/catch 없이 사용하면 unhandled rejection이 발생할 수 있습니다.
🛡️ 에러 처리 추가 제안
const handleInquireClick = async () => {
+ try {
const response = await createChatRoom(clubDetail.presidentUserId);
navigate(`/chats/${response.chatRoomId}`);
+ } catch {
+ // 에러 처리 (토스트 등)
+ }
};📝 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.
Suggested change
| const handleInquireClick = async () => { | |
| if (isSubmitting) return; | |
| try { | |
| setIsSubmitting(true); | |
| const response = await createChatRoom(clubDetail.presidentUserId); | |
| navigate(`/chats/${response.chatRoomId}`); | |
| } finally { | |
| setIsSubmitting(false); | |
| } | |
| const response = await createChatRoom(clubDetail.presidentUserId); | |
| navigate(`/chats/${response.chatRoomId}`); | |
| }; | |
| const handleInquireClick = async () => { | |
| try { | |
| const response = await createChatRoom(clubDetail.presidentUserId); | |
| navigate(`/chats/${response.chatRoomId}`); | |
| } catch { | |
| // 에러 처리 (토스트 등) | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Club/ClubDetail/components/ClubIntro.tsx` around lines 18 - 21,
handleInquireClick currently calls createChatRoom(clubDetail.presidentUserId)
without error handling so any failure from mutateAsync will bubble up; wrap the
call in a try/catch inside handleInquireClick, await createChatRoom(...) in the
try, navigate(`/chats/${response.chatRoomId}`) on success, and handle failures
in the catch by logging the error (or showing a user-facing message/toast) and
avoiding an unhandled rejection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
새로운 기능
개선사항