[Refactor] Observer pattern을 통한 전역 모달 관리#155
Merged
Conversation
- 기존 useModal 훅 기반의 선언적 구조에서 modalStore 기반의 명령형 구조로 전환 - observer Pattern을 활용하여 컴포넌트 내부 JSX와 모달 렌더링 로직의 결합도를 분리
- 수동 닫기(close) 및 페이지 이동(reset) 시 예약된 setTimeout을 명시적으로 클린업하여 사이드 이펙트를 방지합니다.
- 전역 ModalProvider 도입에 따라 ModalBasic 내부의 <Modal> 컨테이너 태그 제거 - Modal, ModalBasic을 사용하는 관련 컴포넌트 일괄 수정
Closed
3 tasks
📝 WalkthroughWalkthrough애플리케이션의 모달 관리를 로컬 훅에서 중앙 Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component
participant ModalStore as modalStore
participant ModalProvider as ModalProvider
participant UI as Modal UI
Component->>ModalStore: open({id, content, autoPlay?, onClose?})
ModalStore->>ModalStore: add modal, start autoPlay timer (선택적)
ModalStore->>ModalProvider: notify subscribers (modals)
ModalProvider->>UI: render modal items
Note over Component,UI: 사용자 상호작용
Component->>ModalStore: close(id)
ModalStore->>ModalStore: cancel timer, remove modal
ModalStore->>ModalStore: 실행 onClose 콜백(있으면)
ModalStore->>ModalProvider: notify subscribers (modals)
ModalProvider->>UI: 제거된 모달 반영
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 제안 라벨
제안 리뷰어
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
🚀 빌드 결과✅ 린트 검사 완료 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shared/ui/modal/modal-basic.tsx (1)
27-41:⚠️ Potential issue | 🟠 MajorX 버튼이
onClose액션을 타지 않습니다.지금 구조에서는
Modal.XButton이ModalProvider의onClose만 호출하고,ModalBasicProps.onClose는 취소 버튼에서만 실행됩니다. 그래서 취소 버튼과 X 버튼의 정리 로직이 달라질 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal-basic.tsx` around lines 27 - 41, The X button currently invokes the provider's close handler instead of the modal instance's onClose prop, causing inconsistent cleanup; update ModalBasic (the JSX block using Modal.XButton, Modal.Buttons, and the props from ModalBasicProps.onClose) so that Modal.XButton receives and calls the same onClose handler passed into the component (e.g., forward the onClose prop down to Modal.XButton or attach a click handler that calls ModalBasicProps.onClose) ensuring both the X button and the cancel Button call the identical onClose logic.src/features/experience-matching/ui/select-company/select-company.tsx (1)
19-19: 🛠️ Refactor suggestion | 🟠 MajorProps 타입을 별도로 정의하는 것을 권장해요.
코딩 가이드라인에 따르면 Props 타입명은 '컴포넌트명Props' 형식을 사용해야 해요. 인라인 타입 대신
SelectCompanyProps인터페이스로 분리하면 가독성과 재사용성이 향상돼요.♻️ 수정 제안
+interface SelectCompanyProps { + onClick: () => void; +} + -export const SelectCompany = ({ onClick }: { onClick: () => void }) => { +export const SelectCompany = ({ onClick }: SelectCompanyProps) => {As per coding guidelines: "Props 타입명이 '컴포넌트명Props' 형식인지 확인"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/experience-matching/ui/select-company/select-company.tsx` at line 19, The SelectCompany component currently uses an inline prop type; extract that into a named interface SelectCompanyProps and update the component signature to use it (e.g., function SelectCompany(props: SelectCompanyProps) or const SelectCompany = ({ onClick }: SelectCompanyProps) => ...), defining SelectCompanyProps with onClick: () => void to follow the "ComponentNameProps" naming guideline and improve readability/reuse (refer to SelectCompany and onClick in the diff).
🤖 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/features/experience-detail/model/use-leave-confirm.tsx`:
- Around line 49-53: The confirmLeave callback currently only calls
blocker.proceed(); update it to also explicitly close the leave modal by calling
modalStore.close(LEAVE_MODAL_ID) (same as cancelLeave) so the modal is dismissed
immediately and the behavior is consistent; locate the confirmLeave function in
use-leave-confirm.tsx and add modalStore.close(LEAVE_MODAL_ID) before or after
blocker.proceed() to ensure the modal is closed on confirmation.
- Line 28: Move the LEAVE_MODAL_ID constant out of the hook to module scope:
declare const LEAVE_MODAL_ID = "leave-confirm-modal" above the useLeaveConfirm
hook definition in use-leave-confirm.tsx (and export it if other modules need
it), then remove the in-hook declaration so all references inside the hook
(e.g., any usages within useLeaveConfirm) point to the module-level constant to
improve readability and reusability.
In `@src/features/experience-detail/ui/experience-viewer/experience-viewer.tsx`:
- Around line 42-55: The delete modal currently calls modalStore.reset() in
handleOpenDeleteModal which closes all global modals; change it to open the
ModalBasic with a fixed unique id (e.g. const id = 'experience-delete') passed
to modalStore.open and then replace modalStore.reset() in both onClose and
onConfirm with modalStore.close(id) so only this modal is closed (keep onConfirm
calling onClickDelete() before modalStore.close(id)). Ensure you pass the id
when opening and use the same id in onClose/onConfirm.
In `@src/features/experience-matching/ui/select-company/select-company.tsx`:
- Line 66: The modal title hardcodes the Korean object particle ("을/를") and
should adapt to whether selectedCompany.name ends with a batchim; update the
text in Modal.Title where selectedCompany.name is used to either (a) call a
small utility (e.g., a josa helper like getJosa(name, '을', '를') or
hasBatchim-based function) and render "{selectedCompany.name}{josa} 선택하셨습니다", or
(b) switch to a neutral phrasing such as "선택한 기업: {selectedCompany.name}" to
avoid particle logic; modify the component to import/implement that josa helper
and use it around selectedCompany.name in the Modal.Title or replace the string
with the neutral form.
- Around line 60-77: The function handleSearch is misnamed because it doesn't
perform a search but confirms the selected company and shows a loading modal;
rename the function (e.g., to handleSelectConfirm or handleCompanySelect) and
update all references/usages to that new name (such as any onClick or prop
handlers that currently call handleSearch) so callers invoke the new identifier;
modify the function declaration for handleSearch to the chosen name and adjust
any exports/props passed into child components to match the new name, preserving
the existing logic that checks selectedCompany, opens modalStore, calls
setCompany(selectedCompany), and invokes onClick().
In `@src/shared/model/store/modal.store.ts`:
- Around line 57-59: The reset() method currently just clears _modalList and
calls _listner, which skips per-modal cleanup (timers and onClose); change
reset() to iterate over the existing _modalList (use a shallow copy) and call
the existing close(...) code-path for each modal (e.g., invoking the store's
close(id) or the modal's onClose/clearTimeout logic) so all timers and onClose
handlers run, then set _modalList = [] and invoke _listner once; ensure you
reference the existing close(...) function and _modalList/_listner symbols so
you reuse the proper cleanup logic rather than simply emptying the array.
- Around line 23-30: The modal ID generation in open() using new
Date().toString() can collide when open() is called rapidly; update open() to
generate collision-resistant IDs (e.g., use crypto.randomUUID() or a UUID v4
library, or a monotonic counter) instead of new Date().toString(); ensure the
generated id is assigned to new_modal.id and that existing close()/filter()
logic that relies on id (and this._modalList) continues to work with the new ID
format (update imports if you choose a UUID library).
- Around line 3-8: Change the ModalItem object shape from a type alias to an
interface: replace the `type ModalItem = { ... }` declaration with `interface
ModalItem { id: string; content: ReactNode; onClose?: () => void; autoPlay?:
number; }` so it follows the project's guideline of using interface for object
shapes; update any imports/exports or references to ModalItem in modal.store.ts
or other files if necessary to ensure the symbol name remains the same and
compiles.
---
Outside diff comments:
In `@src/features/experience-matching/ui/select-company/select-company.tsx`:
- Line 19: The SelectCompany component currently uses an inline prop type;
extract that into a named interface SelectCompanyProps and update the component
signature to use it (e.g., function SelectCompany(props: SelectCompanyProps) or
const SelectCompany = ({ onClick }: SelectCompanyProps) => ...), defining
SelectCompanyProps with onClick: () => void to follow the "ComponentNameProps"
naming guideline and improve readability/reuse (refer to SelectCompany and
onClick in the diff).
In `@src/shared/ui/modal/modal-basic.tsx`:
- Around line 27-41: The X button currently invokes the provider's close handler
instead of the modal instance's onClose prop, causing inconsistent cleanup;
update ModalBasic (the JSX block using Modal.XButton, Modal.Buttons, and the
props from ModalBasicProps.onClose) so that Modal.XButton receives and calls the
same onClose handler passed into the component (e.g., forward the onClose prop
down to Modal.XButton or attach a click handler that calls
ModalBasicProps.onClose) ensuring both the X button and the cancel Button call
the identical onClose logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 071436a1-c2fd-469b-a589-ac7dab18f1f0
📒 Files selected for processing (10)
src/app/providers/modal-provider.tsxsrc/app/routes/root-layout.tsxsrc/features/experience-detail/model/use-leave-confirm.tsxsrc/features/experience-detail/ui/experience-viewer/experience-viewer.tsxsrc/features/experience-matching/ui/select-company/select-company.tsxsrc/pages/experience-detail/experience-detail-page.tsxsrc/shared/model/store/index.tssrc/shared/model/store/modal.store.tssrc/shared/ui/modal/modal-basic.tsxsrc/shared/ui/textfield/textfield.tsx
Comment on lines
+42
to
+55
| const handleOpenDeleteModal = () => { | ||
| modalStore.open( | ||
| <ModalBasic | ||
| title="이 경험을 삭제하시겠습니까?" | ||
| subTitle="작성한 내용은 즉시 제거되며, 복구할 수 없습니다." | ||
| closeText="취소" | ||
| confirmText="삭제" | ||
| onClose={() => modalStore.reset()} // 취소 시 닫기 | ||
| onConfirm={() => { | ||
| onClickDelete(); // 실제 삭제 동작 | ||
| modalStore.reset(); // 모달 닫기 | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
이 확인창에서 reset()을 쓰면 다른 전역 모달까지 같이 닫힙니다.
onClose와 onConfirm이 모두 modalStore.reset()을 호출해서, 이 삭제 모달과 무관한 다른 모달도 함께 사라집니다. 여기서는 고정 ID를 넘기고 modalStore.close(id)로 자기 자신만 닫도록 분리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/experience-detail/ui/experience-viewer/experience-viewer.tsx`
around lines 42 - 55, The delete modal currently calls modalStore.reset() in
handleOpenDeleteModal which closes all global modals; change it to open the
ModalBasic with a fixed unique id (e.g. const id = 'experience-delete') passed
to modalStore.open and then replace modalStore.reset() in both onClose and
onConfirm with modalStore.close(id) so only this modal is closed (keep onConfirm
calling onClickDelete() before modalStore.close(id)). Ensure you pass the id
when opening and use the same id in onClose/onConfirm.
src/features/experience-matching/ui/select-company/select-company.tsx
Outdated
Show resolved
Hide resolved
- 동적변수명 (한글/영어) 뒤에 오는 조사를 구분하여 붙이기 위한 es-hangul 패키지 설치
- 상수(LEAVE_MODAL_ID) 훅 외부로 이동 - confirmLeave 모달에서 명시적으로 모달이 닫히도록 modal.close() 호출 - modalItem interface 변경 - reset 메서드에서 예약된 타이머도 함께 제거
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/shared/model/store/modal.store.ts (2)
57-63:⚠️ Potential issue | 🟠 Major
reset()이close()경로를 우회해서 정리가 달라집니다.여기서는 타이머와 리스트만 비우고 각 모달의
onClose는 호출하지 않습니다. 그런데src/app/providers/modal-provider.tsx:23-25에서 pathname 변경마다modalStore.reset()을 호출하므로, 라우트 이동은 사실상 “모달 닫힘”의 한 경로인데 cleanup semantics가 달라집니다. 기존 모달들을 순회하면서close(id)를 재사용하도록 맞추는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 57 - 63, reset() currently clears timers and the _modalList without invoking each modal's onClose, which diverges from the close() cleanup path used on navigation; change reset() to iterate the current _modalList (shallow copy it first) and call close(id) for each modal id so the existing close(id) semantics (onClose callbacks, timer cleanup, and listener notifications) are reused; ensure you still clear _timers/_modalList as needed after using close(id) to avoid double-cleanup or keeping stale references, and reference the reset(), close(id), _modalList, _timers, and _listner symbols when making the change.
23-30:⚠️ Potential issue | 🟠 Major기본 모달 ID 생성이 충돌 가능합니다.
new Date().toString()는 짧은 시간 안에 여러open()이 호출되면 같은 값을 만들 수 있어서, 타이머가 덮어써지거나close(id)가 의도치 않게 여러 모달에 영향을 줄 수 있습니다. 충돌 없는 ID 생성 방식으로 바꿔두는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 23 - 30, The modal ID generation in open(...) (new_modal) uses new Date().toString(), which can collide when open() is called rapidly; replace that default with a collision-resistant generator (e.g., crypto.randomUUID() or a project-wide nanoid utility or an internal monotonic counter) so each created modal gets a unique id; update the open function's default id behavior (and any tests/consumers if they rely on stringified Date) to use the chosen generator and keep references to the id field (new_modal.id, close(id)) intact.
🤖 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/features/experience-matching/ui/select-company/select-company.tsx`:
- Around line 60-79: handleSearch can be re-entered on rapid clicks causing
setCompany(selectedCompany) and onClick() to run multiple times; add a pending
guard (e.g., a useRef or state boolean like isPending) checked at the top of
handleSearch to return early if true, set isPending = true immediately before
calling modalStore.open, and clear/reset isPending in the modalStore.open
completion callback (the function that currently calls setCompany and onClick)
or after the 3s timeout so the flow only executes once per selection;
alternatively disable the related button when isPending is true. Ensure you
reference and update the same pending flag around handleSearch, modalStore.open,
setCompany and onClick to prevent double execution.
In `@src/shared/model/store/modal.store.ts`:
- Around line 12-20: The current implementation stores a single callback in
_listner and clears it globally in unsubscribe(), which overwrites earlier
subscribers and allows one component's cleanup to remove others; change _listner
to a collection (e.g., Set or array) of callbacks, update subscribe(callback:
(list: ModalItem[]) => void) to add the callback to that collection and return a
dedicated cleanup function that removes only that callback, and update
unsubscribe() (or remove it) so it no longer nulls all listeners; update any
emit/notify logic to iterate the collection when broadcasting modal list
changes.
---
Duplicate comments:
In `@src/shared/model/store/modal.store.ts`:
- Around line 57-63: reset() currently clears timers and the _modalList without
invoking each modal's onClose, which diverges from the close() cleanup path used
on navigation; change reset() to iterate the current _modalList (shallow copy it
first) and call close(id) for each modal id so the existing close(id) semantics
(onClose callbacks, timer cleanup, and listener notifications) are reused;
ensure you still clear _timers/_modalList as needed after using close(id) to
avoid double-cleanup or keeping stale references, and reference the reset(),
close(id), _modalList, _timers, and _listner symbols when making the change.
- Around line 23-30: The modal ID generation in open(...) (new_modal) uses new
Date().toString(), which can collide when open() is called rapidly; replace that
default with a collision-resistant generator (e.g., crypto.randomUUID() or a
project-wide nanoid utility or an internal monotonic counter) so each created
modal gets a unique id; update the open function's default id behavior (and any
tests/consumers if they rely on stringified Date) to use the chosen generator
and keep references to the id field (new_modal.id, close(id)) intact.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aed1f2f9-d101-4c3d-9841-17fa96a619e0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yamland included by none
📒 Files selected for processing (4)
package.jsonsrc/features/experience-detail/model/use-leave-confirm.tsxsrc/features/experience-matching/ui/select-company/select-company.tsxsrc/shared/model/store/modal.store.ts
Comment on lines
+60
to
+79
| const handleSearch = () => { | ||
| if (!selectedCompany) return; | ||
| // 기업 선택 후, 대기하는 모달 | ||
| modalStore.open( | ||
| <> | ||
| <Modal.Content type="auto"> | ||
| <Modal.Title> | ||
| {josa(selectedCompany.name, "을/를")} 선택하셨습니다 | ||
| </Modal.Title> | ||
| <Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle> | ||
| </Modal.Content> | ||
| <Modal.Image /> | ||
| </>, | ||
| 3000, | ||
| () => { | ||
| setCompany(selectedCompany); | ||
| onClick(); | ||
| } | ||
| } | ||
| prevIsOpen.current = isOpen; | ||
| }, [isOpen, selectedCompany, onClick, setCompany]); | ||
| ); | ||
| }; |
There was a problem hiding this comment.
이 플로우는 재진입 방지가 필요합니다.
handleSearch가 클릭마다 새 3초 타이머를 등록하는데, 선택 버튼은 onSearch?.()를 동기적으로 바로 호출합니다. 그래서 빠른 더블클릭이면 setCompany(selectedCompany)와 onClick()이 여러 번 실행될 수 있습니다. 이 구간은 pending 플래그로 한 번만 실행되게 잠그거나, 버튼을 즉시 비활성화하는 방어가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/experience-matching/ui/select-company/select-company.tsx` around
lines 60 - 79, handleSearch can be re-entered on rapid clicks causing
setCompany(selectedCompany) and onClick() to run multiple times; add a pending
guard (e.g., a useRef or state boolean like isPending) checked at the top of
handleSearch to return early if true, set isPending = true immediately before
calling modalStore.open, and clear/reset isPending in the modalStore.open
completion callback (the function that currently calls setCompany and onClick)
or after the 3s timeout so the flow only executes once per selection;
alternatively disable the related button when isPending is true. Ensure you
reference and update the same pending flag around handleSearch, modalStore.open,
setCompany and onClick to prevent double execution.
- 기존 단일 listener 구조에서 다중 subscriber를 지원하도록 observer 패턴 구조 수정 - subscribe가 cleanup 함수를 반환하도록 변경하여 각 subscriber가 독립적으로 해제되도록 개선 - store 레이어의 확장성과 안정성을 고려한 observer 패턴 구조 명확화
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/shared/model/store/modal.store.ts (2)
63-69:⚠️ Potential issue | 🟠 Major
reset()이 모달별 cleanup 경로를 우회합니다.지금 구현은 타이머만 지우고 리스트를 비워서 각 모달의
onClose가 호출되지 않습니다. 라우트 이동 시 cleanup을onClose에 의존하는 호출부는 정리가 누락되므로, 현재 목록의 사본을 순회하면서 기존close(id)경로를 재사용하는 쪽이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 63 - 69, The reset() method currently clears timers and empties _modalList directly, bypassing each modal's onClose; instead, create a shallow copy of _modalList and iterate it, calling the existing close(id) path for each modal (so onClose runs), then clear this._timers and finally notify(); reference the reset(), _timers, _modalList and close(id) symbols and ensure you reuse close(id) rather than directly mutating _modalList.
29-45:⚠️ Potential issue | 🟠 Major기본 모달 ID는 충돌할 수 있습니다.
new Date().toString()는 짧은 시간 안에 여러open()이 호출되면 같은 값을 만들 수 있어서,close()/타이머 정리가 다른 모달까지 엮일 수 있습니다. 기본값은crypto.randomUUID()처럼 충돌 저항성이 있는 방식으로 바꾸는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 29 - 45, The default modal ID generation in open(...) using new Date().toString() can collide; update the open method signature (open in modal.store.ts) so the default id is generated with a collision-resistant approach (e.g., crypto.randomUUID()) instead of new Date().toString(); ensure the rest of the logic that references id (new_modal creation, this._modalList update, this._timers.set(id, ...), and close(id) usage) continues to work with the new ID format and keep the existing behavior when a caller supplies an explicit id.
🤖 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/app/providers/modal-provider.tsx`:
- Around line 16-21: The current modal subscription uses useState + useEffect
(modalStore.subscribe -> setModals) which is not safe for concurrent rendering;
replace this pattern by implementing a getSnapshot function that reads current
modalStore state and subscribe function that delegates to modalStore.subscribe,
then call React's useSyncExternalStore(subscribe, getSnapshot) instead of
useState/useEffect so the component reads synchronized snapshots from
modalStore; update references to useSyncExternalStore, modalStore, and remove
the useEffect/setModals logic accordingly.
- Line 6: The file imports Modal using a deep relative path
("../../shared/ui/modal/modal") which breaks consistency with the other alias
imports; replace that import with the project's alias import (e.g.,
"@/shared/ui/modal/modal") in the modal-provider file where Modal is imported so
it matches the existing alias style, and ensure the import statement referencing
Modal is updated accordingly (no other changes required).
- Around line 8-12: The local ModalItem interface in modal-provider.tsx
duplicates the store type; remove the local declaration and import the canonical
ModalItem type exported by the store (the ModalItem exported from modal.store)
so both the provider and store share a single definition; update any uses in
ModalProvider (e.g., state, props, functions referencing ModalItem) to use the
imported type and ensure the store file exports ModalItem if it doesn't already.
In `@src/shared/model/store/modal.store.ts`:
- Around line 35-36: Rename the local variable new_modal to camelCase newModal
and build the modal object using property shorthand (e.g., { id, content,
autoPlay, onClose }) instead of the verbose form, then append it to
this._modalList (update any occurrences of new_modal to newModal); this keeps
consistency with _modalList and other identifiers in this file.
- Line 13: The private field _timers is typed as Map<string, NodeJS.Timeout>,
which can break in browser builds; change its type to Map<string,
ReturnType<typeof setTimeout>> (and adjust the initializer if needed) so the
timeout type is environment-agnostic; also update any code that assumes
NodeJS.Timeout (e.g., clearTimeout calls or variable annotations) to use the new
ReturnType<typeof setTimeout> type or infer the type from setTimeout results.
---
Duplicate comments:
In `@src/shared/model/store/modal.store.ts`:
- Around line 63-69: The reset() method currently clears timers and empties
_modalList directly, bypassing each modal's onClose; instead, create a shallow
copy of _modalList and iterate it, calling the existing close(id) path for each
modal (so onClose runs), then clear this._timers and finally notify(); reference
the reset(), _timers, _modalList and close(id) symbols and ensure you reuse
close(id) rather than directly mutating _modalList.
- Around line 29-45: The default modal ID generation in open(...) using new
Date().toString() can collide; update the open method signature (open in
modal.store.ts) so the default id is generated with a collision-resistant
approach (e.g., crypto.randomUUID()) instead of new Date().toString(); ensure
the rest of the logic that references id (new_modal creation, this._modalList
update, this._timers.set(id, ...), and close(id) usage) continues to work with
the new ID format and keep the existing behavior when a caller supplies an
explicit id.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34e98715-258b-4518-ab8d-0c8d7f82eca8
📒 Files selected for processing (2)
src/app/providers/modal-provider.tsxsrc/shared/model/store/modal.store.ts
Comment on lines
+16
to
+21
| const [modals, setModals] = useState<ModalItem[]>([]); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribe = modalStore.subscribe(setModals); | ||
| return unsubscribe; | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
외부 store 구독은 useSyncExternalStore로 바꾸는 편이 더 견고합니다.
지금 패턴은 useState([])로 시작한 뒤 effect에서 subscribe만 등록하므로, 현재 snapshot 동기화와 concurrent rendering 안정성을 React가 직접 보장해주지 않습니다. modalStore가 외부 store라면 getSnapshot을 제공하고 useSyncExternalStore로 연결하는 쪽이 더 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/providers/modal-provider.tsx` around lines 16 - 21, The current modal
subscription uses useState + useEffect (modalStore.subscribe -> setModals) which
is not safe for concurrent rendering; replace this pattern by implementing a
getSnapshot function that reads current modalStore state and subscribe function
that delegates to modalStore.subscribe, then call React's
useSyncExternalStore(subscribe, getSnapshot) instead of useState/useEffect so
the component reads synchronized snapshots from modalStore; update references to
useSyncExternalStore, modalStore, and remove the useEffect/setModals logic
accordingly.
| class ModalStore { | ||
| private _modalList: ModalItem[] = []; // 모달 리스트 관리 | ||
| private _listeners = new Set<(list: ModalItem[]) => void>(); | ||
| private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
브라우저 코드에서 NodeJS.Timeout 고정은 타입 환경에 따라 깨질 수 있습니다.
클라이언트 번들에서는 setTimeout 반환형이 DOM/lib 설정에 따라 달라서, 여기처럼 NodeJS.Timeout로 고정하면 타입 충돌이 날 수 있습니다. ReturnType<typeof setTimeout>로 두면 런타임 환경에 독립적으로 안전합니다.
🔧 제안 코드
- private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리
+ private _timers = new Map<string, ReturnType<typeof setTimeout>>(); // 타이머 관리📝 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
| private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리 | |
| private _timers = new Map<string, ReturnType<typeof setTimeout>>(); // 타이머 관리 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/model/store/modal.store.ts` at line 13, The private field _timers
is typed as Map<string, NodeJS.Timeout>, which can break in browser builds;
change its type to Map<string, ReturnType<typeof setTimeout>> (and adjust the
initializer if needed) so the timeout type is environment-agnostic; also update
any code that assumes NodeJS.Timeout (e.g., clearTimeout calls or variable
annotations) to use the new ReturnType<typeof setTimeout> type or infer the type
from setTimeout results.
hummingbbird
approved these changes
Mar 15, 2026
Contributor
hummingbbird
left a comment
hummingbbird
left a comment
There was a problem hiding this comment.
수고하셨습니다! 모달에 옵저버 패턴을 적용하는 방식은 처음 보는 거 같은데, 상태 관리, 책임 분리 측면에서 매우 적절한 리팩토링이라고 생각해요. 특히 (아래에도 써두었지만) useModal 없이 모달을 관리하고, jsx 코드가 분리되는 걸 처음봐서 더더욱 놀랐어요 😳😳 능동적으로 코드의 상태를 고민하고 개선해나가는 모습 정말 멋집니다 배워가요 !!!!!
pr도 잘 작성해주시고, 사용되는 모달들을 전반적으로 수정해주셔서 추후에 모달이 추가되는 상황에도 매우 쉽게! 모달을 사용할 수 있을 거 같네요! 수고 많으셨어요 수빈뜨 ~~~ 👍👍
Comment on lines
+11
to
+13
| private _modalList: ModalItem[] = []; // 모달 리스트 관리 | ||
| private _listeners = new Set<(list: ModalItem[]) => void>(); | ||
| private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리 |
Contributor
There was a problem hiding this comment.
ModalStore에 사용되는 private 변수들의 네이밍을 '_어쩌구'로 한 이유가 있을까요?! (just wonder!!!!)
Collaborator
Author
There was a problem hiding this comment.
class 내부에서만 사용되는(외부에서 접근되지 않는) 변수들에 대해서는 관습적인 변수 앞에 _를 붙여 해당 변수는 외부에서 접근/사용하지 말 것을 알리는 용도로 사용했습니다!
실제로 _ 가 어떠한 기능이 있는 것은 아니구요 예전부터 습관처럼... _를 붙이게 되네요
Contributor
There was a problem hiding this comment.
실제로 리팩토링된 부분을 보니 바로 이해가 되네요 !!! useModal 없이 모달을 관리할 수 있게 되면서, jsx 코드가 분리되고 별도의 선언 없이 store를 사용해 모달을 열 수 있는 점도 너무 좋습니다! 모달에 들어갈 내용들을 사용처에서 입력해주는 것도 응집도 측면에서 정말 좋은 거 같아요 천재 개발자 오수빈답습니다 👍👍
Collaborator
Author
There was a problem hiding this comment.
계속해서 모달에 대한 상태 관리가 use-modal로 인해 분산되어 있다는 생각을 해서 왜 인지 모르게 찜찜했는데
use-modal 없이 modal을 관리할 수 있게 되어서 햅삐하고, 거대칭찬을 받아서 또 햅삐하네요 //
| }: ModalBasicProps) => { | ||
| return ( | ||
| <Modal isOpen={isOpen} onClose={onClose}> | ||
| <> |
| <> | ||
| <Modal.Content type="auto"> | ||
| <Modal.Title> | ||
| {josa(selectedCompany.name, "을/를")} 선택하셨습니다 |
Collaborator
Author
There was a problem hiding this comment.
세상엔 천재들이 많다..... 이런 라이브러리도 만들고....
Collaborator
|
모달의 렌더링 책임을 다만 코드를 보며 store가 자세한 사항은 아래에 코멘트로 남겨두겠습니다 수고 많으셨습니다 |
u-zzn
reviewed
Mar 16, 2026
| id: string = new Date().toString() | ||
| ) { | ||
| const new_modal = { id: id, content: content, autoPlay, onClose }; | ||
| this._modalList = [...this._modalList, new_modal]; |
Collaborator
There was a problem hiding this comment.
여기서 동일한 id의 모달이 이미 열려 있는 경우를 한 번 방어해두면 더 안전할 것 같아요.
현재 구현에서는 같은 id로 open()이 다시 호출되더라도 그대로 배열에 추가되기 때문에 useEffect 재실행이나 버튼 중복 클릭 등의 상황에서 동일한 의미의 모달이 여러 번 쌓일 수도 있을 것 같아요!
특히 useLeaveConfirm처럼 특정 모달을 고정 id로 관리하는 패턴이 사용되고 있기 때문에, store 레벨에서 한 번 방어해두면 이후 사용처에서도 훨씬 안정적으로 사용할 수 있을 것 같은데, 아래와 같은 방식으로 수정하는건 어떻게 생각하시나요?
- 동일한
id가 이미 존재하면 기존 모달을 replace - 혹은 이미 존재할 경우 추가하지 않고 ignore
const existingIndex = this._modalList.findIndex((m) => m.id === id);
if (existingIndex !== -1) {
// 기존 모달 교체
this._modalList = this._modalList.map((modal) =>
modal.id === id ? newModal : modal
);
} else {
this._modalList = [...this._modalList, newModal];
}
Collaborator
Author
There was a problem hiding this comment.
오, 짧은 시간 내에 중복 클릭되어 같은 모달이 띄워지는 경우를 고려해주는 것이
확실히 예상치 못한 UI 에러를 방지할 수 있을 것 같아요, 제안해주신 방향 참고하여 반영하도록 하겠습니다 !
| subTitle="작성한 내용은 즉시 제거되며, 복구할 수 없습니다." | ||
| closeText="취소" | ||
| confirmText="삭제" | ||
| onClose={() => modalStore.reset()} // 취소 시 닫기 |
Collaborator
There was a problem hiding this comment.
현재 store는 modalList 기반으로 여러 모달을 관리할 수 있게 설계되어 있는 것 같은데, 여기서는 특정 삭제 확인 모달만 닫는 목적임에도 reset()으로 전체 모달을 비우고 있어요. 지금은 문제 없을 수 있지만, 이후 다른 전역 모달이 함께 열리는 경우 의도치 않게 전부 닫히는 부작용이 생길 수도 있을 것 같아요!
store에서 close(id)와 reset()의 책임이 분리되어 있는 만큼, 여기서는 reset()보다는 close(id)로 해당 모달만 닫는 쪽이 설계 의도와도 더 잘 맞고 안전할 것 같은데, 어떻게 생각하시나요?
Collaborator
Author
There was a problem hiding this comment.
말씀해주신대로 close(id)로 모달을 개별적으로 닫는 것이 안전한 방향이지만,
experience-viewer 경험/등록/삭제에서의 모달 관련한 수정 코드는 이어 해당 기능들이 리팩토링이 진행될 부분임을 감안해서 모달 동작이 정상적으로 이루어지게끔만 수정 코드를 최대한 줄여 임시적인 느낌으로 수정한 상태입니다! 해당 부분 관련해서는 리팩토링 하면서 함께 반영해주시면 좋을 것 같아요! 🫰🏻
Collaborator
There was a problem hiding this comment.
넵 이해했습니다!! 말씀해주신 부분은 제가 추후 리팩토링 진행하면서 close(id) 기반으로 정리해보겠습니다. 감사합니다 🙂
- 동일한 ID의 모달이 호출되는 경우, 가장 최근의 모달을 최상단으로 올리기 위해 기존의 모달은 제거한 후 최상단에 최신의 모달만 업데이트하도록 수정하였습니다.
u-zzn
approved these changes
Mar 16, 2026
qowjdals23
approved these changes
Mar 16, 2026
Collaborator
qowjdals23
left a comment
qowjdals23
left a comment
There was a problem hiding this comment.
모달 관리를 Observer 패턴으로 전역에서 처리할 생각을 하시다니..
덕분에 각 화면 컴포넌트에서 useModal 여러 번 들고 있지 않아도 되고 jsx도 훨씬 가벼워져서 구조도 깔끔하고 가독성도 너무너무 좋아진 것 같습니다 🤩
단순 기능 수정을 넘어 프로젝트 구조 자체를 견고하게 만들려고 노력하신게 코드 곳곳에서 느껴지네요 !! 너무 수고많으셨습니다~~~ 오늘도 많이 배워가요 ~~ 👍
Comment on lines
+23
to
+26
| useEffect(() => { | ||
| modalStore.reset(); | ||
| }, [pathname]); | ||
|
|
Collaborator
There was a problem hiding this comment.
지금 reset 기준이 pathname만 보고 있어서 같은 페이지 안에서 query string이나 hash만 바뀌는 이동에서는 모달이 그대로 남을 수도 있을 것 같아요 ! 혹시 이 부분은 의도적으로 두신 건지 궁금합니다 ! 😉
Collaborator
Author
There was a problem hiding this comment.
지금 당장은 query를 통해 url이 바뀌는 페이지에서 모달이 사용되는 경우가 없어
주요하게 생각하지 않은 부분이었어요 히힣.
추후에 다른 페이지에서 모달이 쓰일 때 충분히 발생할 수 있는 이슈인 것 같아요. 해당 부분 관련해서 pathname이 아닌 location.key를 활용해 수정하도록 하겠습니다 !
Comment on lines
+35
to
+39
| // 경험 등록 여부 확인 모달 | ||
| useEffect(() => { | ||
| if (data?.totalElements === 0) { | ||
| if (!alertModal.isOpen) { | ||
| alertModal.openModal(); | ||
| } | ||
| modalStore.open( | ||
| <> |
Collaborator
There was a problem hiding this comment.
data?.totalElements === 0일 때마다 바로 modalStore.open()을 호출하고 있어서, 쿼리 refetch가 일어나거나 컴포넌트가 다시 마운트되는 경우 동일한 안내 모달이 한 번 더 쌓일 수 있을 것 같아요 !
이전에 open 여부를 한 번 체크해주는 흐름이 있었던 것 처럼 여기서도 고정 id를 넘겨서 중복 노출을 방지하는건 어떻게 생각하시나요 !? 👀
Collaborator
Author
There was a problem hiding this comment.
쿼리 refetch가 일어나는 경우는 다른 페이지에서 현재 페이지로 다시 포커스가 되거나(refetchOnWindowFocus), query-key가 변경될 때일텐데, 전역적으로 refetchOnWindowFocus 옵션을 false로 설정해두었고, query-key가 변경될 가능성이 낮다고 생각했습니다!
또한 컴포넌트의 마운트의 경우에도 사실상 modal-provider의 useEffect에서 location.key(url이 변경될 때)가 변경됨에 따라 modal.reset()이 실행되어 modal_list가 한 번 비워지기 때문에, 중복 모달이 띄워질 가능성은 낮은 구조이기도 합니다.
하지만 개발 환경의 Strict Mode나 예외적인 리마운트 상황까지 고려했을 때, 고정 ID를 설정하는 것이 안정성을 보장하는 방안이라고 생각이 들어 해당 부분 함께 반영하도록 하겠습니다!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/app/providers/modal-provider.tsx (3)
6-6: 🛠️ Refactor suggestion | 🟠 Major깊은 상대 경로 import는 alias로 통일해주세요.
Line 4에서 이미
@/shared/model/storealias를 사용하고 있는데, Line 6만 상대 경로를 사용하고 있어요.@/shared/ui/modal/modal로 변경하면 일관성과 유지보수성이 좋아져요.🔧 제안 코드
-import { Modal } from "../../shared/ui/modal/modal"; +import { Modal } from "@/shared/ui/modal/modal";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/providers/modal-provider.tsx` at line 6, The import for Modal uses a deep relative path; change it to the project alias to match the rest of the file (e.g., use '@/shared/ui/modal/modal' instead of '../../shared/ui/modal/modal') so imports are consistent with the existing '@/shared/model/store' alias and improve maintainability; update the import that references Modal accordingly.
16-21: 🧹 Nitpick | 🔵 Trivial외부 store 구독은
useSyncExternalStore를 사용하면 더 안전해요.현재
useState+useEffect패턴은 concurrent rendering에서 tearing 문제가 발생할 수 있어요. React 18+에서는 외부 store 연동 시useSyncExternalStore를 권장해요.🔧 제안 코드
-import { useEffect, useState, type ReactNode } from "react"; +import { useSyncExternalStore } from "react"; import { useLocation } from "react-router-dom"; import { modalStore } from "@/shared/model/store"; +import type { ModalItem } from "@/shared/model/store"; import { Modal } from "@/shared/ui/modal/modal"; -interface ModalItem { - id: string; - content: ReactNode; - autoPlay?: number; -} - export const ModalProvider = () => { const location = useLocation(); - const [modals, setModals] = useState<ModalItem[]>([]); - - useEffect(() => { - const unsubscribe = modalStore.subscribe(setModals); - return unsubscribe; - }, []); + const modals = useSyncExternalStore( + modalStore.subscribe.bind(modalStore), + () => modalStore.getSnapshot(), // store에 getSnapshot 메서드 추가 필요 + );이 변경을 적용하려면
modalStore에getSnapshot()메서드를 추가해야 해요:getSnapshot() { return this._modalList; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/providers/modal-provider.tsx` around lines 16 - 21, Replace the manual useState/useEffect subscription with React's useSyncExternalStore to avoid tearing: stop using useState/useEffect in modal-provider.tsx and instead call useSyncExternalStore(modalStore.subscribe, modalStore.getSnapshot) (or a wrapper getter) to read ModalItem[]; also add a getSnapshot method on modalStore that returns the current _modalList so subscribe and snapshot APIs match useSyncExternalStore expectations and preserve type ModalItem[].
8-12: 🛠️ Refactor suggestion | 🟠 Major
ModalItem타입이 store와 중복 정의되어 있어요.
src/shared/model/store/modal.store.ts의ModalItem과 거의 동일한 타입이 여기에도 정의되어 있어요. store에서 타입을 export해서 재사용하면 필드 변경 시 동기화 누락을 방지할 수 있어요. 특히 현재 provider의ModalItem에는onClose필드가 빠져 있어서 store 타입과 불일치해요.🔧 제안 코드
-import { modalStore } from "@/shared/model/store"; +import { modalStore, type ModalItem } from "@/shared/model/store"; -import { Modal } from "../../shared/ui/modal/modal"; - -interface ModalItem { - id: string; - content: ReactNode; - autoPlay?: number; -} +import { Modal } from "@/shared/ui/modal/modal";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/providers/modal-provider.tsx` around lines 8 - 12, The local ModalItem interface in modal-provider.tsx duplicates the ModalItem type defined in src/shared/model/store/modal.store.ts and misses the onClose field; remove the local interface and instead import and use the exported ModalItem type from the store (update any references in ModalProvider, useModal, or related functions to the imported ModalItem) so the provider and store share a single source of truth and stay in sync when fields (like onClose, autoPlay) change.src/shared/model/store/modal.store.ts (3)
13-13: 🧹 Nitpick | 🔵 Trivial브라우저 환경에서
NodeJS.Timeout타입은 호환성 문제가 있을 수 있어요.클라이언트 번들에서
setTimeout반환형은 DOM/lib 설정에 따라 달라질 수 있어서,ReturnType<typeof setTimeout>로 변경하면 런타임 환경에 독립적으로 안전해요.🔧 제안 코드
- private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리 + private _timers = new Map<string, ReturnType<typeof setTimeout>>(); // 타이머 관리🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` at line 13, The _timers map uses NodeJS.Timeout which can break in browser bundles; change its type to be environment-agnostic by replacing NodeJS.Timeout with ReturnType<typeof setTimeout> (update the declaration of private _timers = new Map<string, NodeJS.Timeout>() to private _timers = new Map<string, ReturnType<typeof setTimeout>>() and adjust any related typings/usages in Modal store methods like clearTimer/startTimer if present).
33-33:⚠️ Potential issue | 🟠 Major기본 모달 ID 생성 방식이 충돌 가능성이 있어요.
new Date().toString()은 밀리초 단위 정밀도가 없어서, 짧은 시간 안에 여러open()이 호출되면 동일한 ID가 생성될 수 있어요.crypto.randomUUID()또는 monotonic counter 사용을 권장해요.🔧 제안 코드
open( content: ReactNode, autoPlay?: number, onClose?: () => void, - id: string = new Date().toString() + id: string = crypto.randomUUID() ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` at line 33, The modal ID generation using id: string = new Date().toString() can produce collisions; update the ModalStore to use a collision-safe generator—replace the Date-based default and any ID assignment in methods like open() with crypto.randomUUID() (with a fallback to a module-level monotonic counter if crypto.randomUUID is unavailable) so each modal gets a unique ID; ensure the property name id and the open() method are updated to call the new generator.
74-81:⚠️ Potential issue | 🟠 Major
reset()이 개별 모달의onClose콜백을 호출하지 않아요.현재
reset()은 타이머 정리와 리스트 초기화만 수행하고, 각 모달에 등록된onClose콜백은 실행되지 않아요. 라우트 이동 시 cleanup 로직이 필요한 경우 문제가 될 수 있어요.🔧 제안 코드
reset() { // 예약된 타이머 제거 this._timers.forEach(clearTimeout); this._timers.clear(); // 메모리 참조 제거 + // 각 모달의 onClose 콜백 실행 + this._modalList.forEach((modal) => { + if (modal.onClose) modal.onClose(); + }); + this._modalList = []; this.notify(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 74 - 81, The reset() method currently clears timers and empties _modalList without invoking each modal's onClose callback; update reset() to iterate over this._modalList and call modal.onClose() (guarding existence and catching errors) for each modal before clearing timers and setting this._modalList = []; ensure you still clear this._timers and call this.notify() after callbacks so any modal cleanup runs and observers are informed; reference the reset(), _modalList, _timers, onClose, and notify() symbols when making the change.src/features/experience-matching/ui/select-company/select-company.tsx (1)
63-82:⚠️ Potential issue | 🟠 Major
handleSearch에 재진입 방지 로직이 필요해요.빠른 더블클릭 시
handleSearch가 여러 번 호출되면setCompany와onClick이 중복 실행될 수 있어요. pending 상태를 사용하여 한 번만 실행되도록 방어하는 것을 권장해요.🔧 제안 코드
+import { useEffect, useRef, useState } from "react"; -import { useEffect, useState } from "react"; // ... 컴포넌트 내부 + const isPendingRef = useRef(false); const handleSearch = () => { - if (!selectedCompany) return; + if (!selectedCompany || isPendingRef.current) return; + isPendingRef.current = true; + // 기업 선택 후, 대기하는 모달 modalStore.open( <> <Modal.Content type="auto"> <Modal.Title> {josa(selectedCompany.name, "을/를")} 선택하셨습니다 </Modal.Title> <Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle> </Modal.Content> <Modal.Image /> </>, 3000, () => { + isPendingRef.current = false; setCompany(selectedCompany); onClick(); } ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/experience-matching/ui/select-company/select-company.tsx` around lines 63 - 82, The handleSearch function needs a re-entrancy guard: add a local state flag (e.g., pendingSearch or isSearching) and check it at the start of handleSearch (return immediately if true), set it to true before calling modalStore.open, and reset it to false in the modal callback or after the 3000ms completes; ensure the guard references selectedCompany as before and only calls setCompany(selectedCompany) and onClick() once while pendingSearch is true to prevent double execution from rapid double-clicks.
🤖 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/features/experience-matching/ui/select-company/select-company.tsx`:
- Around line 66-81: The modal opened in select-company.tsx via
modalStore.open(...) uses the default ephemeral ID (new Date().toString()),
causing re-entry issues; update the call to pass a fixed, descriptive ID (e.g.
"SELECT-COMPANY-LOADING") to modalStore.open so the modal is identified
consistently and re-entry is prevented, keeping the existing content, duration
(3000) and completion callback that calls setCompany(selectedCompany) and
onClick(); locate the modalStore.open invocation in the select-company component
to make this change.
---
Duplicate comments:
In `@src/app/providers/modal-provider.tsx`:
- Line 6: The import for Modal uses a deep relative path; change it to the
project alias to match the rest of the file (e.g., use '@/shared/ui/modal/modal'
instead of '../../shared/ui/modal/modal') so imports are consistent with the
existing '@/shared/model/store' alias and improve maintainability; update the
import that references Modal accordingly.
- Around line 16-21: Replace the manual useState/useEffect subscription with
React's useSyncExternalStore to avoid tearing: stop using useState/useEffect in
modal-provider.tsx and instead call useSyncExternalStore(modalStore.subscribe,
modalStore.getSnapshot) (or a wrapper getter) to read ModalItem[]; also add a
getSnapshot method on modalStore that returns the current _modalList so
subscribe and snapshot APIs match useSyncExternalStore expectations and preserve
type ModalItem[].
- Around line 8-12: The local ModalItem interface in modal-provider.tsx
duplicates the ModalItem type defined in src/shared/model/store/modal.store.ts
and misses the onClose field; remove the local interface and instead import and
use the exported ModalItem type from the store (update any references in
ModalProvider, useModal, or related functions to the imported ModalItem) so the
provider and store share a single source of truth and stay in sync when fields
(like onClose, autoPlay) change.
In `@src/features/experience-matching/ui/select-company/select-company.tsx`:
- Around line 63-82: The handleSearch function needs a re-entrancy guard: add a
local state flag (e.g., pendingSearch or isSearching) and check it at the start
of handleSearch (return immediately if true), set it to true before calling
modalStore.open, and reset it to false in the modal callback or after the 3000ms
completes; ensure the guard references selectedCompany as before and only calls
setCompany(selectedCompany) and onClick() once while pendingSearch is true to
prevent double execution from rapid double-clicks.
In `@src/shared/model/store/modal.store.ts`:
- Line 13: The _timers map uses NodeJS.Timeout which can break in browser
bundles; change its type to be environment-agnostic by replacing NodeJS.Timeout
with ReturnType<typeof setTimeout> (update the declaration of private _timers =
new Map<string, NodeJS.Timeout>() to private _timers = new Map<string,
ReturnType<typeof setTimeout>>() and adjust any related typings/usages in Modal
store methods like clearTimer/startTimer if present).
- Line 33: The modal ID generation using id: string = new Date().toString() can
produce collisions; update the ModalStore to use a collision-safe
generator—replace the Date-based default and any ID assignment in methods like
open() with crypto.randomUUID() (with a fallback to a module-level monotonic
counter if crypto.randomUUID is unavailable) so each modal gets a unique ID;
ensure the property name id and the open() method are updated to call the new
generator.
- Around line 74-81: The reset() method currently clears timers and empties
_modalList without invoking each modal's onClose callback; update reset() to
iterate over this._modalList and call modal.onClose() (guarding existence and
catching errors) for each modal before clearing timers and setting
this._modalList = []; ensure you still clear this._timers and call this.notify()
after callbacks so any modal cleanup runs and observers are informed; reference
the reset(), _modalList, _timers, onClose, and notify() symbols when making the
change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5681075a-6782-41c9-b2a5-a307dcbca440
📒 Files selected for processing (3)
src/app/providers/modal-provider.tsxsrc/features/experience-matching/ui/select-company/select-company.tsxsrc/shared/model/store/modal.store.ts
Comment on lines
+66
to
+81
| modalStore.open( | ||
| <> | ||
| <Modal.Content type="auto"> | ||
| <Modal.Title> | ||
| {josa(selectedCompany.name, "을/를")} 선택하셨습니다 | ||
| </Modal.Title> | ||
| <Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle> | ||
| </Modal.Content> | ||
| <Modal.Image /> | ||
| </>, | ||
| 3000, | ||
| () => { | ||
| setCompany(selectedCompany); | ||
| onClick(); | ||
| } | ||
| } | ||
| prevIsOpen.current = isOpen; | ||
| }, [isOpen, selectedCompany, onClick, setCompany]); | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
기업 선택 모달에도 고정 ID 사용을 권장해요.
handleSearch에서 열리는 모달은 고정 ID가 없어서 new Date().toString() 기본값이 사용돼요. 재진입 방지와 함께 "SELECT-COMPANY-LOADING"과 같은 고정 ID를 사용하면 더 안정적이에요.
🔧 제안 코드
modalStore.open(
<>
<Modal.Content type="auto">
<Modal.Title>
{josa(selectedCompany.name, "을/를")} 선택하셨습니다
</Modal.Title>
<Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle>
</Modal.Content>
<Modal.Image />
</>,
3000,
() => {
setCompany(selectedCompany);
onClick();
- }
+ },
+ "SELECT-COMPANY-LOADING"
);📝 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
| modalStore.open( | |
| <> | |
| <Modal.Content type="auto"> | |
| <Modal.Title> | |
| {josa(selectedCompany.name, "을/를")} 선택하셨습니다 | |
| </Modal.Title> | |
| <Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle> | |
| </Modal.Content> | |
| <Modal.Image /> | |
| </>, | |
| 3000, | |
| () => { | |
| setCompany(selectedCompany); | |
| onClick(); | |
| } | |
| } | |
| prevIsOpen.current = isOpen; | |
| }, [isOpen, selectedCompany, onClick, setCompany]); | |
| ); | |
| modalStore.open( | |
| <> | |
| <Modal.Content type="auto"> | |
| <Modal.Title> | |
| {josa(selectedCompany.name, "을/를")} 선택하셨습니다 | |
| </Modal.Title> | |
| <Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle> | |
| </Modal.Content> | |
| <Modal.Image /> | |
| </>, | |
| 3000, | |
| () => { | |
| setCompany(selectedCompany); | |
| onClick(); | |
| }, | |
| "SELECT-COMPANY-LOADING" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/experience-matching/ui/select-company/select-company.tsx` around
lines 66 - 81, The modal opened in select-company.tsx via modalStore.open(...)
uses the default ephemeral ID (new Date().toString()), causing re-entry issues;
update the call to pass a fixed, descriptive ID (e.g. "SELECT-COMPANY-LOADING")
to modalStore.open so the modal is identified consistently and re-entry is
prevented, keeping the existing content, duration (3000) and completion callback
that calls setCompany(selectedCompany) and onClick(); locate the modalStore.open
invocation in the select-company component to make this change.
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
4 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.
✏️ Summary
📑 Tasks
기존의 구조는 비슷하지만 각각 다른 UI의 모달 컴포넌트을 쉽게 구성할 수 있도록 컴파운트 패턴을 활용하여 UI의 유연성을 높였습니다. 또한 모달이
open,close상태를 기준으로 열리고 닫힐 수 있도록use-modal훅을 분리하여 상태를 관리하도록 했습니다.하지만 실제 모달 컴포넌트를 코드에 사용하는 측면에 있어 두 가지 문제점이 있다고 생각했습니다.
만약 어떤 컴포넌트(
select-company.tsx)에서 사용하는 모달의 개수가 2가지 이상이 된다면, 각각의 모달의 상태를 위해각각
use-modal훅을 호출하여 각자의isOpen를 관리하도록 해야 했습니다. 한 가지 모달만 사용할 때도 무조건use-modal훅을 호출해야 한다는 사실도 DX 관점에서 좋지 않다는 생각이 들기도 했습니다.또한 JSX 코드에서 실제 컴포넌트 내용과는 약간 무관한, 동떨어져 있는 모달 코드를 JSX 코드에 함께 작성해야해 컴포넌트의 의미를 명확하게 하지 못하는 한계가 있다고 느껴졌습니다.
이러한 문제점을 해결하기 위해
observer pattern을 도입하기로 했습니다.observer pattern을 도입한 이유는 다음과 같습니다.ModalContainer한 곳으로 위임하여, 개별 컴포넌트의 JSX 구조를 본연의 역할에 맞게 간결하게 유지하고 관심사를 분리하는 것입니다.modalStore.open()과 같은 단순한 명령형 함수 호출만으로 쉽게 모달을 제어할 수 있어 개발 생산성을 높일 수 있습니다.observer class 구현 (
modal.store.ts)modalList 배열을 통해 모달 상태를 관리하는 순수 JS 클래스를 선언하고, 싱글톤 인스턴스로 export 하였습니다.
[ + 추가 수정사항 ]
현재 구현에서는
ModalProvider하나만subscribe하고 있기 때문에 실제 동작에는 문제가 없습니다. 다만ModalStore의subscribe()가 단일 콜백만 보관하는 구조라, 이후 다른 컴포넌트에서 동일한 store를 구독하게 될 경우 기존 subscriber가 덮어써지거나unsubscribe()호출 시 의도하지 않게 다른 subscriber까지 해제될 가능성이 있습니다.Publisher–Subscriber 관계를 1:1이 아닌 1:N 구조로 명확하게 관리하는 것이 더 안전하기 때문에 이를 위해 내부 listener를 하나의 콜백이 아닌 컬렉션으로 관리하고,
subscribe()가 해당 subscriber만 해제할 수 있는 cleanup 함수를 반환하는 형태로 수정하였습니다.모달을 보여줄 Provider 정의 (
modal-provider.ts)modalStore를 실질적으로 구독하여 전역 상태가 변경될 때마다 _modalList에 등록된 모달들을 렌더링하는 최상단 렌더링 컨테이너입니다. 특정 컴포넌트에서
modalStore.open()명령을 보내면, 상태가 변경되었음으로 판단하고 해당 모달을 보여주기 위해 렌더링되는 것입니다.컴포넌트는 오직 모달을 열어라라는 명령(
modalStore.open)만 내리기 때문에, 실제 UI 렌더링 책임은 전역 컨테이너로 넘어갔으며, 컴포넌트 내 JSX는 자신의 컴포넌트 내용 자체에만 집중할 수 있습니다.[추가적인 트러블 슈팅]
전역으로 모달을 분리하면서, 모달이 열린 상태에서 뒤로 가기를 누르거나 페이지를 이동할 경우 모달 자체의 상태가 변화한 것이 아니기 때문에
onClose이벤트가 발생하지 않아 이동 페이지에서도 모달이 계속 떠 있는 문제가 있었습니다.이를 위해
useLocation을 통해 pathname의 변경을 감지하고, 라우트가 바뀔 때modalStore.reset()을 호출하여 모달이 닫힐 수있도록 ModalProvider에 로직을 추가했습니다.👀 To Reviewer
use-modal은 이제 사용하지 않아 머지하기 전에 삭제할 예정입니다.use-modal이 사용하지 않게 됨에 따라경험 등록/보기페이지에서 사용되었던 훅을 제거하고modalStore의 메서드(open, reset)를 호출하여 사하도록 수정하였습니다.modal-provider.tsx에서 전담합니다. 따라서 주로 사용되던 모달 템플릿인modal-basic컴포넌트 내부에 선언되어 있던 래퍼 태그는 제거하였습니다.📸 Screenshot
🔔 ETC
es-hangul패키지를 설치하였습니다Summary by CodeRabbit