🧹 Code Health: Handle state reset in event handler instead of useEffect in CreateOrgModal - #70
Conversation
…ct in CreateOrgModal
|
👋 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. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughCreateOrgModal의 모달 닫힘 초기화 로직을 useEffect(open)에서 handleOpenChange로 이동해, 닫힘 시에만 name/errorMessage 초기화와 mutation.reset()을 수행하도록 변경했다. 입력 변경 시 에러 해제와 제출 버튼의 pending 라벨 분기는 유지된다. Changes모달 상태 관리 패턴 리팩토링
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
…ct in CreateOrgModal
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/components/org/create-org-modal.tsx`:
- Around line 48-49: The modal is directly calling onOpenChange(false) which
bypasses the reset logic in handleOpenChange; replace the direct calls to
onOpenChange(false) (the one after await mutation.mutateAsync and the similar
block around the cancel/close path) with calls to handleOpenChange(false) so the
initialization/reset (clearing name, errorMessage, and resetting mutation state)
runs on both successful submit and cancel paths; update every occurrence
(including the mutate success path and the cancel handler) to use
handleOpenChange instead of calling onOpenChange directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4c62d23-c08c-4d7e-bde8-401ec11e3392
📒 Files selected for processing (1)
packages/web/src/components/org/create-org-modal.tsx
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
1 similar comment
|
…ct in CreateOrgModal
|
@coderabbitai resolve |
|
✅ Action performedComments resolved and changes approved. |
…ct in CreateOrgModal
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/web/src/components/org/create-org-modal.tsx (1)
49-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
handleOpenChange(false)경로로 다시 통일하세요.Line 49와 Line 105에서
onOpenChange(false)를 직접 호출하면 Line 30-38의 닫힘 초기화(setName(""),setErrorMessage(null),mutation.reset())가 실행되지 않습니다. 성공 후나 취소 후 다시 열 때 이전 입력값/에러/뮤테이션 상태가 남습니다.🔧 제안
- onOpenChange(false); + handleOpenChange(false); ... - onClick={() => onOpenChange(false)} + onClick={() => handleOpenChange(false)}Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/components/org/create-org-modal.tsx` at line 49, Replace direct calls to onOpenChange(false) at the two spots with the existing wrapper handleOpenChange(false) so the close-path runs the cleanup logic; specifically, change the occurrences that call onOpenChange(false) to invoke handleOpenChange(false) so setName(""), setErrorMessage(null), and mutation.reset() in handleOpenChange are executed when closing (after success or cancel).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/web/src/components/org/create-org-modal.tsx`:
- Line 49: Replace direct calls to onOpenChange(false) at the two spots with the
existing wrapper handleOpenChange(false) so the close-path runs the cleanup
logic; specifically, change the occurrences that call onOpenChange(false) to
invoke handleOpenChange(false) so setName(""), setErrorMessage(null), and
mutation.reset() in handleOpenChange are executed when closing (after success or
cancel).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18d0682a-5a35-4670-bba3-827131174022
📒 Files selected for processing (1)
packages/web/src/components/org/create-org-modal.tsx
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
|
…ct in CreateOrgModal
1 participant
🎯 What: The code health issue addressed
Moved the state reset logic (clearing
name,errorMessage, and resetting themutation) from auseEffectinto thehandleOpenChangeevent handler inCreateOrgModal. Removed the unuseduseEffectimport.💡 Why: How this improves maintainability
The previous implementation used
useEffectto clear state when the modal closed. This led to anexhaustive-depswarning becausemutationwas omitted to avoid cascading renders, violating thereact-hooks/set-state-in-effectrule. By moving this logic to the event handler that actually triggers the close (handleOpenChange), we follow the recommended React pattern of handling state updates in event handlers rather than synchronizing them with effects. This improves maintainability, performance, and resolves the linting errors cleanly.✅ Verification: How you confirmed the change is safe
Ran
pnpm --filter @argos/web lintwhich now passes without theexhaustive-depsorset-state-in-effecterrors. Ranpnpm testin the web package, all tests passed.✨ Result: The improvement achieved
Clean, lint-error-free component that adheres to React best practices for state management.
PR created automatically by Jules for task 16298289325276099530 started by @seonghobae