refactor: centralize gesture state reset in useTrackpadGesture#334
Open
upendra512 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
refactor: centralize gesture state reset in useTrackpadGesture#334upendra512 wants to merge 1 commit intoAOSSIE-Org:mainfrom
upendra512 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
…E-Org#281) Gesture state was reset in multiple scattered places across the hook, making it easy to miss a ref when adding new gesture state in the future. Introduce resetGestureState() which clears all gesture refs in one place: ongoingTouches, moved, releasedCount, dragging, lastPinchDist, pinching, and any pending draggingTimeout. Refactor handleTouchEnd to capture needed state before calling resetGestureState(), then dispatch drag release and tap events after. This keeps the same external behavior while making the reset path clear. Add handleTouchCancel that calls setIsTracking(false) + resetGestureState() as a clean handler for interrupted gestures (phone call, system interrupt). Expose onTouchCancel in the returned handlers object. Fixes AOSSIE-Org#281 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe changes introduce a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 270-273: handleTouchCancel currently only resets local state via
setIsTracking and resetGestureState, but if a drag was active it never notifies
the remote side that the button was released; update handleTouchCancel to detect
an active drag/pressed state and send the equivalent "press: false" remote event
before clearing local state. Use the same helper the touchend/touchup flow uses
to send button events (the function that emits mouse/press events in this
module) so the remote button state is cleared, then call setIsTracking(false)
and resetGestureState().
- Line 281: The hook useTrackpadGesture now returns onTouchCancel but
TouchArea.tsx still only types and binds onTouchStart/onTouchMove/onTouchEnd;
update the TouchArea component to include the onTouchCancel prop in its props
interface and JSX event bindings (add onTouchCancel={props.onTouchCancel} or
equivalent) and ensure any internal handler references (e.g., handleTouchStart,
handleTouchMove, handleTouchEnd) are extended to accept/call the provided
onTouchCancel so touch-cancel events trigger the same reset/cleanup logic as end
events.
🪄 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
Run ID: 664dfd07-be05-496f-8985-64bc5e15152b
📒 Files selected for processing (1)
src/hooks/useTrackpadGesture.ts
Comment on lines
+270
to
+273
| const handleTouchCancel = () => { | ||
| setIsTracking(false) | ||
| resetGestureState() | ||
| } |
Contributor
There was a problem hiding this comment.
Touch-cancel can leave remote drag/button state stuck.
If cancellation happens during an active drag, state is reset locally but no press: false event is sent. That can leave the remote side thinking left button is still down.
🐛 Proposed fix
const handleTouchCancel = () => {
+ const wasDragging = dragging.current
setIsTracking(false)
resetGestureState()
+ if (wasDragging) {
+ send({ type: "click", button: "left", press: false })
+ }
}📝 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.
Suggested change
| const handleTouchCancel = () => { | |
| setIsTracking(false) | |
| resetGestureState() | |
| } | |
| const handleTouchCancel = () => { | |
| const wasDragging = dragging.current | |
| setIsTracking(false) | |
| resetGestureState() | |
| if (wasDragging) { | |
| send({ type: "click", button: "left", press: false }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useTrackpadGesture.ts` around lines 270 - 273, handleTouchCancel
currently only resets local state via setIsTracking and resetGestureState, but
if a drag was active it never notifies the remote side that the button was
released; update handleTouchCancel to detect an active drag/pressed state and
send the equivalent "press: false" remote event before clearing local state. Use
the same helper the touchend/touchup flow uses to send button events (the
function that emits mouse/press events in this module) so the remote button
state is cleared, then call setIsTracking(false) and resetGestureState().
| onTouchStart: handleTouchStart, | ||
| onTouchMove: handleTouchMove, | ||
| onTouchEnd: handleTouchEnd, | ||
| onTouchCancel: handleTouchCancel, |
Contributor
There was a problem hiding this comment.
onTouchCancel exposure is not fully integrated in all consumers.
This hook now returns onTouchCancel, but src/components/Trackpad/TouchArea.tsx still types and binds only start/move/end handlers. In that path, cancel events will still bypass this reset logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useTrackpadGesture.ts` at line 281, The hook useTrackpadGesture now
returns onTouchCancel but TouchArea.tsx still only types and binds
onTouchStart/onTouchMove/onTouchEnd; update the TouchArea component to include
the onTouchCancel prop in its props interface and JSX event bindings (add
onTouchCancel={props.onTouchCancel} or equivalent) and ensure any internal
handler references (e.g., handleTouchStart, handleTouchMove, handleTouchEnd) are
extended to accept/call the provided onTouchCancel so touch-cancel events
trigger the same reset/cleanup logic as end events.
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.
Closes #281
Gesture state was reset in multiple scattered places, making it easy to miss a ref when adding new gesture state.
Changes:
Checklist:
Summary by CodeRabbit