Skip to content

[Refactor] Observer pattern을 통한 전역 모달 관리#155

Open
odukong wants to merge 8 commits intodevfrom
refactor/#153/modal-refactor
Open

[Refactor] Observer pattern을 통한 전역 모달 관리#155
odukong wants to merge 8 commits intodevfrom
refactor/#153/modal-refactor

Conversation

@odukong
Copy link
Collaborator

@odukong odukong commented Mar 10, 2026

✏️ Summary

📑 Tasks

⁉️기존 구조에서 observer 패턴을 도입한 이유

기존의 구조는 비슷하지만 각각 다른 UI의 모달 컴포넌트을 쉽게 구성할 수 있도록 컴파운트 패턴을 활용하여 UI의 유연성을 높였습니다. 또한 모달이 open, close 상태를 기준으로 열리고 닫힐 수 있도록 use-modal 훅을 분리하여 상태를 관리하도록 했습니다.

하지만 실제 모달 컴포넌트를 코드에 사용하는 측면에 있어 두 가지 문제점이 있다고 생각했습니다.

만약 어떤 컴포넌트(select-company.tsx)에서 사용하는 모달의 개수가 2가지 이상이 된다면, 각각의 모달의 상태를 위해
각각 use-modal훅을 호출하여 각자의 isOpen를 관리하도록 해야 했습니다. 한 가지 모달만 사용할 때도 무조건 use-modal훅을 호출해야 한다는 사실도 DX 관점에서 좋지 않다는 생각이 들기도 했습니다.

// (이전) 모달 상태 관리 
const { autoPlay, isOpen, handleModal } = useModal(3000); // 기업 선택 이후 3초 후 넘어가게 하는 모달
const alertModal = useModal(); // 경험 등록 여부 확인 모달

또한 JSX 코드에서 실제 컴포넌트 내용과는 약간 무관한, 동떨어져 있는 모달 코드를 JSX 코드에 함께 작성해야해 컴포넌트의 의미를 명확하게 하지 못하는 한계가 있다고 느껴졌습니다.

// 반환부에 실질적인 컴포넌트 코드과 모달 코드가 섞여 있어 가독성을 해쳐 의미를 파악하기 어려움
return (
  <div className={styles.layout}>
    <h1 className={styles.title}>어떤 기업을 분석할까요?</h1>
    <MatchingAutoComplete
      value={inputValue}
      onChange={setInputValue}
      results={searchResults}
      onDebounceChange={setSearchKeyword}
      selectedItem={selectedCompany}
      onSelect={setSelectedCompany}
      onSearch={handleModal}
      onSearch={handleSearch}
    />
    {/** 경험 등록 여부 확인 모달 */}
    <Modal isOpen={alertModal.isOpen} onClose={alertModal.closeModal}>
    </Modal>
    {/** 기업 선택 후, 대기 모달 */}
    <Modal autoPlay={autoPlay} isOpen={isOpen} onClose={handleModal}>
    </Modal>
  </div>
);

이러한 문제점을 해결하기 위해 observer pattern을 도입하기로 했습니다.

observer pattern을 도입한 이유는 다음과 같습니다.

  • 가장 큰 목적은 애플리케이션 전체에서 쓰이는 공통 UI인 모달의 렌더링 책임을 전역 ModalContainer 한 곳으로 위임하여, 개별 컴포넌트의 JSX 구조를 본연의 역할에 맞게 간결하게 유지하고 관심사를 분리하는 것입니다.
  • modalStore.open()과 같은 단순한 명령형 함수 호출만으로 쉽게 모달을 제어할 수 있어 개발 생산성을 높일 수 있습니다.
  • 특히, 전역 상태 라이브러리의 도입을 최소화하려는 팀의 지향점에 맞춰, Context API의 리렌더링 문제나 외부 라이브러리 의존 없이 순수 자바스크립트 클래스만으로 가볍게 전역 상태를 관리할 수 있다는 점을 중요하게 생각했습니다.

observer class 구현 (modal.store.ts)

modalList 배열을 통해 모달 상태를 관리하는 순수 JS 클래스를 선언하고, 싱글톤 인스턴스로 export 하였습니다.

import type { ReactNode } from "react";

class ModalStore {
  private _modalList: ModalItem[] = []; // 모달 리스트 관리
  private _listeners = new Set<(list: ModalItem[]) => void>();
  private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리

  private notify() {
    this._listeners.forEach((listener) => {
      listener(this._modalList);
    });
  }

  subscribe(callback: (list: ModalItem[]) => void) {
    this._listeners.add(callback); // 모달 리스트 상태 업데이트 함수 등록

    return () => {
      this._listeners.delete(callback);
    };
  }

  open(
    content: ReactNode,
    autoPlay?: number,
    onClose?: () => void,
    id: string = new Date().toString()
  ) {
      // 모달을 open하는 메서드
  }

  close(id: string) {
     // 특정 모달만 리셋하는 메서드
  }

  reset() {
      // 모든 모달을 리셋하는 메서드
  }
}

export const modalStore = new ModalStore();

[ + 추가 수정사항 ]
현재 구현에서는 ModalProvider 하나만 subscribe하고 있기 때문에 실제 동작에는 문제가 없습니다. 다만 ModalStoresubscribe()가 단일 콜백만 보관하는 구조라, 이후 다른 컴포넌트에서 동일한 store를 구독하게 될 경우 기존 subscriber가 덮어써지거나 unsubscribe() 호출 시 의도하지 않게 다른 subscriber까지 해제될 가능성이 있습니다.

Publisher–Subscriber 관계를 1:1이 아닌 1:N 구조로 명확하게 관리하는 것이 더 안전하기 때문에 이를 위해 내부 listener를 하나의 콜백이 아닌 컬렉션으로 관리하고, subscribe()가 해당 subscriber만 해제할 수 있는 cleanup 함수를 반환하는 형태로 수정하였습니다.

모달을 보여줄 Provider 정의 (modal-provider.ts)

modalStore를 실질적으로 구독하여 전역 상태가 변경될 때마다 _modalList에 등록된 모달들을 렌더링하는 최상단 렌더링 컨테이너입니다. 특정 컴포넌트에서 modalStore.open() 명령을 보내면, 상태가 변경되었음으로 판단하고 해당 모달을 보여주기 위해 렌더링되는 것입니다.

export const ModalProvider = () => {
  const { pathname } = useLocation();
  const [modals, setModals] = useState<ModalItem[]>([]);

  useEffect(() => {
    modalStore.subscribe(setModals);            // modal-provider가 modal-store에 대한 상태를 구독합니다.
    return () => modalStore.unsubscribe();
  }, []);

  useEffect(() => {
    modalStore.reset();
  }, [pathname]);

  return (
     // 구독한 store의 상태가 변경되면 리렌더링을 진행
    <>
      {modals.map((modal) => {
        return (
          <Modal
            key={modal.id}
            isOpen={true}
            autoPlay={modal.autoPlay}
            onClose={() => modalStore.close(modal.id)}
          >
            {modal.content}
          </Modal>
        );
      })}
    </>
  );
};

컴포넌트는 오직 모달을 열어라라는 명령(modalStore.open)만 내리기 때문에, 실제 UI 렌더링 책임은 전역 컨테이너로 넘어갔으며, 컴포넌트 내 JSX는 자신의 컴포넌트 내용 자체에만 집중할 수 있습니다.

// 경험 등록 여부 확인 모달 (모달 관련 코드가 JSX코드 밖으로 이동)
useEffect(() => {
  if (data?.totalElements === 0) {
    modalStore.open(
      <>
        <Modal.Content>
          <Modal.Title>아직 등록된 경험이 없습니다</Modal.Title>
          <Modal.SubTitle>지금 바로 경험을 등록하러 가볼까요?</Modal.SubTitle>
        </Modal.Content>
        <Modal.Buttons>
          <Button variant="secondary" onClick={() => navigate(ROUTES.HOME)}>
            나가기
          </Button>
          <Button
            variant="primary"
            onClick={() => navigate(ROUTES.EXPERIENCE_CREATE)}
          >
            이동하기
          </Button>
        </Modal.Buttons>
      </>
    );
  }
}, [data, navigate]);

[추가적인 트러블 슈팅]
전역으로 모달을 분리하면서, 모달이 열린 상태에서 뒤로 가기를 누르거나 페이지를 이동할 경우 모달 자체의 상태가 변화한 것이 아니기 때문에 onClose 이벤트가 발생하지 않아 이동 페이지에서도 모달이 계속 떠 있는 문제가 있었습니다.

export const ModalProvider = () => {
  const { pathname } = useLocation();
  const [modals, setModals] = useState<ModalItem[]>([]);

  useEffect(() => {
    const unsubscribe = modalStore.subscribe(setModals);
    return unsubscribe;
  }, []);

  // pathname이 변경됨을 감지해 모달이 초기화되도록 함.
  useEffect(() => {
    modalStore.reset();
  }, [pathname]);

이를 위해 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

  • 새로운 기능
    • 전역 모달 제공자 도입으로 앱 전체에서 중앙화된 모달 관리가 가능합니다.
  • 수정/개선
    • 삭제 확인, 이탈 확인, 선택 후 로딩 등 개별 인라인 모달들이 중앙 모달 시스템으로 통합되어 작동이 표준화되었습니다.
  • 변경
    • 작업 설명 입력 필드의 최대 길이가 300자에서 500자로 늘어났습니다.

odukong added 5 commits March 7, 2026 21:43
- 기존 useModal 훅 기반의 선언적 구조에서 modalStore 기반의 명령형 구조로 전환
- observer Pattern을 활용하여 컴포넌트 내부 JSX와 모달 렌더링 로직의 결합도를 분리
- 수동 닫기(close) 및 페이지 이동(reset) 시 예약된 setTimeout을 명시적으로 클린업하여 사이드 이펙트를 방지합니다.
- 전역 ModalProvider 도입에 따라 ModalBasic 내부의 <Modal> 컨테이너 태그 제거
- Modal, ModalBasic을 사용하는 관련 컴포넌트 일괄 수정
@odukong odukong linked an issue Mar 10, 2026 that may be closed by this pull request
3 tasks
@github-actions github-actions bot added 🔗API api 연동 🛠️REFACTOR 코드 리팩토링 수빈🍋 labels Mar 10, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

애플리케이션 전반의 모달 관리를 로컬 훅에서 중앙 집중식 modalStore로 마이그레이션하고, 이를 렌더링/리셋하는 ModalProvider를 도입해 기존 모달 사용 지점을 store 기반으로 전환합니다.

Changes

Cohort / File(s) Summary
모달 스토어 인프라
src/shared/model/store/modal.store.ts, src/shared/model/store/index.ts
새로운 ModalStore 및 싱글턴 modalStore 추가: subscribe/unsubscribe, open/close/reset, per-id autoPlay 타이머, 리스너 알림 로직 및 store 재수출 추가.
ModalProvider 및 통합
src/app/providers/modal-provider.tsx, src/app/routes/root-layout.tsx
ModalProvider 컴포넌트 추가(경로 변경 시 modalStore.reset() 호출, store 구독으로 모달 리스트 렌더링) 및 RootLayout에 삽입.
경험 상세: 이탈/삭제 모달 전환
src/features/experience-detail/model/use-leave-confirm.tsx, src/features/experience-detail/ui/experience-viewer/experience-viewer.tsx, src/pages/experience-detail/experience-detail-page.tsx
로컬 useModal 제거, 이탈/삭제 확인 모달을 modalStore.open/close로 전환. useLeaveConfirm의 공개 반환값 제거 및 호출부 정리.
경험 매칭: 회사 선택 흐름 변경
src/features/experience-matching/ui/select-company/select-company.tsx
기존 모달 훅 제거 후 modalStore 기반 모달로 통합(선택 후 로딩/알림 등). es-hangul 사용 위해 import 추가, 검색/선택 흐름에 useGetCompanyList 도입.
모달 컴포넌트 변경
src/shared/ui/modal/modal-basic.tsx
ModalBasic에서 isOpen prop 제거 및 내부 렌더링을 Modal 래퍼 대신 프래그먼트로 변경(표출 제어를 store에 위임).
기타 사소 변경
src/shared/ui/textfield/textfield.tsx, package.json
jobDescription maxLength 300→500 증가. package.jsones-hangul 의존성 추가.

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, 시작 타이머(선택적)
    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: 제거된 모달 반영
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

제안 라벨

🌟FEAT

제안 리뷰어

  • qowjdals23
  • u-zzn
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed PR 제목은 observer pattern을 통한 전역 모달 관리 리팩토링이라는 주요 변경사항을 명확하게 요약하고 있습니다.
Description check ✅ Passed PR 설명은 템플릿의 주요 섹션(Summary, Tasks, To Reviewer)을 포함하고 있으며 충분한 맥락과 구현 세부사항을 제공합니다.
Linked Issues check ✅ Passed 전역 모달 상태 관리 [#153], 모달 코드 리팩토링, JD 글자수 제한 500자 변경 등 모든 목표가 완료되었습니다.
Out of Scope Changes check ✅ Passed ES-hangul 패키지 추가를 제외한 모든 변경사항이 observer pattern 모달 시스템 도입과 이에 따른 리팩토링과 관련되어 있습니다.
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.

📝 Coding Plan for PR comments
  • Generate 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Mar 10, 2026

🚀 빌드 결과

린트 검사 완료
빌드 성공

로그 확인하기
Actions 탭에서 자세히 보기

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

X 버튼이 onClose 액션을 타지 않습니다.

지금 구조에서는 Modal.XButtonModalProvideronClose만 호출하고, 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 | 🟠 Major

Props 타입을 별도로 정의하는 것을 권장해요.

코딩 가이드라인에 따르면 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

📥 Commits

Reviewing files that changed from the base of the PR and between cf791e2 and 5988b12.

📒 Files selected for processing (10)
  • src/app/providers/modal-provider.tsx
  • src/app/routes/root-layout.tsx
  • src/features/experience-detail/model/use-leave-confirm.tsx
  • src/features/experience-detail/ui/experience-viewer/experience-viewer.tsx
  • src/features/experience-matching/ui/select-company/select-company.tsx
  • src/pages/experience-detail/experience-detail-page.tsx
  • src/shared/model/store/index.ts
  • src/shared/model/store/modal.store.ts
  • src/shared/ui/modal/modal-basic.tsx
  • src/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(); // 모달 닫기
}}
/>
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

이 확인창에서 reset()을 쓰면 다른 전역 모달까지 같이 닫힙니다.

onCloseonConfirm이 모두 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.

Comment on lines +23 to +30
open(
content: ReactNode,
autoPlay?: number,
onClose?: () => void,
id: string = new Date().toString()
) {
const new_modal = { id: id, content: content, autoPlay, onClose };
this._modalList = [...this._modalList, new_modal];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

기본 모달 ID가 충돌할 수 있습니다.

new Date().toString()는 충돌 없는 식별자가 아니어서 짧은 시간 안에 여러 open()이 호출되면 같은 ID가 나올 수 있습니다. 그러면 close()/타이머 정리가 엉키고, filter()가 여러 모달을 한 번에 지울 수 있습니다.

🤖 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() 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).

odukong added 2 commits March 12, 2026 22:50
- 동적변수명 (한글/영어) 뒤에 오는 조사를 구분하여 붙이기 위한 es-hangul 패키지 설치
- 상수(LEAVE_MODAL_ID) 훅 외부로 이동
- confirmLeave 모달에서 명시적으로 모달이 닫히도록 modal.close() 호출
- modalItem interface 변경
- reset 메서드에서 예약된 타이머도 함께 제거
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5988b12 and da52f43.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml, !pnpm-lock.yaml and included by none
📒 Files selected for processing (4)
  • package.json
  • src/features/experience-detail/model/use-leave-confirm.tsx
  • src/features/experience-matching/ui/select-company/select-company.tsx
  • src/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]);
);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

이 플로우는 재진입 방지가 필요합니다.

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 패턴 구조 명확화
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between da52f43 and 509a915.

📒 Files selected for processing (2)
  • src/app/providers/modal-provider.tsx
  • src/shared/model/store/modal.store.ts


import { modalStore } from "@/shared/model/store";

import { Modal } from "../../shared/ui/modal/modal";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

깊은 상대 경로 import는 alias로 통일해주세요.

이 파일은 Line 4에서 이미 alias import를 쓰고 있는데, ../../shared/ui/modal/modal만 상대 경로라 이동/리팩터링에 더 취약합니다. @/shared/ui/modal/modal로 맞추는 편이 일관성과 유지보수성에 좋습니다. As per coding guidelines, "@shared, @features, @widgets 등 alias 기반 절대 경로 import 사용 권장" and "깊은 상대 경로(../../..) 사용은 가독성과 유지보수 측면에서 지양".

🤖 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 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).

Comment on lines +8 to +12
interface ModalItem {
id: string;
content: ReactNode;
autoPlay?: number;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

store의 ModalItem 타입을 재사용하는 편이 안전합니다.

여기 타입이 src/shared/model/store/modal.store.ts:3-8와 거의 동일해서, 필드가 추가되거나 바뀌면 provider 쪽만 조용히 drift할 수 있습니다. store에서 타입을 export하거나 별도 shared type으로 분리해 한 군데서 관리하는 편이 좋겠습니다. As per coding guidelines, "중복 코드 제거 및 재사용 가능한 구조 제안".

🤖 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 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.

Comment on lines +16 to +21
const [modals, setModals] = useState<ModalItem[]>([]);

useEffect(() => {
const unsubscribe = modalStore.subscribe(setModals);
return unsubscribe;
}, []);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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>(); // 타이머 관리
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Comment on lines +35 to +36
const new_modal = { id: id, content: content, autoPlay, onClose };
this._modalList = [...this._modalList, new_modal];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

변수명은 camelCase로 맞춰주세요.

new_modal은 이 파일의 다른 식별자와 컨벤션이 달라서 newModal이 더 일관적입니다. 객체 리터럴도 shorthand로 함께 정리할 수 있습니다. As per coding guidelines, "Use camelCase for variable names".

🤖 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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

수빈🍋 🔗API api 연동 🛠️REFACTOR 코드 리팩토링

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Refactor] 모달 상태관리 리팩토링

1 participant