[FIX] 투두 편집 관련 버그 3건 수정 (반복 선택, 에러 토스트, 모달 오버플로우)#247
Conversation
- 반복 비활성 상태에서 frequency 기본값이 "DAILY"로 채워져 있어 "매일"을 선택해도 변경 없음으로 판단되던 문제를 수정했습니다 - RepeatSelector에 isActive prop을 추가해 반복 활성 여부를 함께 판단하도록 했습니다 - 사용자가 실제로 항목을 클릭했는지 추적해, 값이 기본값과 같아도 반복을 처음 활성화하는 경우엔 반드시 커밋하도록 했습니다 - TodoToolbar가 이미 갖고 있던 isRepeatActive를 RepeatSelector로 전달하도록 했습니다
- 완료 토글, 서브태스크 토글, 순서 변경 실패 시 로컬 상태만 되돌리고 사용자에게 아무 메시지도 보여주지 않던 문제를 수정했습니다 - onUpdateError 콜백을 추가해 서버가 내려준 에러 메시지(error.response.data.message)를 그대로 토스트로 노출하도록 했습니다 - 메시지가 없을 때는 todoUpdateFailed 문구로 폴백하도록 했습니다
- 모달 콘텐츠가 뷰포트보다 커지면 화면 밖으로 잘리던 문제를 수정했습니다 - 다이얼로그를 뷰포트 전체를 덮는 스크롤 래퍼와 flex 중앙 정렬 컨테이너로 감싸, 콘텐츠가 뷰포트보다 클 때만 래퍼가 스크롤되도록 했습니다 - 모달 박스 자체에는 overflow 제약을 두지 않아, 내부 드롭다운(날짜/우선순위 등)이 모달 경계에 잘리지 않고 정상적으로 열리도록 했습니다 - 배경 클릭으로 닫는 로직을 새 스크롤 래퍼로 옮기고, 가려져서 동작하지 않던 기존 backdrop의 클릭 핸들러를 정리했습니다
- DropdownView 아이템의 좌우 패딩(px-1.5)을 제거했습니다
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
⏰ Timo-web
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Walkthrough투두 업데이트 실패 시 롤백과 서버 메시지 전달을 추가하고, 반복 선택 저장 조건을 보완했습니다. 모달의 외부 클릭·스크롤 레이아웃과 드롭다운 항목 여백도 수정했습니다. Changes투두 편집 버그 수정
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Timo Performance ReportBundle Size — timo-web
Lighthouse — timo-web
Image Optimization — timo-web
측정 커밋: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/timo-design-system/src/components/dropdown-view/DropdownView.tsx (1)
47-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win시맨틱한 DOM 구조와 접근성 향상을 위해
border-b사용을 제안해요.드롭다운 아이템의 불필요한 여백을 깔끔하게 정리해주셨네요! ✨
추가적으로, 접근성(A11y) 관점에서 작은 개선을 제안드립니다.
role="listbox"를 가진 부모(Dropdown.Panel) 내부에는role="option"역할을 하는 요소들만 직접적인 자식(논리적 구조상)으로 배치되는 것이 스크린 리더 환경에서 가장 이상적입니다.현재처럼 별도의
div를 구분선으로 사용하기보다는, 마지막 아이템을 제외한Dropdown.Item자체에border-b스타일을 추가하면 DOM 구조도 단순해지고 접근성도 한층 더 좋아집니다. ARIA Listbox 패턴 가이드를 함께 살펴보시면 실무에 활용하기 좋으실 거예요! 📚🛠️ 제안하는 리팩토링 코드
- <Fragment key={item}> - <Dropdown.Item - role="option" - aria-selected={isSelected} - onClick={() => onChange(item)} - className="typo-body-m-12 text-timo-gray-900 py-1" - > - {item} - </Dropdown.Item> - {i < items.length - 1 && ( - <div className="bg-timo-gray-500 h-px w-full" /> - )} - </Fragment> + <Dropdown.Item + key={item} + role="option" + aria-selected={isSelected} + onClick={() => onChange(item)} + className={cn( + "typo-body-m-12 text-timo-gray-900 py-1", + i < items.length - 1 && "border-timo-gray-500 border-b", + )} + > + {item} + </Dropdown.Item>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/timo-design-system/src/components/dropdown-view/DropdownView.tsx` around lines 47 - 59, Update the DropdownView item rendering to remove the separate divider div and apply the equivalent border-b styling directly to each Dropdown.Item except the last item. Preserve the existing role="option", selection behavior, and spacing while ensuring Dropdown.Panel has only option elements as direct children.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/timo-web/app/`[locale]/(main)/(with-time-sidebar)/today/_hooks/useTodayTodoList.ts:
- Around line 102-104: Update the onError handler in the today todo mutation to
avoid restoring the entire previous array; use a functional setTodos update that
reverts only the specific todo changed by the failed request while preserving
concurrent edits to other todos, then keep the existing onUpdateError call.
In `@apps/timo-web/components/modal/OverlayModal.tsx`:
- Around line 99-121: Update the modal layout around the wrapper div and
dialogRef element: remove the parent’s items-center and justify-center alignment
classes, and add m-auto to the dialog’s className composition. Preserve the
existing visibility, sizing, and transition classes so small modals remain
centered while oversized content can scroll from the top.
---
Outside diff comments:
In `@packages/timo-design-system/src/components/dropdown-view/DropdownView.tsx`:
- Around line 47-59: Update the DropdownView item rendering to remove the
separate divider div and apply the equivalent border-b styling directly to each
Dropdown.Item except the last item. Preserve the existing role="option",
selection behavior, and spacing while ensuring Dropdown.Panel has only option
elements as direct children.
🪄 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 Plus
Run ID: 904ece68-d582-43eb-a591-f396241c51a0
📒 Files selected for processing (8)
apps/timo-web/app/[locale]/(main)/(with-time-sidebar)/home/_containers/HomeTodoContainer.tsxapps/timo-web/app/[locale]/(main)/(with-time-sidebar)/home/_hooks/use-home-todos-by-date.tsapps/timo-web/app/[locale]/(main)/(with-time-sidebar)/today/_containers/TodayTodoListContainer.tsxapps/timo-web/app/[locale]/(main)/(with-time-sidebar)/today/_hooks/useTodayTodoList.tsapps/timo-web/components/modal/OverlayModal.tsxpackages/timo-design-system/src/components/dropdown-view/DropdownView.tsxpackages/timo-design-system/src/components/repeat/repeat-selector/RepeatSelector.tsxpackages/timo-design-system/src/components/todo/todo-toolbar/TodoToolbar.tsx
| onError: (error: ErrorType<ErrorDto>) => { | ||
| setTodos(previous); | ||
| onUpdateError(error.response?.data.message); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
상태 롤백 시 동시 업데이트(Concurrent Update) 유실을 방지해 보세요.
에러 발생 시 previous 배열 전체로 상태를 덮어씌우면, 서버 요청이 진행되는 동안 사용자가 다른 투두를 수정했을 때 그 변경 사항까지 함께 날아갈 수 있습니다. 배열 전체를 되돌리기보다는, 변경을 시도했던 특정 투두의 상태만 함수형 업데이트(setTodos((prev) => ...))로 원상 복구하는 방식이 더 안전합니다. 😊
(공식 문서 참고: React State Updates)
Also applies to: 214-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/timo-web/app/`[locale]/(main)/(with-time-sidebar)/today/_hooks/useTodayTodoList.ts
around lines 102 - 104, Update the onError handler in the today todo mutation to
avoid restoring the entire previous array; use a functional setTodos update that
reverts only the specific todo changed by the failed request while preserving
concurrent edits to other todos, then keep the existing onUpdateError call.
| <div | ||
| className="flex min-h-full items-center justify-center py-8" | ||
| onPointerDownCapture={() => { | ||
| wasFloatingLayerOpenRef.current = hasOpenFloatingLayer(); | ||
| }} | ||
| onClick={(event) => { | ||
| if (event.target !== event.currentTarget) return; | ||
| if (wasFloatingLayerOpenRef.current) return; | ||
| onClose(); | ||
| }} | ||
| > | ||
| <div | ||
| ref={dialogRef} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label={ariaLabel} | ||
| tabIndex={-1} | ||
| className={cn( | ||
| "flex flex-col rounded-[4px] bg-white transition-all duration-200 ease-out", | ||
| isVisible ? "scale-100 opacity-100" : "scale-95 opacity-0", | ||
| className, | ||
| )} | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Flexbox의 콘텐츠 잘림 현상을 방지하기 위해 m-auto 활용을 추천해요.
모달이 뷰포트보다 길어질 때 잘리는 문제를 해결하기 위해 flex 레이아웃을 도입하신 점 멋집니다! 👍
다만, 부모 요소에 items-center와 justify-center를 사용하면 내부 콘텐츠가 뷰포트보다 길어질 때 위쪽(Top) 영역이 화면 밖으로 잘려서 스크롤로도 도달할 수 없는 CSS Flexbox의 고질적인 문제가 발생할 수 있어요.
이를 방지하려면 부모의 items-center justify-center를 제거하고, 자식 요소(모달창)에 m-auto를 부여하는 방식을 제안합니다. 이렇게 하면 모달이 작을 때는 완벽하게 중앙 정렬되고, 길어지면 자연스럽게 상단부터 스크롤되도록 만들 수 있습니다. 💡
접근성 및 Flexbox 동작에 대한 MDN 문서도 참고해 보시면 레이아웃 설계에 큰 도움이 될 거예요!
✨ 제안하는 CSS 레이아웃 수정안
- <div
- className="flex min-h-full items-center justify-center py-8"
- onPointerDownCapture={() => {
- wasFloatingLayerOpenRef.current = hasOpenFloatingLayer();
- }}
- onClick={(event) => {
- if (event.target !== event.currentTarget) return;
- if (wasFloatingLayerOpenRef.current) return;
- onClose();
- }}
- >
- <div
- ref={dialogRef}
- role="dialog"
- aria-modal="true"
- aria-label={ariaLabel}
- tabIndex={-1}
- className={cn(
- "flex flex-col rounded-[4px] bg-white transition-all duration-200 ease-out",
- isVisible ? "scale-100 opacity-100" : "scale-95 opacity-0",
- className,
- )}
- >
+ <div
+ className="flex min-h-full py-8"
+ onPointerDownCapture={() => {
+ wasFloatingLayerOpenRef.current = hasOpenFloatingLayer();
+ }}
+ onClick={(event) => {
+ if (event.target !== event.currentTarget) return;
+ if (wasFloatingLayerOpenRef.current) return;
+ onClose();
+ }}
+ >
+ <div
+ ref={dialogRef}
+ role="dialog"
+ aria-modal="true"
+ aria-label={ariaLabel}
+ tabIndex={-1}
+ className={cn(
+ "m-auto flex flex-col rounded-[4px] bg-white transition-all duration-200 ease-out",
+ isVisible ? "scale-100 opacity-100" : "scale-95 opacity-0",
+ className,
+ )}
+ >📝 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.
| <div | |
| className="flex min-h-full items-center justify-center py-8" | |
| onPointerDownCapture={() => { | |
| wasFloatingLayerOpenRef.current = hasOpenFloatingLayer(); | |
| }} | |
| onClick={(event) => { | |
| if (event.target !== event.currentTarget) return; | |
| if (wasFloatingLayerOpenRef.current) return; | |
| onClose(); | |
| }} | |
| > | |
| <div | |
| ref={dialogRef} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={ariaLabel} | |
| tabIndex={-1} | |
| className={cn( | |
| "flex flex-col rounded-[4px] bg-white transition-all duration-200 ease-out", | |
| isVisible ? "scale-100 opacity-100" : "scale-95 opacity-0", | |
| className, | |
| )} | |
| > | |
| <div | |
| className="flex min-h-full py-8" | |
| onPointerDownCapture={() => { | |
| wasFloatingLayerOpenRef.current = hasOpenFloatingLayer(); | |
| }} | |
| onClick={(event) => { | |
| if (event.target !== event.currentTarget) return; | |
| if (wasFloatingLayerOpenRef.current) return; | |
| onClose(); | |
| }} | |
| > | |
| <div | |
| ref={dialogRef} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={ariaLabel} | |
| tabIndex={-1} | |
| className={cn( | |
| "m-auto flex flex-col rounded-[4px] bg-white transition-all duration-200 ease-out", | |
| isVisible ? "scale-100 opacity-100" : "scale-95 opacity-0", | |
| className, | |
| )} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/timo-web/components/modal/OverlayModal.tsx` around lines 99 - 121,
Update the modal layout around the wrapper div and dialogRef element: remove the
parent’s items-center and justify-center alignment classes, and add m-auto to
the dialog’s className composition. Preserve the existing visibility, sizing,
and transition classes so small modals remain centered while oversized content
can scroll from the top.
…to fix/web/246-todo-edit-bugs
- 타이머 완료·정지 성공 시 투두 상세 조회 쿼리 캐시를 무효화하지 않았습니다 - 조회 모달을 열어둔 채 타이머가 끝나도 timerStatus가 갱신되지 않아 편집이 계속 막히는 문제를 해결했습니다
…to fix/web/246-todo-edit-bugs
jjangminii
left a comment
jjangminii
left a comment
There was a problem hiding this comment.
checkout 해서 테스트 완료했습니다~ 굿!
2 participants
ISSUE 🔗
close #246
What is this PR? 🔍
투두 편집 플로우에서 발견된 서로 다른 세 가지 버그를 모아 수정했습니다. 반복 선택자의 "매일" 선택 무반응, 홈/투데이 뷰 수정 실패 시 무음 실패, 상세 모달이 뷰포트를 넘을 때의 오버플로우/드롭다운 잘림 문제를 각각 고쳤습니다.
배경
반복 선택자 "매일" 선택 버그
RepeatSelector가 반복 활성 여부(isActive)를 알 수 있도록 하고, 사용자가 실제로 클릭했는지를 추적해 반복을 처음 활성화하는 경우에도 커밋되도록 했습니다.frequency는 기본값"DAILY"로 채워져 있는데, 닫을 때의 커밋 조건이draftFrequency !== frequency만 검사해서 "이미 DAILY인데 DAILY를 또 선택"한 것으로 오인해onFrequencyChange를 호출하지 않았습니다. WEEKLY/MONTHLY는 기본값과 다르므로 우연히 정상 동작했습니다.hasSelectedFrequencyRef로 드롭다운이 열려 있는 동안 사용자가 주기를 실제로 클릭했는지 추적합니다. 닫을 때 커밋 조건을클릭했음 && (값이 바뀜 || 이전에 비활성이었음)으로 바꿔, 값 자체는 같아도 반복을 새로 켜는 경우엔 반드시onFrequencyChange가 호출되도록 했습니다.TodoToolbar가 아이콘 표시에만 쓰던isRepeatActive를RepeatSelector로 함께 전달합니다.홈/투데이 뷰 수정 실패 시 에러 토스트 누락
useHomeTodosByDate/useTodayTodoList에onUpdateError콜백을 추가하고, 각 mutation의onError에서error.response?.data.message를 그대로 전달합니다. 상위 컨테이너에서는 이미 있던playErrorMessage상태와AnimatedToast를 재사용해 표시하며, 메시지가 없으면Toast.todoUpdateFailed로 폴백합니다. 상세 모달 내부 수정(DetailTodoModalContainer)은 이미 동일한 방식의 에러 토스트가 연결되어 있어 이번 변경 대상에서 제외했습니다.상세 모달 오버플로우 / 드롭다운 잘림
OverlayModal의 다이얼로그 위치 방식을 뷰포트 중앙 고정에서, 뷰포트 전체를 덮는 스크롤 래퍼 + flex 중앙 정렬 방식으로 바꿨습니다.fixed top-1/2 left-1/2 -translate-*로 뷰포트 정중앙에 고정돼 있어, 모달 콘텐츠가 뷰포트보다 커지면 위아래로 잘린 채 스크롤할 방법이 없었습니다. 처음에 모달 박스 자체에max-h-[90vh] overflow-y-auto를 주는 방식으로 시도했으나, 그러면 모달 내부의 날짜/우선순위 드롭다운(Dropdown.Panel,position: absolute)이 모달 박스의 스크롤 경계에 잘려 밖으로 열리지 못하는 회귀가 생겨 되돌렸습니다.fixed inset-0 overflow-y-auto래퍼가 뷰포트 전체 크기이므로 평소(모달이 뷰포트보다 작을 때)는 스크롤이 전혀 발생하지 않아 드롭다운도 그대로 밖으로 열립니다. 모달이 뷰포트보다 커질 때만 이 래퍼가 스크롤 가능해집니다. 배경 클릭으로 닫는 로직을 이 래퍼 안쪽 flex 컨테이너로 옮기고(event.target === event.currentTarget으로 빈 여백 클릭만 판별), 기존 backdrop div의 가려져서 동작하지 않던 클릭 핸들러는 정리했습니다. 새로 추가한 클릭 핸들러는aria-hidden처리를 할 수 없는 위치(실제 다이얼로그를 감싸는 요소)라jsx-a11y/no-static-element-interactions,jsx-a11y/click-events-have-key-events를 의도적으로 예외 처리했습니다. 키보드 사용자는 기존 Escape 핸들러로 동일하게 닫을 수 있습니다.apps/timo-web/components/modal/OverlayModal.tsx한 곳만 변경했고, 이 컴포넌트를 쓰는 투두 상세/생성, 회원탈퇴, 태그 생성 모달 4곳 모두에 공통 적용됩니다.기타
DropdownView.tsx의 드롭다운 아이템 좌우 패딩(px-1.5)을 제거했습니다.To Reviewers
OverlayModal변경은 공용 컴포넌트라 투두 상세/생성, 회원탈퇴, 태그 생성 모달 4곳 전부에 영향을 줍니다. 특히 드롭다운이 모달 밖으로 정상적으로 열리는지, 배경 클릭으로 닫히는 동작이 예전과 동일한지 확인 부탁드립니다.반복 선택자 수정은 "반복이 꺼진 상태에서 매일을 선택" 케이스가 핵심이니 그 시나리오 위주로 봐주시면 좋겠습니다.
이번 세션 안에서 브라우저로 직접 조작해보지는 못했고, 코드 추적과 타입체크/린트로만 검증했습니다. 리뷰 시 실제 화면에서 한 번씩 확인 부탁드립니다.
Screenshot 📷
Test Checklist ✔
pnpm check-types통과pnpm lint통과