Renew onboarding #57
Renew onboarding #57
Conversation
포크한 레포에 작동 테스틀 위해 연결 스토리북 형태도 세팅함
구조 수정 및 tailwind prefix에 맞게 mapping
이전 패키지 버전에선 빌드가 안 됐음.
새로고침 막고, mutate처리 및 뒤로가기 정상화
todo 개수 0이 아닌 경우에 동작하도록 수정
- 게스트 사용자는 목표 생성 API 호출 생략 - 게스트 사용자의 온보딩 완료 후 리디렉션 로직 수정 - TODO 주석 제거 및 코드 정리
- Figma 디자인 기반 SubGoalSelectionScreen 컴포넌트 구현 - 세부 목표 추가하기 버튼과 기본함 표시 - 온보딩 플로우에 새로운 단계 추가 (기간 선택 → 세부 목표 → 완료) - 나중에 설정하기 옵션 제공
- 기존 SubGoalEdit 컴포넌트의 ModalAddingSubGoal 재사용 - 온보딩 스토어에 세부 목표 상태 및 액션 추가 - SubGoalSelectionScreen에서 세부 목표 추가/삭제 기능 구현 - PeriodSelectionScreen에서 목표 생성 시 세부 목표 전달 - 코드 중복 제거 및 일관된 UX 제공
WalkthroughAdds guest-mode support and MSW lifecycle control, introduces sub-goal management for onboarding with a new SubGoalSelection step, updates auth and onboarding stores to expose Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant AuthStore as Auth Store
participant GuestHandler as GuestModeHandler
participant MSW as mocks/index
Note over AuthStore: app may start with isGuest == true
App->>GuestHandler: render
GuestHandler->>AuthStore: read isGuest, isLoggedIn
alt isGuest == true
GuestHandler->>MSW: initMsw() (via MSWComponent)
MSW-->>GuestHandler: running
else Transition to auth (!isGuest && isLoggedIn)
GuestHandler->>GuestHandler: wait 100ms
GuestHandler->>MSW: stopMsw()
MSW-->>GuestHandler: stopped
GuestHandler->>App: resetOnboarding()
end
sequenceDiagram
autonumber
actor User
participant Onboarding as SubGoalSelection
participant Store as Onboarding Store
participant Auth as Auth Store
participant API as goalApi
User->>Onboarding: open SubGoalSelection
Onboarding->>Store: ensure single default "기본함"
User->>Onboarding: add/edit/delete/reorder sub-goals
Onboarding->>Store: update subGoals (order/title)
User->>Onboarding: click Next
Onboarding->>Auth: read isGuest
alt isGuest == true
Onboarding->>Onboarding: skip API, call onNext
else
Onboarding->>API: createGoal({..., subGoals: [{title,order},...]})
API-->>Onboarding: response
Onboarding-->>User: onNext or error handling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
- OnboardingSubGoalEdit 컴포넌트로 기존 SubGoalEdit 로직 재사용 - 커스텀 서브골 리스트 구현 제거하고 검증된 컴포넌트 활용 - 동일한 UX: 추가/편집/삭제/드래그 앤 드롭 기능 제공 - 코드 중복 제거 및 유지보수성 향상
- 서브골이 없으면 "나중에 설정하기" 표시 - 서브골이 있으면 "다음" 표시하여 포함하여 진행 - 사용자 의도를 명확하게 전달하는 UX 개선
세부 목표가 있을 때 goalApi.createGoal을 호출하여 실제 목표와 세부 목표를 생성하도록 구현
- 목표 생성 핸들러에서 세부 목표 개별 생성 및 관계 설정 구현 - IndexedDB 스토어명 수정 (sub-goals → subGoals, todo-results → todoResults) - 누락된 스토어 추가 (groupMembers, groupMessages, reactions) - 자동 증가 ID 제거하고 문자열 ID 사용하도록 수정 - DB 버전 업그레이드로 스키마 변경 반영
- 온보딩 시 기본 세부 목표 '기본함' 자동 생성 - '기본함' 세부 목표의 편집/삭제 기능 비활성화 - 스토어 레벨에서 중복 '기본함' 생성 방지 - 컴포넌트 초기화 상태 관리로 중복 실행 방지 - 화면 레벨 중복 정리 로직 추가 - SubGoalEditItem 컴포넌트 편집/삭제 버튼 조건부 렌더링 - tmpKey 고유성 보장으로 React 키 충돌 해결
- 게스트 모드에서 월 단위 목표의 dueDate 계산 로직 추가 - 목표 목록 API에서 isPeriodByMonth 기반 dueDate 자동 계산 - 목표 상세 API에서 GoalDueDateRs 형식으로 응답 구조화 - 메인 페이지 서비스 이용일수 계산 시 createdAt 누락 처리 - 게스트 사용자 초기 데이터에 createdAt 필드 추가 - calcLeftDay 함수 호출 시 유효하지 않은 날짜 방지 - IndexedDB 스키마 버전 업그레이드로 변경사항 반영
- MSW 상태 추적으로 중복 start/stop 호출 방지 - 게스트 모드 전환 시 적절한 cleanup 타이밍 구현 - MSWComponent 단순화로 중복 cleanup 제거 - 인증 상태 변경 시 게스트 모드 자동 비활성화 - 100ms 지연으로 상태 안정화 후 cleanup 실행 - 무한 로딩 및 MSW 경고 메시지 해결 - 게스트 데이터 정리로 인증된 사용자 혼란 방지
- DBGoal 인터페이스에 isPeriodByMonth, month 프로퍼티 추가 - TodoResult 생성 시 id 필드 추가로 dbService.add 호환성 확보 - npm run build 성공적으로 완료되도록 타입 불일치 문제 해결
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
components/group/groupRoom/GroupChatRoom/GroupChatRoom.tsx (1)
68-73: Potential runtime error in dependency: unsafe optional chaining before deep property access
data?.[data?.length - 1].messages[...]can evaluate toundefined.messageswhendatais nullish, causing a crash. Compute the last message id defensively, then depend on that scalar.Type-safe approach:
import { useMemo } from "react"; const lastMessageId = useMemo(() => { if (!data || data.length === 0) return undefined; const lastPage = data[data.length - 1]; if (!lastPage?.messages || lastPage.messages.length === 0) return undefined; const lastMsg = lastPage.messages[lastPage.messages.length - 1]; return lastMsg?.messageId; }, [data]); useEffect(() => { if (lastMessageRef.current) { lastMessageRef.current.scrollIntoView({ behavior: "smooth" }); } }, [lastMessageId]);components/shared/BottomSheets/TodoBottomSheet/TodoBottomSheet.tsx (1)
33-38: Fix type: id should reference TodoInfoForSubmission['id']You currently alias id to the type of todo. It’s the same now (string) but this will bite later if id’s type changes.
-type TodoInfo = { - id?: TodoInfoForSubmission["todo"]; +type TodoInfo = { + id?: TodoInfoForSubmission["id"]; todo: TodoInfoForSubmission["todo"]; date?: TodoInfoForSubmission["date"]; subGoalTitle?: TodoInfoForSubmission["subGoalTitle"]; subGoalId?: TodoInfoForSubmission["subGoalId"]; };components/group/GroupChatItem.tsx (1)
224-233: Broken Tailwind class: “overflow-hidden” is split across lines and not applied.This currently renders as two invalid classes “overflow” and “-hidden”, so overflow isn’t clipped.
- <div - className="pl-4 pr-3 py-2 relative bg-background-assistive rounded-lg inline-flex flex-col justify-center items-start gap-2 overflow - -hidden" - > + <div + className="pl-4 pr-3 py-2 relative bg-background-assistive rounded-lg inline-flex flex-col justify-center items-start gap-2 overflow-hidden" + >app/onboarding/_components/LoginScreen.tsx (1)
51-58: Sensitive tokens are logged to the console — remove or gate by envAccess/refresh tokens printed in logs are PII/secrets and must not appear in production logs.
Harden by gating all debug logs behind a dev check, or remove them entirely:
const isDev = process.env.NODE_ENV !== "production"; if (isDev) { console.log("URL 파라미터들:", { code, error, token, state, accessTokenFromUrl, refreshTokenFromUrl, }); } // ... if (isDev) { const authState = useAuthStore.getState(); console.log("현재 auth store 상태:", { isLoggedIn: authState.isLoggedIn, oauthCode: authState.oauthCode, accessToken: authState.accessToken, refreshToken: authState.refreshToken, }); }Also consider never logging raw token values; at most log presence/length or a redacted prefix.
Also applies to: 112-118
app/onboarding/_components/PeriodSelectionScreen.tsx (1)
59-61: Calendar header (Mon–Sun) is misaligned with JS getDay() (Sun–Sat) — first day offset bugYour weekday header starts with Monday, but getDay() returns 0 for Sunday. The current offset computation for the leading "previous month" days will render under the wrong columns. Return a Monday-based index.
Apply this diff:
- const getFirstDayOfMonth = (month: number, year: number) => { - return new Date(year, month, 1).getDay(); - }; + // Return Monday-based index (0=Mon..6=Sun) to align with the ["월","화","수","목","금","토","일"] header + const getFirstDayOfMonth = (month: number, year: number) => { + const day = new Date(year, month, 1).getDay(); // 0=Sun..6=Sat + return (day + 6) % 7; // shift so Mon=0 + };components/main/GoalCard/GoalCard.tsx (1)
46-64: Guard spreading potentially undefined data (rawGoalData)
{ ...rawGoalData, ... }will throw ifrawGoalDataisundefined(possible before fetch resolves or ifinitSubGoalTodois absent). Wrap with a safe default.- const goalWithSubGoalTodo: ConvertedGoalWithSubGoalTodo = { - ...rawGoalData, + const goalWithSubGoalTodo: ConvertedGoalWithSubGoalTodo = { + ...(rawGoalData ?? {}), subGoals: rawGoalData?.subGoals?.map((subGoalInfo) => { // ... }) ?? [], } as ConvertedGoalWithSubGoalTodo;
| const debounceer = <T, E>(apiRequest: typeof httpClient.request<T, E>) => { | ||
| const timeLimit = 1000; | ||
| let timer: number; | ||
| let rejectTimer: (reason?: any) => void; | ||
| return ( | ||
| requestParams: Parameters<typeof httpClient.request<T, E>>[0], | ||
| ): ReturnType<typeof httpClient.request<T>> => { | ||
| if (timer) { | ||
| clearTimeout(timer); | ||
| rejectTimer("debouncing"); | ||
| } | ||
| const apiRes: Promise<T> = new Promise((resolve, reject) => { | ||
| rejectTimer = reject; | ||
| timer = Number( | ||
| setTimeout(async () => { | ||
| try { | ||
| const res = apiRequest(requestParams); | ||
| resolve(res); | ||
| } catch (error) { | ||
| console.error(error); | ||
| showToast(`API ERROR`, new Date()); | ||
| } | ||
| }, timeLimit), | ||
| ); | ||
| }); | ||
| return apiRes; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Global request debouncing will cancel unrelated API calls and introduces typing/errors
There are multiple high-impact issues here:
- Functionality: A single timer shared across all requests means any request within 1s cancels the previous one, even if they are for different endpoints/components. This will cause flaky UI and lost requests.
- Error handling: You call apiRequest without await; async errors won’t be caught by the try/catch, and you don’t reject in the catch, potentially leaving Promises hanging.
- Types: typeof httpClient.request<T, E> is invalid. You cannot apply generics to typeof in that way; this won’t type-check reliably.
- Timer typing: number is brittle across Node/DOM. Prefer ReturnType.
- Naming: Typo “debounceer”.
Recommend a keyed debouncer (e.g., method+path) and only debounce idempotent GETs. Also fix typing and error handling and ensure we don’t lose the instance context.
Apply this refactor within this block:
-// Debouncer 감싸도 될 것 같은데?
-const debounceer = <T, E>(apiRequest: typeof httpClient.request<T, E>) => {
- const timeLimit = 1000;
- let timer: number;
- let rejectTimer: (reason?: any) => void;
- return (
- requestParams: Parameters<typeof httpClient.request<T, E>>[0],
- ): ReturnType<typeof httpClient.request<T>> => {
- if (timer) {
- clearTimeout(timer);
- rejectTimer("debouncing");
- }
- const apiRes: Promise<T> = new Promise((resolve, reject) => {
- rejectTimer = reject;
- timer = Number(
- setTimeout(async () => {
- try {
- const res = apiRequest(requestParams);
- resolve(res);
- } catch (error) {
- console.error(error);
- showToast(`API ERROR`, new Date());
- }
- }, timeLimit),
- );
- });
- return apiRes;
- };
-};
+// Request debouncer: debounce by request key (method + path) and only for GET requests
+type RequestParams = Parameters<HttpClient["request"]>[0];
+type RequestReturn = ReturnType<HttpClient["request"]>;
+
+const createRequestDebouncer = (apiRequest: (p: RequestParams) => RequestReturn) => {
+ const timeLimit = 1000;
+ const pending = new Map<
+ string,
+ { timer: ReturnType<typeof setTimeout>; reject?: (reason?: any) => void }
+ >();
+
+ const makeKey = (p: RequestParams) => {
+ const method = (p?.method ?? "GET").toUpperCase();
+ // Keep key conservative: method + path; optionally include a stable query string if needed
+ return `${method}:${p?.path ?? ""}`;
+ };
+
+ return function debouncedRequest(params: RequestParams): RequestReturn {
+ const method = (params?.method ?? "GET").toUpperCase();
+ // Only debounce safe/idempotent reads
+ if (method !== "GET") {
+ return apiRequest(params);
+ }
+
+ const key = makeKey(params);
+ const prev = pending.get(key);
+ if (prev) {
+ clearTimeout(prev.timer);
+ prev.reject?.(new Error("debouncing"));
+ }
+
+ return new Promise((resolve, reject) => {
+ const timer = setTimeout(async () => {
+ try {
+ const res = await apiRequest(params);
+ resolve(res);
+ } catch (error) {
+ console.error(error);
+ showToast("API ERROR", new Date());
+ reject(error);
+ } finally {
+ pending.delete(key);
+ }
+ }, timeLimit);
+
+ pending.set(key, { timer, reject });
+ }) as unknown as RequestReturn;
+ };
+};🤖 Prompt for AI Agents
In api/service.ts around lines 35–62, the debouncer shares a single timer and
has typing/behavior bugs; replace it with a keyed debouncer (key = method+url or
other relevant id) so timers/rejectors are per-request key and only debounce
idempotent GETs (or caller-specified safe methods); fix the function signature
to accept a properly typed apiRequest function (use generics on the debouncer
itself and type apiRequest as a function returning Promise<T> rather than typeof
with generics), await apiRequest inside the setTimeout callback so errors are
caught, always call reject(...) on error and resolve(...) with the awaited
result, use ReturnType<typeof setTimeout> for timer typing, ensure the
apiRequest is called with the correct this/context (bind or arrow) and clear
timers/rejectors after settle, and rename the function to debounce (remove
typo).
| <Drawer.Root | ||
| key={`${ContentComponent}`} | ||
| open={isBottomSheetOpen} | ||
| defaultOpen={false} | ||
| handleOnly | ||
| disablePreventScroll={isBottomSheetOpen && !hasBackdrop} | ||
| > |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Vaul props: handleOnly and disablePreventScroll.
These are not standard across all Vaul versions. Ensure they’re supported and doing what you expect. If unsupported, they’ll be ignored silently.
🌐 Web query:
Does Vaul Drawer.Root support the props `handleOnly` and `disablePreventScroll`? If yes, from which version, and what are their effects?
💡 Result:
Vaul's <Drawer.Root> component does not currently support the props handleOnly or disablePreventScroll according to the official API documentation and all available change logs and references as of August 2025[1][10][8].
Details:
-
Props available on
<Drawer.Root>(as documented in the official API and on npm) include:opendefaultOpenonOpenChangedismissiblemodalsnapPointsonAnimationEnd- Others related to snap and drawer behavior[1][10].
-
handleOnly:
- There is no documented prop named
handleOnlyon<Drawer.Root>or in the listed API properties[1][10]. - You can, however, add a drag handle using the
<Drawer.Handle />component as a child of<Drawer.Content>, which provides a draggable element for users, but there is no prop to restrict dragging “only” via the handle[1].
- There is no documented prop named
-
disablePreventScroll:
- There is no documented prop named
disablePreventScroll, nor is there any mention of controlling page scroll prevention directly through a prop on<Drawer.Root>[1][10]. - Scroll locking is managed internally by Vaul when the drawer is open, as is common with modal/drawer UX. Workarounds for scroll issues have involved external packages like
react-remove-scroll, but these are not exposed or toggleable via a simple prop[3].
- There is no documented prop named
Version History:
- No release notes or changelogs indicate the addition of either
handleOnlyordisablePreventScrollas props to<Drawer.Root>as of the latest releases through version 1.0.0[1][7][8]. - Recent GitHub issues and discussions do not mention these as new or planned props[8].
Effects/Workarounds:
- For “handle only” behavior: Currently, dragging can be initiated from anywhere inside the drawer by default; you cannot restrict drag-only-to-handle with a root prop. Use
<Drawer.Handle />for visual indication, but code changes are required for drag-limiting[1]. - For scroll prevention control: By default, the body scroll is prevented when the drawer is open. More granular control requires you to use workarounds, such as wrappings with
RemoveScrollor forking the library[3].
In summary:
Neither handleOnly nor disablePreventScroll is supported props on <Drawer.Root> in Vaul as of the latest versions documented up to August 2025[1][10][7][8]. All drawer scrolling and drag behaviors are managed internally or must be controlled via workarounds.
Citations:
- 1: https://www.npmjs.com/package/vaul/v/1.0.0
- 2: https://ui.shadcn.com/docs/changelog
- 3: Mobile: Drawer doesn't close with scrollable content and modal set to false emilkowalski/vaul#168
- 4: https://ui.nuxt.com/components/drawer
- 5: https://vaul.emilkowal.ski
- 6: https://emilkowal.ski/ui/building-a-drawer-component
- 7: https://www.npmjs.com/package/vaul/v/0.9.5?activeTab=versions
- 8: https://github.com/emilkowalski/vaul
- 9: https://shadcn-svelte.com/docs/migration/tailwind-v4
- 10: https://vaul.emilkowal.ski/api
Remove unsupported props from Vaul <Drawer.Root>.
Vaul’s <Drawer.Root> does not support handleOnly or disablePreventScroll, so they will be ignored at runtime. You’ll need to remove these props (or replace them with supported patterns) to ensure the drawer behaves as expected.
• File: app/_components/BottomSheetRenderer.tsx (around lines 63–69)
• Remove the two unsupported props and, if needed:
– Use <Drawer.Handle /> inside <Drawer.Content> for a drag handle
– Wrap content in a scroll-lock component (e.g., react-remove-scroll) to control body scrolling
Suggested diff:
<Drawer.Root
key={`${ContentComponent}`}
open={isBottomSheetOpen}
defaultOpen={false}
- handleOnly
- disablePreventScroll={isBottomSheetOpen && !hasBackdrop}
>📝 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.
| <Drawer.Root | |
| key={`${ContentComponent}`} | |
| open={isBottomSheetOpen} | |
| defaultOpen={false} | |
| handleOnly | |
| disablePreventScroll={isBottomSheetOpen && !hasBackdrop} | |
| > | |
| <Drawer.Root | |
| key={`${ContentComponent}`} | |
| open={isBottomSheetOpen} | |
| defaultOpen={false} | |
| > |
🤖 Prompt for AI Agents
In app/_components/BottomSheetRenderer.tsx around lines 63–69, remove the
unsupported props handleOnly and disablePreventScroll from the <Drawer.Root>
usage because Vaul’s Drawer.Root ignores them; instead, add a <Drawer.Handle />
inside <Drawer.Content> if a drag handle is needed and wrap scrollable content
or the drawer content with a scroll-lock solution (e.g., react-remove-scroll or
a similar component) to control body scrolling when the drawer is open.
| useEffect(() => { | ||
| if (!openBottomSheet) { | ||
| closeBottomSheet(); | ||
| } | ||
|
|
||
| const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = { | ||
| backdropProps: { | ||
| className: "fixed inset-0 z-20 bg-neutral-700/50 ", | ||
| onClick: () => { | ||
| setOpenBottomSheet(false); | ||
| }, | ||
| }, | ||
| ContentComponent: GoalDurationBottomSheet, | ||
| hasBackdrop: true, | ||
| bottomSheetFixerStyle: { | ||
| bottom: 0, | ||
| }, | ||
| contentProps: { | ||
| setopenBottomSheet: setOpenBottomSheet, | ||
| onEdit: async ({ type, value }) => { | ||
| setGoalAddInfo((prev) => ({ | ||
| ...prev, | ||
| durationType: type, | ||
| durationValue: value, | ||
| })); | ||
| // 닫기 | ||
| setOpenBottomSheet(false); | ||
| }, | ||
| }, | ||
| }; | ||
| const isOpended = checkOpened(); | ||
| if (openBottomSheet && !isOpended) { | ||
| openGoalDurationBottomSheet(bottomSheetInfo); | ||
| return; | ||
| } | ||
|
|
||
| if (openBottomSheet) { | ||
| updateGoalDurationBottomSheet(bottomSheetInfo); | ||
| } | ||
|
|
||
| return () => { | ||
| if (!openBottomSheet) { | ||
| closeBottomSheet(); | ||
| } | ||
| }; | ||
| }, [openBottomSheet]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential memory leaks and race conditions in useEffect.
The effect creates and updates bottom sheet configurations but has several issues:
- The cleanup function (Lines 80-84) only runs on component unmount or when dependencies change, but doesn't handle edge cases where
closeBottomSheetmight be called multiple times. - Missing dependencies in the effect dependency array could lead to stale closures.
Add all dependencies and improve cleanup:
useEffect(() => {
if (!openBottomSheet) {
closeBottomSheet();
+ return;
}
const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = {
backdropProps: {
className: "fixed inset-0 z-20 bg-neutral-700/50 ",
onClick: () => {
setOpenBottomSheet(false);
},
},
ContentComponent: GoalDurationBottomSheet,
hasBackdrop: true,
bottomSheetFixerStyle: {
bottom: 0,
},
contentProps: {
setopenBottomSheet: setOpenBottomSheet,
onEdit: async ({ type, value }) => {
setGoalAddInfo((prev) => ({
...prev,
durationType: type,
durationValue: value,
}));
// 닫기
setOpenBottomSheet(false);
},
},
};
const isOpended = checkOpened();
if (openBottomSheet && !isOpended) {
openGoalDurationBottomSheet(bottomSheetInfo);
return;
}
if (openBottomSheet) {
updateGoalDurationBottomSheet(bottomSheetInfo);
}
- return () => {
- if (!openBottomSheet) {
- closeBottomSheet();
- }
- };
- }, [openBottomSheet]);
+ }, [
+ openBottomSheet,
+ checkOpened,
+ closeBottomSheet,
+ openGoalDurationBottomSheet,
+ updateGoalDurationBottomSheet,
+ setGoalAddInfo,
+ ]);📝 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.
| useEffect(() => { | |
| if (!openBottomSheet) { | |
| closeBottomSheet(); | |
| } | |
| const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = { | |
| backdropProps: { | |
| className: "fixed inset-0 z-20 bg-neutral-700/50 ", | |
| onClick: () => { | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| ContentComponent: GoalDurationBottomSheet, | |
| hasBackdrop: true, | |
| bottomSheetFixerStyle: { | |
| bottom: 0, | |
| }, | |
| contentProps: { | |
| setopenBottomSheet: setOpenBottomSheet, | |
| onEdit: async ({ type, value }) => { | |
| setGoalAddInfo((prev) => ({ | |
| ...prev, | |
| durationType: type, | |
| durationValue: value, | |
| })); | |
| // 닫기 | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| }; | |
| const isOpended = checkOpened(); | |
| if (openBottomSheet && !isOpended) { | |
| openGoalDurationBottomSheet(bottomSheetInfo); | |
| return; | |
| } | |
| if (openBottomSheet) { | |
| updateGoalDurationBottomSheet(bottomSheetInfo); | |
| } | |
| return () => { | |
| if (!openBottomSheet) { | |
| closeBottomSheet(); | |
| } | |
| }; | |
| }, [openBottomSheet]); | |
| useEffect(() => { | |
| if (!openBottomSheet) { | |
| closeBottomSheet(); | |
| return; | |
| } | |
| const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = { | |
| backdropProps: { | |
| className: "fixed inset-0 z-20 bg-neutral-700/50 ", | |
| onClick: () => { | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| ContentComponent: GoalDurationBottomSheet, | |
| hasBackdrop: true, | |
| bottomSheetFixerStyle: { | |
| bottom: 0, | |
| }, | |
| contentProps: { | |
| setopenBottomSheet: setOpenBottomSheet, | |
| onEdit: async ({ type, value }) => { | |
| setGoalAddInfo((prev) => ({ | |
| ...prev, | |
| durationType: type, | |
| durationValue: value, | |
| })); | |
| // 닫기 | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| }; | |
| const isOpended = checkOpened(); | |
| if (openBottomSheet && !isOpended) { | |
| openGoalDurationBottomSheet(bottomSheetInfo); | |
| return; | |
| } | |
| if (openBottomSheet) { | |
| updateGoalDurationBottomSheet(bottomSheetInfo); | |
| } | |
| }, [ | |
| openBottomSheet, | |
| checkOpened, | |
| closeBottomSheet, | |
| openGoalDurationBottomSheet, | |
| updateGoalDurationBottomSheet, | |
| setGoalAddInfo, | |
| ]); |
| useEffect(() => { | ||
| if (!openBottomSheet) { | ||
| closeBottomSheet(); | ||
| } | ||
|
|
||
| const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = { | ||
| backdropProps: { | ||
| className: "fixed inset-0 z-20 bg-neutral-700/50 ", | ||
| onClick: () => { | ||
| setOpenBottomSheet(false); | ||
| }, | ||
| }, | ||
| ContentComponent: GoalDurationBottomSheet, | ||
| hasBackdrop: true, | ||
| bottomSheetFixerStyle: { | ||
| bottom: 0, | ||
| }, | ||
| contentProps: { | ||
| setopenBottomSheet: setOpenBottomSheet, | ||
| onEdit: async ({ type, value }) => { | ||
| setEditContents && | ||
| setEditContents((prev) => ({ | ||
| ...prev, | ||
| durationType: type, | ||
| durationValue: value, | ||
| })); | ||
| // 닫기 | ||
| setOpenBottomSheet(false); | ||
| }, | ||
| }, | ||
| }; | ||
| const isOpended = checkOpened(); | ||
| if (openBottomSheet && !isOpended) { | ||
| openGoalDurationBottomSheet(bottomSheetInfo); | ||
| return; | ||
| } | ||
|
|
||
| if (openBottomSheet) updateGoalDurationBottomSheet(bottomSheetInfo); | ||
|
|
||
| return () => { | ||
| if (!openBottomSheet) closeBottomSheet(); | ||
| }; | ||
| }, [openBottomSheet]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Similar useEffect pattern issues as in adding-goal/page.tsx.
This component has the same memory leak and dependency array issues as the adding-goal page.
Apply the same fix pattern:
useEffect(() => {
if (!openBottomSheet) {
closeBottomSheet();
+ return;
}
const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = {
// ... configuration
};
const isOpended = checkOpened();
if (openBottomSheet && !isOpended) {
openGoalDurationBottomSheet(bottomSheetInfo);
return;
}
if (openBottomSheet) updateGoalDurationBottomSheet(bottomSheetInfo);
- return () => {
- if (!openBottomSheet) closeBottomSheet();
- };
- }, [openBottomSheet]);
+ }, [
+ openBottomSheet,
+ checkOpened,
+ closeBottomSheet,
+ openGoalDurationBottomSheet,
+ updateGoalDurationBottomSheet,
+ setEditContents,
+ ]);📝 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.
| useEffect(() => { | |
| if (!openBottomSheet) { | |
| closeBottomSheet(); | |
| } | |
| const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = { | |
| backdropProps: { | |
| className: "fixed inset-0 z-20 bg-neutral-700/50 ", | |
| onClick: () => { | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| ContentComponent: GoalDurationBottomSheet, | |
| hasBackdrop: true, | |
| bottomSheetFixerStyle: { | |
| bottom: 0, | |
| }, | |
| contentProps: { | |
| setopenBottomSheet: setOpenBottomSheet, | |
| onEdit: async ({ type, value }) => { | |
| setEditContents && | |
| setEditContents((prev) => ({ | |
| ...prev, | |
| durationType: type, | |
| durationValue: value, | |
| })); | |
| // 닫기 | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| }; | |
| const isOpended = checkOpened(); | |
| if (openBottomSheet && !isOpended) { | |
| openGoalDurationBottomSheet(bottomSheetInfo); | |
| return; | |
| } | |
| if (openBottomSheet) updateGoalDurationBottomSheet(bottomSheetInfo); | |
| return () => { | |
| if (!openBottomSheet) closeBottomSheet(); | |
| }; | |
| }, [openBottomSheet]); | |
| useEffect(() => { | |
| if (!openBottomSheet) { | |
| closeBottomSheet(); | |
| return; | |
| } | |
| const bottomSheetInfo: Parameters<typeof openGoalDurationBottomSheet>[0] = { | |
| backdropProps: { | |
| className: "fixed inset-0 z-20 bg-neutral-700/50 ", | |
| onClick: () => { | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| ContentComponent: GoalDurationBottomSheet, | |
| hasBackdrop: true, | |
| bottomSheetFixerStyle: { | |
| bottom: 0, | |
| }, | |
| contentProps: { | |
| setopenBottomSheet: setOpenBottomSheet, | |
| onEdit: async ({ type, value }) => { | |
| setEditContents && | |
| setEditContents((prev) => ({ | |
| ...prev, | |
| durationType: type, | |
| durationValue: value, | |
| })); | |
| // 닫기 | |
| setOpenBottomSheet(false); | |
| }, | |
| }, | |
| }; | |
| const isOpended = checkOpened(); | |
| if (openBottomSheet && !isOpended) { | |
| openGoalDurationBottomSheet(bottomSheetInfo); | |
| return; | |
| } | |
| if (openBottomSheet) updateGoalDurationBottomSheet(bottomSheetInfo); | |
| }, [ | |
| openBottomSheet, | |
| checkOpened, | |
| closeBottomSheet, | |
| openGoalDurationBottomSheet, | |
| updateGoalDurationBottomSheet, | |
| setEditContents, | |
| ]); |
🤖 Prompt for AI Agents
In app/details/edit/[goal_id]/_components/GoalEdit/GoalEdit.tsx around lines 57
to 99, the useEffect creates/updates a bottom sheet but has stale-closure and
missing-dependency issues causing potential memory leaks; fix by ensuring all
external functions/vars used inside the effect (openGoalDurationBottomSheet,
updateGoalDurationBottomSheet, closeBottomSheet, checkOpened, setEditContents,
setOpenBottomSheet) are stable or included in the dependency array — either wrap
those handlers in useCallback/useRef upstream or include them in the effect
deps, and add an isMounted/isActive flag (set true at start, false in cleanup)
to avoid state updates after unmount; also ensure the cleanup only closes the
sheet when mounted is true and remove any redundant double-close to prevent
leaks.
| openModal( | ||
| <ModalDeletingSubGoal | ||
| onClose={closeModal} | ||
| onDeletSubGoale={async () => { |
There was a problem hiding this comment.
Typo in prop name onDeletSubGoale.
The modal callback prop has a typo: should be onDeleteSubGoal instead of onDeletSubGoale.
- onDeletSubGoale={async () => {
+ onDeleteSubGoal={async () => {📝 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.
| onDeletSubGoale={async () => { | |
| onDeleteSubGoal={async () => { |
🤖 Prompt for AI Agents
In app/onboarding/_components/OnboardingSubGoalEdit.tsx around line 134, there
is a typo in the prop name "onDeletSubGoale"; rename it to "onDeleteSubGoal"
wherever it's passed and update the receiving component/prop definition to match
the corrected camelCase name so the callback wiring compiles and works as
intended.
| const isImage = | ||
| e.target.files?.[0].type.startsWith("image"); | ||
|
|
||
| const imageUrl = newFile | ||
| ? URL.createObjectURL(newFile) | ||
| : undefined; | ||
|
|
||
| setTodoResult((prev) => ({ | ||
| ...prev, | ||
| file: newFile, | ||
| fileName, | ||
| imagePreviewUrl: isImage ? imageUrl : undefined, | ||
| })); | ||
| }} |
There was a problem hiding this comment.
Fix potential crash when no file selected; prefer using newFile guard
e.target.files?.[0].type.startsWith("image") will throw if the user cancels the file picker (files is null/empty). Use the newFile variable you already computed and optional chaining.
- const isImage =
- e.target.files?.[0].type.startsWith("image");
-
- const imageUrl = newFile
- ? URL.createObjectURL(newFile)
- : undefined;
+ const isImage = newFile?.type?.startsWith("image") ?? false;
+ const imageUrl =
+ isImage && newFile ? URL.createObjectURL(newFile) : undefined;📝 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.
| const isImage = | |
| e.target.files?.[0].type.startsWith("image"); | |
| const imageUrl = newFile | |
| ? URL.createObjectURL(newFile) | |
| : undefined; | |
| setTodoResult((prev) => ({ | |
| ...prev, | |
| file: newFile, | |
| fileName, | |
| imagePreviewUrl: isImage ? imageUrl : undefined, | |
| })); | |
| }} | |
| const isImage = newFile?.type?.startsWith("image") ?? false; | |
| const imageUrl = | |
| isImage && newFile ? URL.createObjectURL(newFile) : undefined; | |
| setTodoResult((prev) => ({ | |
| ...prev, | |
| file: newFile, | |
| fileName, | |
| imagePreviewUrl: isImage ? imageUrl : undefined, | |
| })); | |
| }} |
🤖 Prompt for AI Agents
In
components/shared/BottomSheets/TodoResultBottomSheet/TodoResultBottomSheet.tsx
around lines 228 to 241, the code reads
e.target.files?.[0].type.startsWith("image") which can throw when no file is
selected; change the isImage check to use the newFile guard (e.g.
newFile?.type?.startsWith("image") or Boolean(newFile && newFile.type &&
newFile.type.startsWith("image"))) and ensure any use of URL.createObjectURL is
only done when newFile is truthy so the handler gracefully handles cancelled
file picks.
| const openBottomSheet = ( | ||
| newBottomSheetInfo: NewBottomSheetInfo<TContentProps>, | ||
| ) => { | ||
| // | ||
| // const newBottomSheetId = crypto.randomUUID(); | ||
| const newBottomSheetId = uuidV4(); | ||
| updateBottomSheetInfoHistory((prev) => { | ||
| const newHistory = [ | ||
| ...prev, | ||
| { ...newBottomSheetInfo, id: newBottomSheetId }, | ||
| ]; | ||
| return newHistory; | ||
| }); | ||
| setId(newBottomSheetId); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return the generated bottom sheet id from openBottomSheet (doc-code mismatch)
The JSDoc says this returns an id, but the function returns void. Returning the id allows advanced flows to reference it if needed.
- const openBottomSheet = (
+ const openBottomSheet = (
newBottomSheetInfo: NewBottomSheetInfo<TContentProps>,
- ) => {
+ ): string => {
//
// const newBottomSheetId = crypto.randomUUID();
const newBottomSheetId = uuidV4();
updateBottomSheetInfoHistory((prev) => {
const newHistory = [
...prev,
{ ...newBottomSheetInfo, id: newBottomSheetId },
];
return newHistory;
});
setId(newBottomSheetId);
+ return newBottomSheetId;
};📝 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.
| const openBottomSheet = ( | |
| newBottomSheetInfo: NewBottomSheetInfo<TContentProps>, | |
| ) => { | |
| // | |
| // const newBottomSheetId = crypto.randomUUID(); | |
| const newBottomSheetId = uuidV4(); | |
| updateBottomSheetInfoHistory((prev) => { | |
| const newHistory = [ | |
| ...prev, | |
| { ...newBottomSheetInfo, id: newBottomSheetId }, | |
| ]; | |
| return newHistory; | |
| }); | |
| setId(newBottomSheetId); | |
| }; | |
| const openBottomSheet = ( | |
| newBottomSheetInfo: NewBottomSheetInfo<TContentProps>, | |
| ): string => { | |
| // | |
| // const newBottomSheetId = crypto.randomUUID(); | |
| const newBottomSheetId = uuidV4(); | |
| updateBottomSheetInfoHistory((prev) => { | |
| const newHistory = [ | |
| ...prev, | |
| { ...newBottomSheetInfo, id: newBottomSheetId }, | |
| ]; | |
| return newHistory; | |
| }); | |
| setId(newBottomSheetId); | |
| return newBottomSheetId; | |
| }; |
🤖 Prompt for AI Agents
In hooks/useBottomSheet.ts around lines 42 to 56, openBottomSheet currently
generates a newBottomSheetId but returns void despite JSDoc saying it returns an
id; change the function to return the generated id (string) by returning
newBottomSheetId at the end, update the function signature/type to reflect a
string return, and adjust/annotate the JSDoc to indicate it returns the new
bottom sheet id; ensure any callers are updated or their return values handled
(or left unused) to avoid type errors.
mocks/guestMode/handlers.ts
Outdated
| if (goal.isPeriodByMonth && goal.month && !goal.dueDate) { | ||
| // Calculate due date from creation date + months | ||
| const createdDate = new Date(goal.createdAt || new Date()); | ||
| const dueDate = new Date(createdDate); | ||
| dueDate.setMonth(dueDate.getMonth() + goal.month); | ||
|
|
||
| return { | ||
| ...goal, | ||
| dueDate: dueDate.toISOString().split('T')[0] // YYYY-MM-DD format | ||
| }; | ||
| } | ||
| return goal; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential date calculation issues with month-based goals.
The dueDate calculation for month-based goals doesn't handle edge cases like month-end dates properly. For example, if created on January 31st with 1 month duration, JavaScript's setMonth would result in March 3rd (or 2nd in non-leap years).
Use a more robust date calculation:
if (goal.isPeriodByMonth && goal.month && !goal.dueDate) {
// Calculate due date from creation date + months
const createdDate = new Date(goal.createdAt || new Date());
const dueDate = new Date(createdDate);
- dueDate.setMonth(dueDate.getMonth() + goal.month);
+ const targetMonth = dueDate.getMonth() + goal.month;
+ const targetYear = dueDate.getFullYear() + Math.floor(targetMonth / 12);
+ const newMonth = targetMonth % 12;
+ dueDate.setFullYear(targetYear, newMonth, 1);
+ // Handle last day of month
+ const lastDayOfTargetMonth = new Date(targetYear, newMonth + 1, 0).getDate();
+ const dayToSet = Math.min(createdDate.getDate(), lastDayOfTargetMonth);
+ dueDate.setDate(dayToSet);
return {
...goal,
dueDate: dueDate.toISOString().split('T')[0] // YYYY-MM-DD format
};
}📝 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.
| if (goal.isPeriodByMonth && goal.month && !goal.dueDate) { | |
| // Calculate due date from creation date + months | |
| const createdDate = new Date(goal.createdAt || new Date()); | |
| const dueDate = new Date(createdDate); | |
| dueDate.setMonth(dueDate.getMonth() + goal.month); | |
| return { | |
| ...goal, | |
| dueDate: dueDate.toISOString().split('T')[0] // YYYY-MM-DD format | |
| }; | |
| } | |
| return goal; | |
| }); | |
| if (goal.isPeriodByMonth && goal.month && !goal.dueDate) { | |
| // Calculate due date from creation date + months | |
| const createdDate = new Date(goal.createdAt || new Date()); | |
| const dueDate = new Date(createdDate); | |
| const targetMonth = dueDate.getMonth() + goal.month; | |
| const targetYear = dueDate.getFullYear() + Math.floor(targetMonth / 12); | |
| const newMonth = targetMonth % 12; | |
| dueDate.setFullYear(targetYear, newMonth, 1); | |
| // Handle last day of month | |
| const lastDayOfTargetMonth = new Date(targetYear, newMonth + 1, 0).getDate(); | |
| const dayToSet = Math.min(createdDate.getDate(), lastDayOfTargetMonth); | |
| dueDate.setDate(dayToSet); | |
| return { | |
| ...goal, | |
| dueDate: dueDate.toISOString().split('T')[0] // YYYY-MM-DD format | |
| }; | |
| } | |
| return goal; |
🤖 Prompt for AI Agents
In mocks/guestMode/handlers.ts around lines 125 to 137, the month-based dueDate
calculation uses Date.setMonth which misbehaves for month-end dates (e.g. Jan 31
+ 1 month becomes Mar 3); fix by computing the target year and month explicitly,
determine the number of days in the target month, and set the due date day to
the minimum of the original day and that month's last day, then build the ISO
YYYY-MM-DD string from that normalized date so month additions preserve expected
month-end semantics.
mocks/guestMode/handlers.ts
Outdated
| }); | ||
|
|
||
| //test | ||
| console.log("goal in handler: ", processedGoals); |
There was a problem hiding this comment.
Remove console.log statement from production code.
Debug logging should be removed or replaced with proper logging infrastructure.
- //test
- console.log("goal in handler: ", processedGoals);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mocks/guestMode/handlers.ts around line 140, remove the debugging
console.log("goal in handler: ", processedGoals); from production code and
replace it with a call to the project's logging utility (or delete it if not
needed). If you need to retain the information, use the standardized logger at
the appropriate level (e.g., logger.debug or processLogger.debug) and ensure no
sensitive data is logged; otherwise simply delete the console.log line.
| <img width="1894" height="945" alt="image" src="https://github.com/user-attachments/assets/0154c0fc-1dcf-4f58-b415-591a75fd751e" /> | ||
| <a href="10th-motimo-storybook.vercel.app">10th-motimo-storybook.vercel.app</a> | ||
|
|
There was a problem hiding this comment.
Fix broken Storybook link (missing protocol)
Without https://, this becomes a relative link on GitHub.
-<a href="10th-motimo-storybook.vercel.app">10th-motimo-storybook.vercel.app</a>
+<a href="https://10th-motimo-storybook.vercel.app">10th-motimo-storybook.vercel.app</a>📝 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.
| <a href="https://10th-motimo-storybook.vercel.app">10th-motimo-storybook.vercel.app</a> |
🤖 Prompt for AI Agents
In README.md around line 188 the Storybook link is missing the URL protocol
causing GitHub to treat it as a relative link; update the link to include the
full protocol (https://) so it points to the absolute Storybook URL (e.g.
https://your-storybook-domain) and verify the link renders correctly on GitHub
by previewing the README.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/_components/GuestModeHandler.tsx (4)
3-6: Group imports by category for better organization.Consider grouping imports by external packages and internal modules for better readability.
import { useEffect, useRef } from "react"; + import useAuthStore from "@/stores/useAuthStore"; import useOnboardingStore from "@/stores/useOnboardingStore"; import { MSWComponent } from "@/components/_mocks/MSWComponent";
13-41: Consider extracting cleanup logic into a custom hook for better separation of concerns.The cleanup logic is complex enough to warrant extraction into a custom hook (e.g.,
useGuestModeCleanup). This would improve testability and make the component more focused on rendering.Example refactor:
+const useGuestModeCleanup = (isGuest: boolean, isLoggedIn: boolean, resetOnboarding: () => void) => { + const hasCleanedUp = useRef(false); + + useEffect(() => { + if (!isGuest && isLoggedIn && !hasCleanedUp.current) { + const timeoutId = setTimeout(async () => { + try { + const { stopMsw } = await import("../../mocks/index"); + await stopMsw(); + resetOnboarding(); + hasCleanedUp.current = true; + console.log("Guest mode cleanup completed"); + } catch (error) { + console.log("MSW cleanup error (likely already stopped):", error); + } + }, 100); + + return () => clearTimeout(timeoutId); + } + + if (isGuest) { + hasCleanedUp.current = false; + } + }, [isGuest, isLoggedIn, resetOnboarding]); +}; const GuestModeHandler = () => { const { isGuest, isLoggedIn } = useAuthStore(); const { resetOnboarding } = useOnboardingStore(); - const hasCleanedUp = useRef(false); - - // Ensure MSW is stopped and guest data is cleared when transitioning out of guest mode - useEffect(() => { - // Only cleanup when transitioning from guest mode to authenticated mode - if (!isGuest && isLoggedIn && !hasCleanedUp.current) { - // Add a small delay to ensure auth state is stable - const timeoutId = setTimeout(async () => { - try { - // Stop MSW worker - const { stopMsw } = await import("../../mocks/index"); - await stopMsw(); - - // Clear guest mode onboarding data - resetOnboarding(); - - hasCleanedUp.current = true; - console.log("Guest mode cleanup completed"); - } catch (error) { - console.log("MSW cleanup error (likely already stopped):", error); - } - }, 100); // Small delay to let auth state stabilize - - return () => clearTimeout(timeoutId); - } - - // Reset cleanup flag when entering guest mode again - if (isGuest) { - hasCleanedUp.current = false; - } - }, [isGuest, isLoggedIn, resetOnboarding]); + + useGuestModeCleanup(isGuest, isLoggedIn, resetOnboarding); return <>{isGuest && <MSWComponent />}</>; };
18-33: Consider adding more robust error handling and using environment-specific logging.The current error handling could be improved:
- The 100ms delay might be insufficient in slow environments
- Console logs should be conditional based on environment
- The error message assumes MSW is already stopped, but other errors could occur
- // Add a small delay to ensure auth state is stable - const timeoutId = setTimeout(async () => { + // Add a delay to ensure auth state is stable + const CLEANUP_DELAY_MS = process.env.NODE_ENV === 'test' ? 0 : 100; + const timeoutId = setTimeout(async () => { try { // Stop MSW worker const { stopMsw } = await import("../../mocks/index"); await stopMsw(); // Clear guest mode onboarding data resetOnboarding(); hasCleanedUp.current = true; - console.log("Guest mode cleanup completed"); + if (process.env.NODE_ENV === 'development') { + console.log("Guest mode cleanup completed"); + } } catch (error) { - console.log("MSW cleanup error (likely already stopped):", error); + if (process.env.NODE_ENV === 'development') { + console.warn("Guest mode cleanup error:", error); + } + // Still mark as cleaned up to prevent infinite retries + hasCleanedUp.current = true; } - }, 100); // Small delay to let auth state stabilize + }, CLEANUP_DELAY_MS);
43-43: Consider memoizing the JSX fragment to avoid unnecessary re-renders.The conditional rendering could benefit from memoization when
isGuestdoesn't change.+import { useMemo } from "react"; - return <>{isGuest && <MSWComponent />}</>; + return useMemo(() => ( + <>{isGuest && <MSWComponent />}</> + ), [isGuest]);app/onboarding/page.tsx (2)
18-23: Avoid full-store subscriptions: use a Zustand selector to reduce re-renders.Destructuring the entire store without a selector will re-render this component on any auth store change. Prefer a selector with shallow compare.
Apply this diff to the hook usage:
- } = useAuthStore(); + } = useAuthStore( + (s) => ({ + setHasCompletedOnboarding: s.setHasCompletedOnboarding, + isLoggedIn: s.isLoggedIn, + hasCompletedOnboarding: s.hasCompletedOnboarding, + isGuest: s.isGuest, + }), + shallow, + );And add this import near the top of the file:
import { shallow } from "zustand/shallow";
125-131: Spinner gating for non-guest is sensible, but there’s a potential indefinite spinner edge case.If useGoals does not initiate a request before login (e.g., the hook internally no-ops while logged out/guest and doesn’t set error), after login you satisfy (!goals && !error) and show the spinner, but the mutate effect won’t run because error is falsy — leading to a stuck spinner.
Consider broadening the “retry after login” effect to also fire when there’s no data and not currently loading:
// Replace the effect around lines 51-57 with: useEffect(() => { if (!hasHydrated || !isLoggedIn || isGuest) return; // Re-fetch when previously errored OR when no data has ever been loaded. if (error || (!goals && !isLoading)) { console.log("Retrying goals API after login or when no data yet..."); mutate(); } }, [hasHydrated, isLoggedIn, isGuest, goals, isLoading, error, mutate]);Additionally, if feasible, make useGoals skip fetching entirely for guests (e.g., SWR key = null or an
enabledflag) to avoid unnecessary 401s and console noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/_components/GuestModeHandler.tsx(1 hunks)app/onboarding/page.tsx(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/_components/GuestModeHandler.tsx (1)
mocks/index.ts (1)
stopMsw(29-44)
app/onboarding/page.tsx (1)
app/onboarding/_components/SubGoalSelectionScreen.tsx (1)
SubGoalSelectionScreen(16-101)
🔇 Additional comments (5)
app/onboarding/page.tsx (5)
8-8: New SubGoalSelection step is correctly wired into the flow.Import looks correct and aligns with the new step at index 3.
67-71: Redirect gating by isGuest is correct.This prevents guests from being redirected away from onboarding when they’ve “completed” it. This matches the PR objectives.
If you are developing under React StrictMode, consider guarding router.replace with a ref to avoid double-invocation side-effects in dev (optional).
73-78: Auto-complete onboarding when goals already exist (non-guest) — correct, but watch StrictMode double fire.Logic is sound and prevents UI flicker with the spinner below. In dev StrictMode, the effect may run twice; the router.replace is idempotent but setHasCompletedOnboarding could be invoked twice.
If this causes noisy logs or unexpected state transitions during development, add a ref guard to ensure single run per mount.
79-87: Dependency list includes isGuest — good.Including isGuest ensures redirects react to guest/auth transitions.
106-108: Step mapping looks good: SubGoalSelection at 3, Completion at 4.This aligns with the updated onboarding flow and the button-label behavior in SubGoalSelectionScreen.
Hys-Lee
left a comment
Hys-Lee
left a comment
There was a problem hiding this comment.
나머지 부분은 동작에 오류 없는 것으로 보입니답.
추가로 api 전체 debounce에서 시간을 좀 길게 잡고 비효율적인 동작이 있어서 제가 추가로 손 보겠습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
mocks/guestMode/db.ts (2)
2-2: Must bump IndexedDB DB_VERSION to trigger onupgradeneeded and create new stores on existing clients.Keeping
DB_VERSION = 1means users who already opened the DB at v1 will never runonupgradeneeded, sosubGoalswon’t be created and any access to it will throw a NotFoundError. Bump the version.Apply this minimal diff:
-const DB_VERSION = 1; +const DB_VERSION = 2;
100-108: Schema/store-name mismatch: handlers use "todoResults" but schema creates "todo-results".Handlers add/get/delete from
todoResultswhile the DB definestodo-results. This will break at runtime (store not found). Also, the current store useskeyPath: "id", but handlers operate withtodoResultIdas the primary identifier. Recommend adding atodoResultsstore withkeyPath: "todoResultId"and migrating iftodo-resultsexists.Suggested schema upgrade (inside onupgradeneeded), plus retaining the old store for backward compatibility:
// id를 keyPath로 지정하여 고유 키로 사용 - if (!db.objectStoreNames.contains("todo-results")) { - const store = db.createObjectStore("todo-results", { - keyPath: "id", - autoIncrement: true, - }); - // 'title'로 검색할 수 있도록 인덱스 생성 (선택 사항) - // store.createIndex("title_idx", "title", { unique: false }); - } + // Legacy store kept for migration only (todo-results) + if (!db.objectStoreNames.contains("todoResults")) { + const store = db.createObjectStore("todoResults", { + keyPath: "todoResultId", + }); + // Indexes as needed later (none required right now) + } + // Optional: migrate from legacy "todo-results" to "todoResults" + if (db.objectStoreNames.contains("todo-results") && db.objectStoreNames.contains("todoResults")) { + const tx = (request.transaction as IDBTransaction)!; + const oldStore = tx.objectStore("todo-results"); + const newStore = tx.objectStore("todoResults"); + const getAllReq = oldStore.getAll(); + getAllReq.onsuccess = () => { + for (const rec of getAllReq.result as any[]) { + const migrated = rec.id ? { ...rec, todoResultId: String(rec.id) } : rec; + newStore.add(migrated); + } + }; + // Optionally delete old store after verifying migration works: + // db.deleteObjectStore("todo-results"); + }Follow-up: After this change, no handler code needs to change (they already use
todoResultsandtodoResultId).mocks/guestMode/handlers.ts (1)
138-145: Bug: sub-goals are linked to a different goal ID than the one persisted.You generate
goalIdbut then createnewGoalwith a differentid. Sub-goals are linked togoalId, so they won’t join with the saved goal (whoseidis different). This breaks reads like GET/v1/goals/:goalId/sub-goals/todos/all.Apply this fix to ensure the same ID is used everywhere:
- const goalId = genId(); - const newGoal = { - id: genId(), + const goalId = genId(); + const newGoal = { + id: goalId, ...body, createdAt: new Date().toISOString(), } as any; await dbService.add<DBGoal>("goals", newGoal); // Create subGoals if provided if (body.subGoals && body.subGoals.length > 0) { const subGoalIds: string[] = []; for (let i = 0; i < body.subGoals.length; i++) { const subGoal = body.subGoals[i]; const subGoalId = genId(); const newSubGoal = { id: subGoalId, - goalId: goalId, + goalId: goalId, // or newGoal.id (same value) title: subGoal.title, todos: [], isCompleted: false, createdAt: new Date().toISOString(), } as DBSubGoal; await dbService.add<DBSubGoal>("subGoals", newSubGoal); subGoalIds.push(subGoalId); } // Update goal with subGoal IDs newGoal.subGoals = subGoalIds; await dbService.put("goals", newGoal); }Also applies to: 151-169
♻️ Duplicate comments (4)
mocks/guestMode/db.ts (2)
80-88: Good: subGoals store is now defined (resolves prior “missing for guest mode” note).The
subGoalsobject store creation is present and consistent with handler usage. This unblocks guest-mode sub-goal persistence.
81-81: (FYI) Past comment referenced this exact spot; it’s now addressed.Earlier feedback noted this wasn’t visible in the diff; it’s present now.
mocks/guestMode/handlers.ts (2)
122-124: Remove debug console.log from production mocks.Keep logs behind a dev flag or use the project logger to avoid noisy output.
- //test - console.log("goal in handler: ", goals); + // noop: avoid console noise in production mocks
1-606: Month-based dueDate edge cases (carry-over at month ends).A prior review flagged
Date.setMonthpitfalls (e.g., Jan 31 + 1 month). If that logic still exists elsewhere in this file (not visible in this diff), please normalize month additions as suggested previously.If needed, I can provide a targeted patch once you point me to the exact block.
🧹 Nitpick comments (3)
mocks/guestMode/db.ts (2)
51-51: Unnecessary “title_idx” on stores withouttitlefield.
users,cheers, and possibly others don’t have atitleproperty. Creatingtitle_idxthere is harmless but unused noise.You can safely drop those index creations, or add them only on stores that actually include
title.Also applies to: 67-67, 77-77, 87-87, 97-97
123-138: Consider returning the inserted key from add() without forcing anidproperty.
add<T>always attaches{ id: insertedId }, which is wrong for stores liketodoResultsthat key ontodoResultId. Prefer returning{ key: request.result, value: data }or accept the key field name as a parameter.Example API sketch (outside this diff range):
- add<T, K extends keyof T>(storeName: StoreName, data: T, keyField: K): Promise
- or return
{ key: IDBValidKey }and let callers merge.mocks/guestMode/handlers.ts (1)
409-410: Inconsistent status/body: 204 responses must not include a body.Returning JSON with
{ status: 204 }is contradictory. Use 200 with body, or 204 with no body.- return HttpResponse.json({ todoId: updated.id } as TodoIdRs, { - status: 204, - }); + return HttpResponse.json({ todoId: updated.id } as TodoIdRs, { status: 200 }); + // or: + // return HttpResponse.json(undefined, { status: 204 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
mocks/guestMode/db.ts(1 hunks)mocks/guestMode/handlers.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
mocks/guestMode/handlers.ts (2)
mocks/guestMode/types.ts (1)
DBSubGoal(31-38)mocks/guestMode/db.ts (1)
dbService(206-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: chromatic-test
🔇 Additional comments (1)
mocks/guestMode/handlers.ts (1)
460-461: The above script will reveal exactly how the IndexedDB stores are defined (including their names and keyPaths) and confirm whether atodo-resultsstore (kebab-case) is registered versustodoResults. Once we see those definitions, we can update these handlers—or the schema—in lock-step to prevent the runtime errors you noted.
2 participants
Todo
Summary by CodeRabbit
New Features
Style