Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@vanilla-extract/recipes": "^0.5.7",
"@vanilla-extract/vite-plugin": "^5.1.4",
"axios": "^1.13.2",
"es-hangul": "^2.3.8",
"eslint-import-resolver-typescript": "^4.4.4",
"framer-motion": "^12.27.1",
"react": "^19.2.0",
Expand Down
8 changes: 8 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 43 additions & 0 deletions src/app/providers/modal-provider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { useEffect, useState, type ReactNode } from "react";
import { useLocation } from "react-router-dom";

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

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

interface ModalItem {
id: string;
content: ReactNode;
autoPlay?: number;
}

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

useEffect(() => {
const unsubscribe = modalStore.subscribe(setModals);
return unsubscribe;
}, []);
Comment on lines +16 to +21
Copy link
Copy Markdown

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.


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

Comment on lines +23 to +26
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

지금 reset 기준이 pathname만 보고 있어서 같은 페이지 안에서 query string이나 hash만 바뀌는 이동에서는 모달이 그대로 남을 수도 있을 것 같아요 ! 혹시 이 부분은 의도적으로 두신 건지 궁금합니다 ! 😉

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

지금 당장은 query를 통해 url이 바뀌는 페이지에서 모달이 사용되는 경우가 없어
주요하게 생각하지 않은 부분이었어요 히힣.
추후에 다른 페이지에서 모달이 쓰일 때 충분히 발생할 수 있는 이슈인 것 같아요. 해당 부분 관련해서 pathname이 아닌 location.key를 활용해 수정하도록 하겠습니다 !

return (
<>
{modals.map((modal) => {
return (
<Modal
key={modal.id}
isOpen={true}
autoPlay={modal.autoPlay}
onClose={() => modalStore.close(modal.id)}
>
{modal.content}
</Modal>
);
})}
</>
);
};
2 changes: 2 additions & 0 deletions src/app/routes/root-layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import { Outlet } from "react-router-dom";

import { Header } from "@widgets/index";

import { ModalProvider } from "../providers/modal-provider";
import { StoreResetListener } from "../providers/store-reset-listener";

export const RootLayout = () => {
return (
<>
<ModalProvider />
<StoreResetListener />
<Header />
<main>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { useEffect, useCallback, useRef } from "react";
import { useEffect, useCallback } from "react";
import { useBlocker } from "react-router-dom";

import { useModal } from "@/shared/ui/modal/use-modal";
import { modalStore } from "@/shared/model/store";
import { ModalBasic } from "@/shared/ui";

import {
initialDraft,
Expand All @@ -23,12 +24,12 @@ const isDraftDirty = (draft: ExperienceUpsertBody): boolean => {
);
};

const LEAVE_MODAL_ID = "leave-confirm-modal";

export const useLeaveConfirm = () => {
const mode = useExperienceDetailStore((s) => s.mode);
const draft = useExperienceDetailStore((s) => s.draft);

const { isOpen, openModal, closeModal } = useModal();

const shouldBlock =
(mode === "create" || mode === "edit") && isDraftDirty(draft);

Expand All @@ -46,17 +47,37 @@ export const useLeaveConfirm = () => {
return shouldBlockNow && !isSubmitting && !isTransitioning;
});

const prevBlockerStateRef = useRef(blocker.state);
const confirmLeave = useCallback(() => {
if (blocker.state === "blocked") {
blocker.proceed();
}
modalStore.close(LEAVE_MODAL_ID);
}, [blocker]);

useEffect(() => {
const wasBlocked = prevBlockerStateRef.current === "blocked";
const nowBlocked = blocker.state === "blocked";
prevBlockerStateRef.current = blocker.state;
const cancelLeave = useCallback(() => {
if (blocker.state === "blocked") {
blocker.reset();
}
modalStore.close(LEAVE_MODAL_ID);
}, [blocker]);

if (nowBlocked && !wasBlocked && !isOpen) {
openModal();
useEffect(() => {
if (blocker.state === "blocked") {
modalStore.open(
<ModalBasic
title={`작성중인 내용이 있습니다.\n정말 나가시겠습니까?`}
subTitle="저장하지 않으면 내용이 사라져요."
closeText="이어서 작성"
confirmText="나가기"
onClose={cancelLeave}
onConfirm={confirmLeave}
/>,
0,
undefined,
LEAVE_MODAL_ID
);
}
}, [blocker.state, isOpen, openModal]);
}, [blocker.state, cancelLeave, confirmLeave]);

useEffect(() => {
const handleBeforeUnload = (e: BeforeUnloadEvent) => {
Expand All @@ -70,23 +91,5 @@ export const useLeaveConfirm = () => {
return () => window.removeEventListener("beforeunload", handleBeforeUnload);
}, [shouldBlock]);

const confirmLeave = useCallback(() => {
if (blocker.state === "blocked") {
blocker.proceed();
}
closeModal();
}, [blocker, closeModal]);

const cancelLeave = useCallback(() => {
if (blocker.state === "blocked") {
blocker.reset();
}
closeModal();
}, [blocker, closeModal]);

return {
isOpen,
confirmLeave,
cancelLeave,
};
return {};
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

실제로 리팩토링된 부분을 보니 바로 이해가 되네요 !!! useModal 없이 모달을 관리할 수 있게 되면서, jsx 코드가 분리되고 별도의 선언 없이 store를 사용해 모달을 열 수 있는 점도 너무 좋습니다! 모달에 들어갈 내용들을 사용처에서 입력해주는 것도 응집도 측면에서 정말 좋은 거 같아요 천재 개발자 오수빈답습니다 👍👍

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

계속해서 모달에 대한 상태 관리가 use-modal로 인해 분산되어 있다는 생각을 해서 왜 인지 모르게 찜찜했는데
use-modal 없이 modal을 관리할 수 있게 되어서 햅삐하고, 거대칭찬을 받아서 또 햅삐하네요 //
부끄_하치와레

Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { EXPERIENCE_TYPE } from "@/shared/config/experience";
import { parseYMD } from "@/shared/lib/format-date";
import { modalStore } from "@/shared/model/store";
import { ModalBasic, Tooltip } from "@/shared/ui";
import { Button } from "@/shared/ui/button/button";
import { useModal } from "@/shared/ui/modal/use-modal";
import { Tag } from "@/shared/ui/tag/tag";
import { Textfield } from "@/shared/ui/textfield/textfield";
import { HELP_TOOLTIP_CONTENT } from "@/shared/ui/tooltip/tooltip.content";
Expand All @@ -24,9 +24,6 @@ const ExperienceViewer = () => {
const { showEditDelete, onClickEdit, onClickDelete, onToggleDefault } =
useExperienceHeaderActions();

const { isOpen: isDeleteModalOpen, handleModal: toggleDeleteModal } =
useModal();

const startDate = current?.startAt ? parseYMD(current.startAt) : null;
const endDate = current?.endAt ? parseYMD(current.endAt) : null;

Expand All @@ -42,6 +39,22 @@ const ExperienceViewer = () => {

const typeLabel = current.type ? EXPERIENCE_TYPE[current.type] : "미지정";

const handleOpenDeleteModal = () => {
modalStore.open(
<ModalBasic
title="이 경험을 삭제하시겠습니까?"
subTitle="작성한 내용은 즉시 제거되며, 복구할 수 없습니다."
closeText="취소"
confirmText="삭제"
onClose={() => modalStore.reset()} // 취소 시 닫기
Copy link
Copy Markdown
Collaborator

@u-zzn u-zzn Mar 16, 2026

Choose a reason for hiding this comment

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

현재 store는 modalList 기반으로 여러 모달을 관리할 수 있게 설계되어 있는 것 같은데, 여기서는 특정 삭제 확인 모달만 닫는 목적임에도 reset()으로 전체 모달을 비우고 있어요. 지금은 문제 없을 수 있지만, 이후 다른 전역 모달이 함께 열리는 경우 의도치 않게 전부 닫히는 부작용이 생길 수도 있을 것 같아요!

store에서 close(id)reset()의 책임이 분리되어 있는 만큼, 여기서는 reset()보다는 close(id)로 해당 모달만 닫는 쪽이 설계 의도와도 더 잘 맞고 안전할 것 같은데, 어떻게 생각하시나요? ☺️

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

말씀해주신대로 close(id)로 모달을 개별적으로 닫는 것이 안전한 방향이지만,
experience-viewer 경험/등록/삭제에서의 모달 관련한 수정 코드는 이어 해당 기능들이 리팩토링이 진행될 부분임을 감안해서 모달 동작이 정상적으로 이루어지게끔만 수정 코드를 최대한 줄여 임시적인 느낌으로 수정한 상태입니다! 해당 부분 관련해서는 리팩토링 하면서 함께 반영해주시면 좋을 것 같아요! 🫰🏻

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

넵 이해했습니다!! 말씀해주신 부분은 제가 추후 리팩토링 진행하면서 close(id) 기반으로 정리해보겠습니다. 감사합니다 🙂

onConfirm={() => {
onClickDelete(); // 실제 삭제 동작
modalStore.reset(); // 모달 닫기
}}
/>
);
Comment on lines +42 to +55
Copy link
Copy Markdown

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.

};

return (
<main className={s.page}>
<StickyHeader
Expand All @@ -53,7 +66,7 @@ const ExperienceViewer = () => {
<Button
variant="secondary"
size="small"
onClick={toggleDeleteModal}
onClick={handleOpenDeleteModal}
>
삭제하기
</Button>
Expand Down Expand Up @@ -130,19 +143,6 @@ const ExperienceViewer = () => {
</div>
</div>
</section>

<ModalBasic
title="이 경험을 삭제하시겠습니까?"
subTitle="작성한 내용은 즉시 제거되며, 복구할 수 없습니다."
closeText="취소"
confirmText="삭제"
isOpen={isDeleteModalOpen}
onClose={toggleDeleteModal}
onConfirm={() => {
toggleDeleteModal();
onClickDelete();
}}
/>
</main>
);
};
Expand Down
Loading
Loading