[FIX] 수정 모달 플레이 버튼 재클릭 시 상태 토글 안 되는 문제 수정#244
Conversation
- overlay로 여는 모달의 onTogglePlay 콜백이 모달을 열 때의 낡은 activeTimer를 계속 참조하던 문제를 ref 기반 콜백으로 해결했습니다 - startTimer/changeStatus 진행 중이거나 activeTimer 리페치 중에는 재생 버튼 클릭을 무시하도록 가드를 추가했습니다
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- 코드만으로 파악되는 설명 주석을 정리했습니다
Timo Performance ReportBundle Size — timo-web
Lighthouse — timo-web
Image Optimization — timo-web
측정 커밋: |
|
Warning Review limit reached
Next review available in: 17 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 (2)
Walkthrough홈과 오늘 목록 훅에 타이머 작업 대기 상태와 중복 실행 방지 가드를 추가하고, 상세 모달은 최신 재생 토글 핸들러를 참조하도록 변경했습니다. Changes타이머 제어 흐름
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
jjangminii
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/containers/todo-modal/detail/DetailTodoModalContainer.tsx`:
- Line 6: Replace the render-time ref.current mutation used for the play toggle
with React’s useEffectEvent, and rename the resulting handler to
handleTogglePlay. Update the affected effect and callback dependencies/usages to
invoke handleTogglePlay while preserving the existing toggle behavior and
closure-safety.
🪄 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: 4af30d5e-eae0-44f8-bf2b-a270c1654df0
📒 Files selected for processing (3)
apps/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/_hooks/useTodayTodoList.tsapps/timo-web/containers/todo-modal/detail/DetailTodoModalContainer.tsx
| import { useTranslations } from "next-intl"; | ||
| import { overlay } from "overlay-kit"; | ||
| import { useCallback, useEffect, useState } from "react"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
렌더링 중 ref.current 직접 수정을 지양하고 useEffectEvent를 활용해 보세요.
훌륭한 아이디어로 클로저 캡처 문제를 해결하셨네요! 센스 만점입니다. 🎉
다만, React 컴포넌트 렌더링 중에 ref.current를 직접 수정하는 것은 컴포넌트의 순수성을 해칠 수 있어 동시성(Concurrent) 렌더링 환경에서 예기치 않은 동작을 유발할 수 있습니다.
제공된 React 19.2 문서에 따르면, 이러한 경우 안정적인 이벤트 핸들러 참조를 제공하는 useEffectEvent를 사용하도록 권장하고 있습니다. 이를 활용하면 렌더링 중 사이드 이펙트 걱정 없이 코드를 훨씬 깔끔하고 안전하게 유지할 수 있습니다. 또한 코딩 가이드라인의 이벤트 핸들러 네이밍 규칙(handle 접두사)에 맞춰 handleTogglePlay로 네이밍을 조정하면 더 완벽할 것 같습니다. 최신 API를 적용해서 코드를 한 단계 더 업그레이드해 보세요! ✨
참고 문서: React 공식 문서 - useEffectEvent (또는 React 19.2 릴리스 노트)
💡 제안하는 리팩토링 코드
-import { useCallback, useEffect, useRef, useState } from "react";
+import { useCallback, useEffect, useEffectEvent, useState } from "react";
// ...
- const onTogglePlayRef = useRef(onTogglePlay);
- onTogglePlayRef.current = onTogglePlay;
+ const handleTogglePlay = useEffectEvent(() => {
+ onTogglePlay();
+ });
// ...
- onTogglePlay={() => onTogglePlayRef.current()}
+ onTogglePlay={handleTogglePlay}Also applies to: 156-158, 177-177
🤖 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/containers/todo-modal/detail/DetailTodoModalContainer.tsx` at
line 6, Replace the render-time ref.current mutation used for the play toggle
with React’s useEffectEvent, and rename the resulting handler to
handleTogglePlay. Update the affected effect and callback dependencies/usages to
invoke handleTogglePlay while preserving the existing toggle behavior and
closure-safety.
Source: Path instructions
ehye1
left a comment
ehye1
left a comment
There was a problem hiding this comment.
단순히 API 중복 호출만 막으면 되는줄 알았는데 overlay.open()이 최초 콜백을 계속 유지하면서 최신 상태를 반영하지 못한 것이 이유였군요.. 모달/portal/overlay를 사용할 경우에는 stale closure를 의심해봐야겠어요
오류 수정 수고하셨습니다!
…to fix/web/243-play-button-toggle-race-condition
3 participants
ISSUE 🔗
close #243
What is this PR? 🔍
할 일 수정 모달에서 플레이 버튼을 연속으로 클릭하면 두 번째 클릭부터 상태가 반영되지 않고 서버가 409를 반환하던 문제를 수정했습니다.
배경
overlay-kit의overlay.open(callback)으로 열리는데, 이 콜백은 모달을 여는 순간에 딱 한 번만 호출되고 모달이 열려있는 동안 다시 호출되지 않습니다.onTogglePlay(부모가 만든 클로저)가 그 시점의activeTimer를 그대로 고정해서 참조하기 때문에, 첫 클릭으로 타이머 상태가 실제로 바뀌어도(예: RUNNING → PAUSED) 모달의 클릭 핸들러는 계속 예전 상태를 기준으로 액션(PAUSE/RESUME)을 계산했습니다. 화면에 보이는 타이머 상태(뱃지 등)는DetailTodoModalQuery가useActiveTimer()를 직접 구독해서 최신으로 갱신되지만, 클릭 시 실행되는 액션 로직만 낡은 상태를 참조하는 불일치가 있었습니다.overlay.open()에 넘기는 콜백에는 매 렌더마다 최신 값으로 갱신되는 ref를 통해 접근하도록 해서, 클릭 시점에는 항상 최신activeTimer상태를 반영한 로직이 실행되도록 했습니다. 추가로 startTimer/changeStatus mutation이 진행 중이거나 activeTimer 쿼리가 리페치 중일 때는 재생 버튼 입력 자체를 무시하는 가드도 추가했습니다.수정 모달 플레이 버튼
DetailTodoModalContainer.tsx에서overlay.open()에 넘기는onTogglePlay를 ref로 감싸고,useTodayTodoList.ts/use-home-todos-by-date.ts의handlePlay/handleTogglePlay에 진행 중 재클릭 가드를 추가했습니다.TIMER_409)가 발생했습니다.DetailTodoModalContainer에서onTogglePlayRef = useRef(onTogglePlay)를 두고 매 렌더마다onTogglePlayRef.current = onTogglePlay로 갱신합니다.overlay.open()에 넘기는 JSX는onTogglePlay={() => onTogglePlayRef.current()}처럼 ref를 통해 간접 호출하도록 바꿔서, 이 래퍼 함수 자체는 모달 오픈 시점에 고정되어도 실제 호출은 클릭 시점에ref.current(그 시점의 최신handlePlay)를 참조하게 됩니다. 또한useTodayTodoList/use-home-todos-by-date에useStartTimer/useChangeStatus의isPending과useActiveTimer()의isFetching을 조합한isTimerActionPending을 만들어handlePlay/handleTogglePlay진입부에서 가드했습니다.isPlayPendingprop을 추가했었는데, 이 값도 같은 이유(고정된 콜럭/props)로 모달 안에서는 정확히 갱신될 수 없어 제거했습니다. ref 기반 수정만으로 실제 동작(액션 계산)은 항상 최신 상태를 반영하므로 별도의 시각적 disable 없이도 정확성은 보장됩니다.To Reviewers
overlay-kit으로 여는 다른 모달/오버레이에도 같은 패턴(모달 오픈 시점에 고정된 콜백이 이후 상태 변화를 반영하지 못하는 문제)이 잠재해 있을 수 있습니다. 이번 PR에서는 재현이 확인된 수정 모달 플레이 버튼만 고쳤고, 다른 오버레이는 범위 밖입니다.Screenshot 📷
Test Checklist ✔
pnpm check-types통과pnpm lint통과