Fix(#264): 모바일/태블릿 Divider 터치 지원 및 캔버스 필기감 개선#265
Open
stringnine wants to merge 4 commits into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 Walkthrough워크스루이 PR은 두 가지 주요 입력 처리 개선을 포함합니다: 채팅 패널 divider의 터치 이벤트 지원 추가와 캔버스 드로잉 엔진의 포인터 압력 및 캡처 메커니즘 재구성입니다. Divider 터치 지원은 모바일/태블릿 환경에서 패널 리사이즈를 활성화하며, 캔버스 개선은 펜/지우개 도구의 선 품질과 입력 안정성을 강화합니다. 변경사항채팅 패널 Divider 터치 이벤트 지원
캔버스 드로잉 개선
시퀀스 다이어그램sequenceDiagram
participant User as 사용자
participant Divider as Divider
participant useResizable as useResizable
participant Browser as 브라우저
User->>Divider: 핸들 터치 시작
Divider->>useResizable: handleTouchStart(e)
useResizable->>Browser: preventDefault()
useResizable->>useResizable: isDragging=true
User->>Browser: 터치 드래그
Browser->>useResizable: touchmove 리스너
useResizable->>useResizable: RAF로 폭 갱신
User->>Browser: 터치 끝
Browser->>useResizable: touchend 리스너
useResizable->>useResizable: isDragging=false
sequenceDiagram
participant Canvas as 캔버스
participant handleDown as handlePointerDown
participant Document as 문서
participant handleUp as handlePointerUp
Canvas->>handleDown: 포인터다운
handleDown->>Canvas: setPointerCapture
handleDown->>Canvas: activeStroke 생성
Canvas->>Document: pointermove (캡처됨)
Document->>Document: 선 포인트 추가
Canvas->>Document: pointerup (캡처됨)
Document->>handleUp: 호출
handleUp->>Canvas: releasePointerCapture
handleUp->>Canvas: 선 커밋
예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~50분 제안 라벨
제안 리뷰어
시🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/features/editor/components/canvas/CanvasBoard.tsx (3)
1066-1084: 💤 Low value슬라이더
step=0.2인데 표시값은Math.round로 정수화됩니다.사용자가 슬라이더를 한 칸 움직여도 표시값이 동일하게 보이는 구간이 생겨 “반응 없음”처럼 느껴질 수 있습니다. 표시 정밀도와 step을 맞추는 편이 자연스럽습니다(예:
step={1}또는toFixed(1)).💄 제안
- step={0.2} + step={1}또는
toFixed(1)로 표시:- <span className="w-[28px] text-right tabular-nums"> - {Math.round(activeToolSize)} - </span> + <span className="w-[32px] text-right tabular-nums"> + {activeToolSize.toFixed(1)} + </span>🤖 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 `@src/features/editor/components/canvas/CanvasBoard.tsx` around lines 1066 - 1084, The displayed tool size is being rounded with Math.round(activeToolSize) while the slider uses step={0.2}, causing perceived non-responsiveness; update the display to match the slider precision (e.g., replace Math.round(activeToolSize) with activeToolSize.toFixed(1)) or change the slider step to 1 so step and display align, and ensure this reflects the same state used by setPenSize/setEraserSize and the activeToolSize variable for consistency.
168-177: 💤 Low value단일 포인트 정규화의
+0.01매직 넘버에 의도 주석을 추가하면 좋겠습니다.
perfect-freehand의getStroke가 단일 포인트에서 폴리곤을 만들기 위한 알려진 우회 처리지만, 의도가 코드만으로는 드러나지 않습니다. 또한 이 경로 결과는 보통StrokeShape의linePoints.length < 6가드에 의해DotShape로 떨어지는 것이 정상 흐름이라는 점도 함께 명시하면 후속 변경 시 회귀를 막기 좋습니다.🤖 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 `@src/features/editor/components/canvas/CanvasBoard.tsx` around lines 168 - 177, The normalization of single-point input in normalizedPoints currently adds a magic +0.01 offset without explanation; update the code near normalizedPoints to add a concise comment explaining that this small offset is a deliberate workaround for perfect-freehand's getStroke behavior to force a minimal polygon from a single point, and note that the resulting path is expected to be handled by the StrokeShape guard (linePoints.length < 6) and converted to a DotShape in normal flow to avoid regressions; reference points, normalizedPoints, getStroke, StrokeShape, DotShape, and linePoints.length < 6 in the comment so future maintainers understand intent.
763-779: ⚡ Quick win
document레벨pointermove에passive: false가 컴포넌트 마운트 동안 상시 부착됩니다.
passive: false리스너가document에 붙어 있는 한, 페이지의 모든pointermove에 대해 브라우저가 핸들러 완료를 기다린 뒤 스크롤/제스처를 진행합니다. 핸들러가 빠르게 early return 하더라도 전역 스크롤·터치 반응성에 마진을 깎아 먹는 패턴입니다(특히pointerup/pointercancel은 스크롤 측면에서preventDefault가 의미 있는 이벤트도 아닙니다).
setPointerCapture가 잡혀 있으면 캡처된 포인터의pointermove는 stage container로 전달되어bubble로 document에도 도달하므로, 스트로크 시작 시 부착 → 종료 시 분리 패턴이면 충분합니다.♻️ 제안 리팩터링
- useEffect(() => { - document.addEventListener("pointermove", handleDocumentPointerMove, { - passive: false, - }); - document.addEventListener("pointerup", handleDocumentPointerEnd, { - passive: false, - }); - document.addEventListener("pointercancel", handleDocumentPointerEnd, { - passive: false, - }); - - return () => { - document.removeEventListener("pointermove", handleDocumentPointerMove); - document.removeEventListener("pointerup", handleDocumentPointerEnd); - document.removeEventListener("pointercancel", handleDocumentPointerEnd); - }; - }, [handleDocumentPointerEnd, handleDocumentPointerMove]);대신
handlePointerDown의 pen/eraser 분기에서 캡처 직후 부착하고,handlePointerUp에서 해제하는 식으로 라이프사이클을 좁히는 것을 권장합니다. 부수적으로tool이 바뀔 때마다 핸들러 참조가 갱신되어 useEffect가 재실행되는 churn도 함께 사라집니다.🤖 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 `@src/features/editor/components/canvas/CanvasBoard.tsx` around lines 763 - 779, The document-level pointer listeners are attached for the entire component lifecycle with passive:false, hurting scroll/gesture responsiveness; instead, remove the global useEffect attachment and attach document.addEventListener("pointermove", handleDocumentPointerMove, {passive:false}) and pointerup/pointercancel to handleDocumentPointerEnd immediately after you call setPointerCapture in handlePointerDown (pen/eraser branch), and remove those same listeners in handlePointerUp/handleDocumentPointerEnd when the stroke ends; update handlePointerDown, handlePointerUp (and any capture logic) to manage adding/removing handleDocumentPointerMove and handleDocumentPointerEnd so listeners only exist during an active stroke.
🤖 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 `@src/features/chat/hooks/useChatPanel.ts`:
- Around line 22-35: activeTab currently always prefers the raw URL param
(panelParam) so clicking tabs doesn't change the visible tab because
handleTabChange only updates fallbackTab; modify handleTabChange (and any
tab-click handlers in useChatPanel) to update both fallbackTab via
setFallbackTab(newTab) and the URL searchParams (use the existing
searchParams/setSearchParams mechanism) so the "panel" query param is set to the
new tab (or removed/normalized when switching to the default viewer behavior),
and ensure closing/clearing viewer/file logic also removes the "panel" param so
activeTab and the URL stay in sync.
In `@src/features/chat/hooks/useResizable.ts`:
- Around line 67-70: handleTouchStart currently sets isDragging without
recording which touch to follow, causing jumpy behavior in multi-touch; modify
handleTouchStart to capture and store the initiating touch's identifier into
activeTouchIdRef (or create that ref if missing), update handleTouchMove to
locate the touch by comparing touch.identifier against activeTouchIdRef.current
using e.changedTouches or e.touches (ignore e.touches[0] directly), and clear
activeTouchIdRef.current = null inside handleTouchEnd (and stop dragging) so the
tracked identifier is reset; update references to activeTouchIdRef,
handleTouchMove, and handleTouchEnd accordingly.
- Around line 136-164: When cleaning up the useEffect that attaches drag
listeners (the effect depending on isDragging, handleMouseMove, handleMouseUp,
handleTouchMove, handleTouchEnd), also cancel any pending requestAnimationFrame
to avoid calling setWidth after unmount; add or use the RAF handle (e.g., rafId
or rafRef used by your drag logic) and call cancelAnimationFrame(rafId) in the
effect cleanup in addition to removing the event listeners and resetting
document styles so any scheduled RAF started during dragging is aborted on
unmount.
In `@src/features/editor/components/canvas/CanvasBoard.tsx`:
- Around line 635-651: handlePointerUp must accept an Event (or PointerEvent)
parameter and early-return unless event.pointerId === activePointerIdRef.current
so that only the pointer that started the stroke can end it; update the Stage
onPointerUp/onPointerCancel handlers to pass the event through to
handlePointerUp, keep handleDocumentPointerEnd calling handlePointerUp() with no
args (it already filters by pointerId) and retain the
releasePointerCapture/cleanup logic inside handlePointerUp using
activePointerIdRef/current; reference functions/refs: handlePointerUp,
handleDocumentPointerEnd, activePointerIdRef, stageRef.
---
Nitpick comments:
In `@src/features/editor/components/canvas/CanvasBoard.tsx`:
- Around line 1066-1084: The displayed tool size is being rounded with
Math.round(activeToolSize) while the slider uses step={0.2}, causing perceived
non-responsiveness; update the display to match the slider precision (e.g.,
replace Math.round(activeToolSize) with activeToolSize.toFixed(1)) or change the
slider step to 1 so step and display align, and ensure this reflects the same
state used by setPenSize/setEraserSize and the activeToolSize variable for
consistency.
- Around line 168-177: The normalization of single-point input in
normalizedPoints currently adds a magic +0.01 offset without explanation; update
the code near normalizedPoints to add a concise comment explaining that this
small offset is a deliberate workaround for perfect-freehand's getStroke
behavior to force a minimal polygon from a single point, and note that the
resulting path is expected to be handled by the StrokeShape guard
(linePoints.length < 6) and converted to a DotShape in normal flow to avoid
regressions; reference points, normalizedPoints, getStroke, StrokeShape,
DotShape, and linePoints.length < 6 in the comment so future maintainers
understand intent.
- Around line 763-779: The document-level pointer listeners are attached for the
entire component lifecycle with passive:false, hurting scroll/gesture
responsiveness; instead, remove the global useEffect attachment and attach
document.addEventListener("pointermove", handleDocumentPointerMove,
{passive:false}) and pointerup/pointercancel to handleDocumentPointerEnd
immediately after you call setPointerCapture in handlePointerDown (pen/eraser
branch), and remove those same listeners in
handlePointerUp/handleDocumentPointerEnd when the stroke ends; update
handlePointerDown, handlePointerUp (and any capture logic) to manage
adding/removing handleDocumentPointerMove and handleDocumentPointerEnd so
listeners only exist during an active stroke.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5bb248a-c624-43bd-bc19-02504043ce30
📒 Files selected for processing (5)
src/features/chat/components/divider/Divider.tsxsrc/features/chat/hooks/useChatPanel.tssrc/features/chat/hooks/useResizable.tssrc/features/editor/components/canvas/CanvasBoard.tsxsrc/pages/ChatPage.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈
🏷️ PR 타입
📝 작업 내용
Divider 터치 드래그 지원
useResizable.ts:touchstart/touchmove/touchend이벤트 핸들러 추가touchmove중 스크롤 방지 (passive: false)requestAnimationFrame으로 드래그 업데이트 throttle → 태블릿 버벅임 해소Divider.tsx:onTouchStartprop 추가useChatPanel.ts/ChatPage.tsx:handleTouchStart전달 연결캔버스 필기감 개선 (perfect-freehand 옵션 튜닝)
MIN_POINT_DISTANCE: 0.35 → 2.0 (포인트 수 감소, 계산 부하 개선)streamline: 0.32 → 0.38 (떨림 보정, 커밋 시 수축 현상 없는 범위)smoothing: 0.68 → 0.72thinning: 0.32 → 0.15 (굵기 변화 줄여 일관된 선 느낌)✅ 체크리스트
📎 기타 참고사항
Summary by CodeRabbit
릴리스 노트
New Features
Refactor