[REFACTOR/FIX] 회원 그룹 관리 페이지 구조 개선 및 QA 반영#560
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough상세 페이지의 오케스트레이션 로직을 controller로 이동해 리팩터링했습니다. new: Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 코드 리뷰 피드백1) 책임 분리(아키텍처)
2) 네이밍 & 타입 안전성
3) 에러 처리(안정성)
4) 제출 가능성(canSubmit) 검증 강화
5) 의존성 주입으로 테스트 용이성 향상
6) 퍼블릭 API 축소·문서화
7) 변경된 퍼블릭 동작:
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | 제목이 PR의 주요 변경사항을 명확하게 요약합니다. 회원 그룹 관리 페이지의 구조 개선과 QA 반영이라는 핵심 목표를 정확하게 나타냅니다. |
| Linked Issues check | ✅ Passed | 코드 변경사항이 #559의 목표를 충족합니다. 컨트롤러(useController), 정책 분리(getHeaderConfig, getStickyButtonConfig), Alert/BottomSheet 훅 분리가 구현되어 오케스트레이션 로직의 책임 분리가 명확히 이루어졌습니다. |
| Out of Scope Changes check | ✅ Passed | 모든 변경사항이 #559의 목표 범위 내에 있습니다. MemberGenerationAccordion의 defaultOpen 추가, moveForm에서 copyForm 변경은 모두 구조 개선과 버그 수정에 관련된 필수 변경입니다. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
refactor/group-management-page
Tip
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/admin/src/app-pages/group-management/detail/model/useAlerts.tsx (1)
13-31:useCallback메모이제이션 고려
useBottomSheets훅에서는useCallback으로 핸들러를 메모이제이션했는데, 이 훅에서는 일반 함수로 선언되어 있습니다. 이 핸들러들이 자식 컴포넌트에 props로 전달될 경우 불필요한 리렌더링이 발생할 수 있습니다.일관성과 성능을 위해
useCallback적용을 권장합니다.♻️ useCallback 적용 예시
+ import { useCallback } from 'react'; - const openSaveEditAlert = () => { + const openSaveEditAlert = useCallback(() => { openAlert({ state: 'default', title: '수정하시겠습니까?', // ... }); - }; + }, [openAlert, closeAlert, onSubmitEdit]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/detail/model/useAlerts.tsx` around lines 13 - 31, Wrap the openSaveEditAlert handler in React.useCallback to memoize it (similar to how handlers in useBottomSheets are memoized) so it doesn't change across renders when passed as a prop; reference the openSaveEditAlert function and ensure its dependency array includes openAlert, closeAlert, and onSubmitEdit so the callback updates only when those change.apps/admin/src/app-pages/group-management/ui/GroupManagementDetailPage.tsx (1)
24-31: 로딩 상태 접근성 개선 권장스크린 리더 사용자에게 로딩 상태를 알리기 위해
aria-live또는role="status"속성 추가를 고려해 주세요.♿ 접근성 개선 예시
if (!c.draft) { return ( <div className="flex h-full flex-col"> <AppHeader overrideHeader={c.header} /> - <div className="flex flex-1 items-center justify-center">Loading...</div> + <div className="flex flex-1 items-center justify-center" role="status" aria-live="polite"> + 로딩 중... + </div> </div> ); }As per coding guidelines: "접근성: aria-label, role, 키보드 내비게이션, 포커스 트랩, 대비/색상 의존 금지."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/ui/GroupManagementDetailPage.tsx` around lines 24 - 31, The loading fallback for the GroupManagementDetailPage (the block checking c.draft) lacks screen-reader announcements; update the loading container (the div currently rendering "Loading..." inside the return when !c.draft) to include an accessible live region—e.g., add aria-live="polite" or role="status" (and ensure the text is visible or provide a visually-hidden element with the message) so assistive technologies are notified when the page is loading; target the div that currently has className="flex flex-1 items-center justify-center" to apply this change.apps/admin/src/app-pages/group-management/detail/model/useController.tsx (2)
83-90: safeDraft 객체 메모이제이션 고려
safeDraft가 매 렌더링마다 새 객체를 생성합니다. 현재 사용 패턴에서는 큰 문제가 없지만, 향후 의존성 배열에 사용될 경우 불필요한 리렌더링을 유발할 수 있습니다.♻️ useMemo 적용 예시
- const safeDraft: GroupFormDraft = draft ?? { - generation: maxGeneration, - groupType: 'study' as ContentsType, - groupName: '', - groupIntroduction: '', - leader: undefined, - members: [], - }; + const safeDraft: GroupFormDraft = useMemo(() => draft ?? { + generation: maxGeneration, + groupType: 'study' as ContentsType, + groupName: '', + groupIntroduction: '', + leader: undefined, + members: [], + }, [draft, maxGeneration]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx` around lines 83 - 90, safeDraft is recreated on every render; wrap its creation in useMemo so it only changes when inputs change: compute safeDraft with useMemo based on draft and maxGeneration (keep the fallback shape with generation: maxGeneration, groupType: 'study' as ContentsType, groupName:'', groupIntroduction:'', leader: undefined, members:[]) and ensure useMemo is imported; update references to safeDraft in useController.tsx so dependent hooks/arrays use the memoized value.
117-135: 비동기 핸들러 에러 처리 강화 권장
handleDeleteGroup과handleSubmit에서 mutation 호출 시 에러가 mutation 훅 내부에서 처리되지만, 예기치 않은 에러(네트워크 중단 등)가 발생할 경우 unhandled rejection이 될 수 있습니다.방어적으로 try-catch를 추가하거나, 에러 바운더리와 함께 사용하는 것을 권장합니다.
🛡️ 에러 처리 강화 예시
const handleDeleteGroup = async () => { if (mode !== 'edit') return; + try { await deleteGroup(); showToast('그룹이 삭제되었습니다.'); router.back(); + } catch { + // mutation onError에서 이미 처리됨, 추가 처리 필요시 여기에 작성 + } };As per coding guidelines: "에러/예외 처리 기준: 사용자-facing 에러 메시지, 로깅, 재시도 전략, timeout, abort 등을 일관되게."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx` around lines 117 - 135, handleDeleteGroup and handleSubmit should guard against unexpected rejections by wrapping async mutation calls in try-catch blocks: in handleDeleteGroup wrap await deleteGroup() in try, on success call showToast('그룹이 삭제되었습니다.') and router.back(), on catch call showToast with a user-facing error message and log the error (e.g., console.error or existing logger) without navigating; in handleSubmit keep the isSubmitPending short-circuit, then for the create branch wrap const created = await createGroup(safeDraft) in try and on success router.replace(PAGE_ROUTES.GROUP_MNG.VIEW(created.teamId)) and on error showToast/log the error, and for the edit branch wrap await updateGroup(safeDraft) similarly and only route on success; ensure errors are not swallowed so callers don’t get unhandled rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin/src/app-pages/group-management/detail/model/useAlerts.tsx`:
- Around line 13-31: Wrap the openSaveEditAlert handler in React.useCallback to
memoize it (similar to how handlers in useBottomSheets are memoized) so it
doesn't change across renders when passed as a prop; reference the
openSaveEditAlert function and ensure its dependency array includes openAlert,
closeAlert, and onSubmitEdit so the callback updates only when those change.
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`:
- Around line 83-90: safeDraft is recreated on every render; wrap its creation
in useMemo so it only changes when inputs change: compute safeDraft with useMemo
based on draft and maxGeneration (keep the fallback shape with generation:
maxGeneration, groupType: 'study' as ContentsType, groupName:'',
groupIntroduction:'', leader: undefined, members:[]) and ensure useMemo is
imported; update references to safeDraft in useController.tsx so dependent
hooks/arrays use the memoized value.
- Around line 117-135: handleDeleteGroup and handleSubmit should guard against
unexpected rejections by wrapping async mutation calls in try-catch blocks: in
handleDeleteGroup wrap await deleteGroup() in try, on success call
showToast('그룹이 삭제되었습니다.') and router.back(), on catch call showToast with a
user-facing error message and log the error (e.g., console.error or existing
logger) without navigating; in handleSubmit keep the isSubmitPending
short-circuit, then for the create branch wrap const created = await
createGroup(safeDraft) in try and on success
router.replace(PAGE_ROUTES.GROUP_MNG.VIEW(created.teamId)) and on error
showToast/log the error, and for the edit branch wrap await
updateGroup(safeDraft) similarly and only route on success; ensure errors are
not swallowed so callers don’t get unhandled rejections.
In `@apps/admin/src/app-pages/group-management/ui/GroupManagementDetailPage.tsx`:
- Around line 24-31: The loading fallback for the GroupManagementDetailPage (the
block checking c.draft) lacks screen-reader announcements; update the loading
container (the div currently rendering "Loading..." inside the return when
!c.draft) to include an accessible live region—e.g., add aria-live="polite" or
role="status" (and ensure the text is visible or provide a visually-hidden
element with the message) so assistive technologies are notified when the page
is loading; target the div that currently has className="flex flex-1
items-center justify-center" to apply this change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/admin/src/app-pages/group-management/detail/model/getHeaderConfig.tsapps/admin/src/app-pages/group-management/detail/model/getStickyButtonConfig.tsapps/admin/src/app-pages/group-management/detail/model/useAlerts.tsxapps/admin/src/app-pages/group-management/detail/model/useBottomSheets.tsxapps/admin/src/app-pages/group-management/detail/model/useController.tsxapps/admin/src/app-pages/group-management/ui/GroupManagementDetailPage.tsx
- create 폼 삭제 후에서 상세 페이지로 이동하는 시간 사이 리렌더링이 발생하면 empty space 노출되었음 - 상세 페이지 마운트시 create 폼 삭제
jinj00oo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/admin/src/widgets/group-management/ui/MemberSearchAccordionList.tsx (1)
27-32:⚠️ Potential issue | 🟠 Major모든 아코디언을 기본 오픈하면 초기 로딩 비용이 급격히 커집니다.
지금 구성은
generation개수만큼MemberGenerationAccordion가 마운트 직후 열린 상태가 되어, 세대별 조회와 후속 멤버 조회가 한 번에 발사됩니다. 검색 결과가 많은 운영 환경에서는 첫 렌더 지연과 백엔드 burst가 커지고, 키보드 사용자도 모든 멤버 카드가 즉시 포커스 트리에 들어와 탐색 비용이 커집니다. 기본 오픈이 꼭 필요하다면 첫 섹션만 열거나, 요구사항상 전부 열려야 한다면 세대별 개별 fetch 대신 리스트 레벨 배치 조회로 바꾸는 편이 안전합니다. As per coding guidelines,apps/admin/**: "테이블/필터/페이지네이션은 성능(대량 데이터)과 접근성(키보드/스크린리더) 모두 점검."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/widgets/group-management/ui/MemberSearchAccordionList.tsx` around lines 27 - 32, All accordions are being opened by default which causes heavy initial fetches and accessibility/performance issues; locate the MemberGenerationAccordion usage in MemberSearchAccordionList and remove or conditionally set the defaultOpen prop so not every generation mounts open (e.g., defaultOpen only for the first generation: defaultOpen={generation === firstGeneration} or omit defaultOpen entirely), or if business requires all-open, refactor the data flow so MemberSearchAccordionList performs a single batched members fetch and passes results down to MemberGenerationAccordion to avoid per-section fetch storms (adjust fetch logic invoked by MemberGenerationAccordion accordingly).
🧹 Nitpick comments (1)
apps/admin/src/features/group-management/model/useGroupFormStore.ts (1)
28-29:copyForm의 새 계약이 구현 내부에만 숨어 있습니다.지금 구현은 source를 유지하고,
fromKey누락이나toKey충돌 시 조용히 no-op 합니다. 그런데 반환 타입이void라서 호출부는 복사 성공 여부를 알 수 없고,commit/cleanup 같은 후속 로직을 안전하게 분기하기 어렵습니다. boolean/result 반환이나 JSDoc으로 no-op/cleanup 계약을 밖으로 드러내 두는 편이 안전합니다.As per coding guidelines, "변경이 “규칙/패턴”이라면 docs로 남긴다(컨벤션/ADR/가이드). 단발성 예외는 금지."
Also applies to: 75-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/features/group-management/model/useGroupFormStore.ts` around lines 28 - 29, The copyForm API currently hides its failure semantics by returning void and silently no-oping on missing fromKey or toKey conflicts; update the copyForm signature to return a boolean or a Result-like type so callers can know success/failure (e.g., change copyForm: (fromKey: string, toKey: string, opts?: { overwrite?: boolean }) => boolean) and implement it to return false on missing source or conflict (true on success); additionally, document the contract with JSDoc on copyForm (and mirror behavior doc for removeForm/FormKey if needed) so callers know when cleanup/commit should run and what conditions cause no-op versus overwritten behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`:
- Around line 125-143: The handlers (handleDeleteGroup, handleSubmit) — and
other callbacks mentioned (onSubmitEdit, onDeleteGroup, sticky onCreate/onEdit)
— call TanStack Query mutateAsync (from useCreateGroupMutation,
useDeleteGroupMutation, useUpdateGroupMutation) without catching rejections;
wrap each await mutateAsync(...) call in a try/catch that catches the error (no
extra handling required since onError already logs/shows toast) to prevent
unhandled promise rejections, then proceed with existing post-success logic
(router.replace/router.back/showToast) only after the awaited call resolves
successfully.
- Around line 4-5: Remove the internal Next import (AppRouterInstance) and
replace it with a local RouterLike type that exposes push/replace/back, update
Params to use RouterLike and keep ReadonlyURLSearchParams; then add proper async
error handling around deleteGroup, createGroup and updateGroup in
handleDeleteGroup and handleSubmit (wrap calls in try/catch, showToast or log on
failure, and only call router.back()/router.replace(...) on success) and expose
the handlers directly (use onSubmitEdit: handleSubmit and onDeleteGroup:
handleDeleteGroup instead of wrapping with void) so errors are handled inside
the handlers.
---
Outside diff comments:
In `@apps/admin/src/widgets/group-management/ui/MemberSearchAccordionList.tsx`:
- Around line 27-32: All accordions are being opened by default which causes
heavy initial fetches and accessibility/performance issues; locate the
MemberGenerationAccordion usage in MemberSearchAccordionList and remove or
conditionally set the defaultOpen prop so not every generation mounts open
(e.g., defaultOpen only for the first generation: defaultOpen={generation ===
firstGeneration} or omit defaultOpen entirely), or if business requires
all-open, refactor the data flow so MemberSearchAccordionList performs a single
batched members fetch and passes results down to MemberGenerationAccordion to
avoid per-section fetch storms (adjust fetch logic invoked by
MemberGenerationAccordion accordingly).
---
Nitpick comments:
In `@apps/admin/src/features/group-management/model/useGroupFormStore.ts`:
- Around line 28-29: The copyForm API currently hides its failure semantics by
returning void and silently no-oping on missing fromKey or toKey conflicts;
update the copyForm signature to return a boolean or a Result-like type so
callers can know success/failure (e.g., change copyForm: (fromKey: string,
toKey: string, opts?: { overwrite?: boolean }) => boolean) and implement it to
return false on missing source or conflict (true on success); additionally,
document the contract with JSDoc on copyForm (and mirror behavior doc for
removeForm/FormKey if needed) so callers know when cleanup/commit should run and
what conditions cause no-op versus overwritten behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54a456b4-19af-45b0-bd9d-81e2c6ee8afa
📒 Files selected for processing (5)
apps/admin/src/app-pages/group-management/detail/model/useController.tsxapps/admin/src/features/group-management/model/queries/useCreateGroupMutation.tsapps/admin/src/features/group-management/model/useGroupFormStore.tsapps/admin/src/features/member-by-generation/ui/MemberGenerationAccordion.tsxapps/admin/src/widgets/group-management/ui/MemberSearchAccordionList.tsx
apps/admin/src/app-pages/group-management/detail/model/useController.tsx
Outdated
Show resolved
Hide resolved
apps/admin/src/app-pages/group-management/detail/model/useController.tsx
Show resolved
Hide resolved
- 원인: 수정과 달리 생성 후 canSubmit이 true가 되었음 - 해결: 라우팅 중임을 나타나는 state 추가
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/admin/src/app-pages/group-management/detail/model/useController.tsx (2)
4-5: 🛠️ Refactor suggestion | 🟠 Major
next/dist/*내부 타입 의존은 공개 표면으로 바꾸는 편이 안전합니다.App Router의 공개 문서는 라우터를
next/navigation에서 사용하도록 안내하고, 이 훅이 실제로 쓰는 메서드도push/replace/back뿐입니다. 그래서AppRouterInstance를 내부 빌드 경로에서 직접 가져오기보다, 이 파일에서 필요한 표면만RouterLike로 선언해 받는 쪽이 업그레이드 내성이 더 좋습니다. (nextjs.org)예시
-import type { AppRouterInstance } from 'next/dist/shared/lib/app-router-context.shared-runtime'; import type { ReadonlyURLSearchParams } from 'next/navigation'; + +type RouterLike = { + push: (href: string, options?: { scroll?: boolean }) => void; + replace: (href: string, options?: { scroll?: boolean }) => void; + back: () => void; +}; type Params = { mode: GroupManagementMode; id?: string; - router: AppRouterInstance; + router: RouterLike; searchParams: ReadonlyURLSearchParams; };Also applies to: 25-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx` around lines 4 - 5, The file imports the internal type AppRouterInstance from "next/dist/…" which exposes an unstable public surface; replace that dependency by declaring and using a minimal RouterLike type in useController.tsx that only includes the methods you actually use (push, replace, back) and update function signatures/params that currently reference AppRouterInstance to use RouterLike instead; keep the existing ReadonlyURLSearchParams import from "next/navigation", and apply the same replacement wherever AppRouterInstance is referenced (e.g., the handler/props around the useController function and any variables assigned that type).
136-140:⚠️ Potential issue | 🟠 Major삭제/수정 경로의
mutateAsync거부는 여기서도 잡아주세요.
mutateAsync는 실패 시 reject되는 Promise라서,onError가 있어도 호출부에서 catch하지 않으면 unhandled rejection이 남을 수 있습니다. 지금은 삭제 경로와 수정 경로가await를 try/catch 없이 통과하고, alert 콜백에서는void로 Promise를 버리고 있어서 실패 시 후속 라우팅까지 섞일 위험이 있습니다. 핸들러 내부에서 catch한 뒤 성공했을 때만 토스트와 라우팅을 이어가세요. (tanstack.com)예시
const handleDeleteGroup = async () => { if (mode !== 'edit') return; - await deleteGroup(); - showToast('그룹이 삭제되었습니다.'); - router.back(); + try { + await deleteGroup(); + showToast('그룹이 삭제되었습니다.'); + router.back(); + } catch { + // mutation onError에서 사용자 피드백 처리 + } };} else if (mode === 'edit' && groupId) { - await updateGroup(safeDraft); - router.replace(PAGE_ROUTES.GROUP_MNG.VIEW(groupId)); + try { + await updateGroup(safeDraft); + router.replace(PAGE_ROUTES.GROUP_MNG.VIEW(groupId)); + } catch { + // mutation onError에서 사용자 피드백 처리 + } }const { openSaveEditAlert, openDeleteAlert, openGoBackAlert, openPickLeaderAlert } = useAlerts({ - onSubmitEdit: () => void handleSubmit(), - onDeleteGroup: () => void handleDeleteGroup(), + onSubmitEdit: handleSubmit, + onDeleteGroup: handleDeleteGroup, onLeavePage: handleLeavePage, });As per coding guidelines, "[apps/admin] 어드민 권한/감사 로그/파괴적 액션(삭제 등)은 안전장치(확인 모달, 롤백/재시도, 권한 체크)를 필수로."
Also applies to: 143-158, 175-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx` around lines 136 - 140, The delete handler handleDeleteGroup currently awaits deleteGroup() (a mutateAsync) without try/catch, causing potential unhandled rejections and incorrect post-success actions; wrap the call to deleteGroup() in a try/catch (or use .catch) and only call showToast('그룹이 삭제되었습니다.') and router.back() when the mutation resolves successfully, and in the catch branch log/show an error (e.g., showToast with failure message) to prevent routing on failure; apply the same fix pattern to the edit/update handlers that call mutateAsync (referenced mutate functions around lines after handleDeleteGroup) so all destructive/mutation paths handle rejection explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`:
- Around line 36-37: The current effect hydrates the edit form too early using
only !isGroupDetailLoading which lets the default initialDraft overwrite even on
failed/invalid fetches; update the hydrate guard so that when mode === 'edit'
you only call hydrate (from useGroupFormStore) after confirming groupId is a
valid Number, groupDetail is present (successful fetch), and
isGroupDetailLoading is false (and optionally no fetch error), while keeping the
existing create path (formKey for 'create') unchanged; reference the symbols
groupId, formKey, mode, isGroupDetailLoading, groupDetail, hydrate, and the
hasForm guard to ensure the hydrate call is only reached for true successful
detail loads so server values rehydrate correctly.
---
Duplicate comments:
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`:
- Around line 4-5: The file imports the internal type AppRouterInstance from
"next/dist/…" which exposes an unstable public surface; replace that dependency
by declaring and using a minimal RouterLike type in useController.tsx that only
includes the methods you actually use (push, replace, back) and update function
signatures/params that currently reference AppRouterInstance to use RouterLike
instead; keep the existing ReadonlyURLSearchParams import from
"next/navigation", and apply the same replacement wherever AppRouterInstance is
referenced (e.g., the handler/props around the useController function and any
variables assigned that type).
- Around line 136-140: The delete handler handleDeleteGroup currently awaits
deleteGroup() (a mutateAsync) without try/catch, causing potential unhandled
rejections and incorrect post-success actions; wrap the call to deleteGroup() in
a try/catch (or use .catch) and only call showToast('그룹이 삭제되었습니다.') and
router.back() when the mutation resolves successfully, and in the catch branch
log/show an error (e.g., showToast with failure message) to prevent routing on
failure; apply the same fix pattern to the edit/update handlers that call
mutateAsync (referenced mutate functions around lines after handleDeleteGroup)
so all destructive/mutation paths handle rejection explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bf44a8e-97d0-4572-a7ef-677163f2a6ee
📒 Files selected for processing (2)
apps/admin/src/app-pages/group-management/detail/model/getStickyButtonConfig.tsapps/admin/src/app-pages/group-management/detail/model/useController.tsx
ej070961
reviewed
Mar 6, 2026
Comment on lines
+57
to
+73
| const hasForm = useGroupFormStore((s) => s.forms[formKey] != null); | ||
| const draft = useGroupFormStore((s) => s.forms[formKey]?.draft); | ||
| const dirty = useGroupFormStore((s) => s.forms[formKey]?.dirty ?? false); | ||
| const isValid = useGroupFormStore((s) => s.isValid(formKey)); | ||
| const canSubmit = dirty && isValid; | ||
|
|
||
| const hydrate = useGroupFormStore((s) => s.hydrate); | ||
| const removeForm = useGroupFormStore((s) => s.removeForm); | ||
| const resetDraft = useGroupFormStore((s) => s.resetDraft); | ||
|
|
||
| const setGeneration = useGroupFormStore((s) => s.setGeneration); | ||
| const setGroupType = useGroupFormStore((s) => s.setGroupType); | ||
| const setGroupName = useGroupFormStore((s) => s.setGroupName); | ||
| const setGroupIntroduction = useGroupFormStore((s) => s.setGroupIntroduction); | ||
|
|
||
| const pickLeader = useGroupFormStore((s) => s.pickLeader); | ||
| const removeMember = useGroupFormStore((s) => s.removeMember); |
Contributor
There was a problem hiding this comment.
필요한 값만 구독해 불필요한 리렌더를 줄이려는 의도가 잘 드러나는 코드인거 같네요!!
다만 구독하는 값이 많다 보니 코드가 다소 길어 보이는 것 같습니다.
hydrate, removeForm과 같은 액션 함수들은 일반적으로 참조가 변경되지 않는 값들이라, shallow 비교를 활용해 묶어서 구조 분해하면 가독성을 조금 더 개선할 수 있을 것 같습니다.
예를 들어 아래와 같이 액션들만 별도로 묶어 가져오는 방식도 고려해볼 수 있을 것 같습니다.
const {
hydrate,
removeForm,
resetDraft,
setGeneration,
setGroupType,
setGroupName,
setGroupIntroduction,
pickLeader,
removeMember,
} = useGroupFormStore(
(s) => ({
hydrate: s.hydrate,
removeForm: s.removeForm,
resetDraft: s.resetDraft,
setGeneration: s.setGeneration,
setGroupType: s.setGroupType,
setGroupName: s.setGroupName,
setGroupIntroduction: s.setGroupIntroduction,
pickLeader: s.pickLeader,
removeMember: s.removeMember,
}),
shallow
);
Collaborator
Author
There was a problem hiding this comment.
좋은 의견 감사드립니다!
shallow는 타입스크립트에서 두 번째 인자를 넘기지 못하도록 추론하는 문제가 있어, useShallow를 사용하는 방식으로 수정하였습니다!
- mutateAsync는 Promise 반환하므로 try-catch - Next에서 공식 지원하지 않는 의존성 제거
- 잘못된 id로 접근하는 경우 hydrate 방지
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/admin/src/app-pages/group-management/detail/model/useController.tsx (1)
214-219:void연산자 패턴에 대한 코멘트 권장
() => void handleSubmit()패턴은 Promise 반환값을 명시적으로 무시하는 유효한 방식입니다. 에러 처리가 핸들러 내부에서 완료되므로 안전합니다. 다만 이 패턴이 흔하지 않아 다른 개발자가 혼란스러워할 수 있으니, 간단한 주석을 추가하거나 더 명시적인 래퍼 함수를 고려해볼 수 있습니다.💡 대안 예시
// 옵션 1: 주석 추가 const { openSaveEditAlert, openDeleteAlert, openGoBackAlert, openPickLeaderAlert } = useAlerts({ // void를 사용하여 Promise를 명시적으로 무시 (에러는 핸들러 내부에서 처리) onSubmitEdit: () => void handleSubmit(), onDeleteGroup: () => void handleDeleteGroup(), onLeavePage: handleLeavePage, }); // 옵션 2: 헬퍼 함수 (프로젝트 전반에서 사용할 경우) const fireAndForget = <T extends (...args: never[]) => Promise<unknown>>(fn: T) => (...args: Parameters<T>) => void fn(...args);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx` around lines 214 - 219, The arrow wrappers `() => void handleSubmit()` and `() => void handleDeleteGroup()` silently discard returned Promises which is legal but uncommon; update the `useAlerts` props to make this intent explicit by either adding a brief inline comment near the `onSubmitEdit` and `onDeleteGroup` assignments stating that the Promise is intentionally ignored and errors are handled internally, or replace the pattern with a small, shared `fireAndForget` helper wrapper and use it when passing `handleSubmit` and `handleDeleteGroup` into `useAlerts` so the intent is clear (refer to `useAlerts`, `handleSubmit`, `handleDeleteGroup`, and `fireAndForget`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`:
- Around line 42-43: The current conversion Number(id) can yield NaN which will
make formKey become "NaN" and break the form store; replace the naive cast with
a validation: parse and check the result (e.g., use parseInt/Number and
Number.isFinite or !isNaN) when computing groupId, set groupId to undefined if
invalid, and compute formKey as mode === 'create' ? 'create' : (groupId !==
undefined ? String(groupId) : handle-invalid-id), where "handle-invalid-id" is
either a stable fallback key or trigger an early error/redirect; update the
logic around groupId and formKey in useController.tsx to use these validated
values.
---
Nitpick comments:
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`:
- Around line 214-219: The arrow wrappers `() => void handleSubmit()` and `() =>
void handleDeleteGroup()` silently discard returned Promises which is legal but
uncommon; update the `useAlerts` props to make this intent explicit by either
adding a brief inline comment near the `onSubmitEdit` and `onDeleteGroup`
assignments stating that the Promise is intentionally ignored and errors are
handled internally, or replace the pattern with a small, shared `fireAndForget`
helper wrapper and use it when passing `handleSubmit` and `handleDeleteGroup`
into `useAlerts` so the intent is clear (refer to `useAlerts`, `handleSubmit`,
`handleDeleteGroup`, and `fireAndForget`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eabae0d7-0916-4e9b-a3d9-07d1ab8d4b95
📒 Files selected for processing (1)
apps/admin/src/app-pages/group-management/detail/model/useController.tsx
Comment on lines
+42
to
+43
| const groupId = id ? Number(id) : undefined; | ||
| const formKey = mode === 'create' ? 'create' : String(id); |
There was a problem hiding this comment.
id 파라미터 유효성 검증 필요
Number(id)가 유효하지 않은 문자열에 대해 NaN을 반환할 수 있습니다. NaN은 undefined와 다르게 truthy 체크를 통과하지만 숫자 연산에서는 실패합니다. 이로 인해 formKey가 "NaN"이 되어 예기치 않은 폼 스토어 동작이 발생할 수 있습니다.
🛡️ 유효성 검증 추가 예시
- const groupId = id ? Number(id) : undefined;
- const formKey = mode === 'create' ? 'create' : String(id);
+ const parsedId = id ? Number(id) : undefined;
+ const groupId = parsedId && !Number.isNaN(parsedId) ? parsedId : undefined;
+ const formKey = mode === 'create' ? 'create' : groupId ? String(groupId) : 'invalid';또는 잘못된 id가 전달되었을 때 조기에 에러를 처리하는 방식도 고려해볼 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app-pages/group-management/detail/model/useController.tsx`
around lines 42 - 43, The current conversion Number(id) can yield NaN which will
make formKey become "NaN" and break the form store; replace the naive cast with
a validation: parse and check the result (e.g., use parseInt/Number and
Number.isFinite or !isNaN) when computing groupId, set groupId to undefined if
invalid, and compute formKey as mode === 'create' ? 'create' : (groupId !==
undefined ? String(groupId) : handle-invalid-id), where "handle-invalid-id" is
either a stable fallback key or trigger an early error/redirect; update the
logic around groupId and formKey in useController.tsx to use these validated
values.
ej070961
approved these changes
Mar 8, 2026
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
2 participants
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.
✅ 체크리스트
🔗 관련 이슈
📌 작업 목적
🔨 주요 작업 내용
moveForm으로 폼 이관을 했으므로 바로 데이터가 보이는 흐름이어야 했습니다.moveForm에서create폼을 지운 후부터 상세 페이지로 라우팅 되기 전 순간까지 create 모드로 리렌더링이 일어나면서 empty space가 보인 것으로 확인되었습니다.☑️ 해결 방법 (선택)
moveForm을copyForm으로 변경하여 새로운 폼을 생성하기만 하고, 기존 폼의 삭제는 수행하지 않도록 수정했습니다.create폼을 클린업 하기 위해서 상세 페이지가 마운트되면create폼을 지우는 로직을 추가하였습니다.🧪 테스트 방법
💬 논의할 점
🙋🏻 참고 자료
Summary by CodeRabbit
Summary by CodeRabbit