Skip to content

Add rotated sheet gestures and coordinate conversion#166

Draft
thomasttvo wants to merge 66 commits into
masterfrom
thomas/extract-diana-rnzv-patch
Draft

Add rotated sheet gestures and coordinate conversion#166
thomasttvo wants to merge 66 commits into
masterfrom
thomas/extract-diana-rnzv-patch

Conversation

@thomasttvo
Copy link
Copy Markdown
Collaborator

Summary

Moves the Diana patch-package changes for @openspacelabs/react-native-zoomable-view into the reanimated library branch so Diana can consume them from the package instead of carrying patches/@openspacelabs+react-native-zoomable-view+2.4.3.patch.

This PR is stacked on #151. It adds the sheet-home APIs needed by Diana: rotated content transforms, optional pinch panning, two-finger rotation callbacks, overlay content, momentum-end callback support, and rotated coordinate conversion.

Test Plan

  • In Diana sheet-first home, open a sheet with the reanimated zoomable view enabled.
  • Pinch, pan, and two-finger rotate the sheet; confirm rotation, zoom anchor behavior, and follow/non-follow panning match the current patched behavior.
  • Tap the sheet and static pin flows that depend on coordinate conversion; confirm selected positions land on the intended sheet coordinates.
  • Spot-check existing zoomable-view consumers that do not pass contentRotation, pinchPanEnabled, or onRotation to confirm default pan/zoom behavior still works.

Risk

  • Gesture behavior is the main risk because this changes pinch panning, momentum decay, and single-tap handling in the shared zoomable view.
  • Rotated coordinate conversion affects Diana sheet-home hit testing and static pin position callbacks.
  • Removing the package patch in Diana should wait until this stacked PR and PR Reanimated #151 are consumed together.

Change details

  • Adds contentRotation, pinchPanEnabled, onRotation, overlayContent, and onMomentumEnd to the public props.
  • Applies content-rotation-aware pan, pinch zoom, tap-to-pan, and viewport-to-image coordinate math.
  • Fixes pinch distance calculation to use Y deltas, adds touch-angle calculation, and switches zoom subject measurement to layout dimensions so transforms do not distort the source size.

elliottkember and others added 16 commits January 6, 2026 13:28
…e-define

# Conflicts:
#	.github/workflows/lint.yml
#	lib/commonjs/ReactNativeZoomableView.js
#	lib/commonjs/ReactNativeZoomableView.js.map
#	lib/commonjs/animations/index.js
#	lib/commonjs/animations/index.js.map
#	lib/commonjs/components/AnimatedTouchFeedback.js
#	lib/commonjs/components/AnimatedTouchFeedback.js.map
#	lib/commonjs/components/StaticPin.js
#	lib/commonjs/components/StaticPin.js.map
#	lib/commonjs/components/index.js
#	lib/commonjs/components/index.js.map
#	lib/commonjs/debugHelper/index.js
#	lib/commonjs/debugHelper/index.js.map
#	lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js
#	lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js.map
#	lib/commonjs/helper/coordinateConversion.js
#	lib/commonjs/helper/coordinateConversion.js.map
#	lib/commonjs/helper/index.js
#	lib/commonjs/helper/index.js.map
#	lib/commonjs/index.js
#	lib/commonjs/index.js.map
#	lib/commonjs/typings/index.js
#	lib/commonjs/typings/index.js.map
#	lib/module/ReactNativeZoomableView.js
#	lib/module/ReactNativeZoomableView.js.map
#	lib/module/animations/index.js
#	lib/module/animations/index.js.map
#	lib/module/components/AnimatedTouchFeedback.js
#	lib/module/components/AnimatedTouchFeedback.js.map
#	lib/module/components/StaticPin.js
#	lib/module/components/StaticPin.js.map
#	lib/module/components/index.js
#	lib/module/components/index.js.map
#	lib/module/debugHelper/index.js
#	lib/module/debugHelper/index.js.map
#	lib/module/helper/calcNewScaledOffsetForZoomCentering.js
#	lib/module/helper/calcNewScaledOffsetForZoomCentering.js.map
#	lib/module/helper/coordinateConversion.js
#	lib/module/helper/coordinateConversion.js.map
#	lib/module/helper/index.js
#	lib/module/helper/index.js.map
#	lib/module/index.js
#	lib/module/index.js.map
#	lib/module/typings/index.js
#	lib/module/typings/index.js.map
#	lib/typescript/ReactNativeZoomableView.d.ts
#	lib/typescript/animations/index.d.ts
#	lib/typescript/components/AnimatedTouchFeedback.d.ts
#	lib/typescript/components/StaticPin.d.ts
#	lib/typescript/components/index.d.ts
#	lib/typescript/debugHelper/index.d.ts
#	lib/typescript/helper/calcNewScaledOffsetForZoomCentering.d.ts
#	lib/typescript/helper/coordinateConversion.d.ts
#	lib/typescript/helper/index.d.ts
#	lib/typescript/index.d.ts
#	lib/typescript/typings/index.d.ts
#	package.json
#	src/ReactNativeZoomableView.tsx
#	src/animations/index.ts
#	yarn.lock
PR #158 removed checked-in lib/ and added it to .gitignore.
These remaining tracked files are now redundant.
# Conflicts:
#	.github/workflows/lint.yml
#	example/App.tsx
#	example/metro.config.js
#	example/package.json
#	example/yarn.lock
#	lib/commonjs/ReactNativeZoomableView.js
#	lib/commonjs/ReactNativeZoomableView.js.map
#	lib/commonjs/animations/index.js
#	lib/commonjs/animations/index.js.map
#	lib/commonjs/components/AnimatedTouchFeedback.js
#	lib/commonjs/components/AnimatedTouchFeedback.js.map
#	lib/commonjs/components/StaticPin.js
#	lib/commonjs/components/StaticPin.js.map
#	lib/commonjs/components/index.js
#	lib/commonjs/components/index.js.map
#	lib/commonjs/debugHelper/index.js
#	lib/commonjs/debugHelper/index.js.map
#	lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js
#	lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js.map
#	lib/commonjs/helper/coordinateConversion.js
#	lib/commonjs/helper/coordinateConversion.js.map
#	lib/commonjs/helper/index.js
#	lib/commonjs/helper/index.js.map
#	lib/commonjs/index.js
#	lib/commonjs/index.js.map
#	lib/commonjs/typings/index.js
#	lib/commonjs/typings/index.js.map
#	lib/module/ReactNativeZoomableView.js
#	lib/module/ReactNativeZoomableView.js.map
#	lib/module/animations/index.js
#	lib/module/animations/index.js.map
#	lib/module/components/AnimatedTouchFeedback.js
#	lib/module/components/AnimatedTouchFeedback.js.map
#	lib/module/components/StaticPin.js
#	lib/module/components/StaticPin.js.map
#	lib/module/components/index.js
#	lib/module/components/index.js.map
#	lib/module/debugHelper/index.js
#	lib/module/debugHelper/index.js.map
#	lib/module/helper/calcNewScaledOffsetForZoomCentering.js
#	lib/module/helper/calcNewScaledOffsetForZoomCentering.js.map
#	lib/module/helper/coordinateConversion.js
#	lib/module/helper/coordinateConversion.js.map
#	lib/module/helper/index.js
#	lib/module/helper/index.js.map
#	lib/module/index.js
#	lib/module/index.js.map
#	lib/module/typings/index.js
#	lib/module/typings/index.js.map
#	lib/typescript/ReactNativeZoomableView.d.ts
#	lib/typescript/animations/index.d.ts
#	lib/typescript/components/AnimatedTouchFeedback.d.ts
#	lib/typescript/components/StaticPin.d.ts
#	lib/typescript/components/index.d.ts
#	lib/typescript/debugHelper/index.d.ts
#	lib/typescript/helper/calcNewScaledOffsetForZoomCentering.d.ts
#	lib/typescript/helper/coordinateConversion.d.ts
#	lib/typescript/helper/index.d.ts
#	lib/typescript/index.d.ts
#	lib/typescript/typings/index.d.ts
#	package.json
#	src/ReactNativeZoomableView.tsx
#	src/animations/index.ts
#	src/components/StaticPin.tsx
#	src/helper/index.ts
#	src/index.tsx
#	src/typings/index.ts
#	yarn.lock
PR #158 removed checked-in lib/ and added it to .gitignore.
These remaining tracked files are now redundant.
# Conflicts:
#	.github/workflows/lint.yml
#	lib/commonjs/ReactNativeZoomableView.js
#	lib/commonjs/ReactNativeZoomableView.js.map
#	lib/commonjs/animations/index.js
#	lib/commonjs/animations/index.js.map
#	lib/commonjs/components/AnimatedTouchFeedback.js
#	lib/commonjs/components/AnimatedTouchFeedback.js.map
#	lib/commonjs/components/StaticPin.js
#	lib/commonjs/components/StaticPin.js.map
#	lib/commonjs/components/index.js
#	lib/commonjs/components/index.js.map
#	lib/commonjs/debugHelper/index.js
#	lib/commonjs/debugHelper/index.js.map
#	lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js
#	lib/commonjs/helper/calcNewScaledOffsetForZoomCentering.js.map
#	lib/commonjs/helper/coordinateConversion.js
#	lib/commonjs/helper/coordinateConversion.js.map
#	lib/commonjs/helper/index.js
#	lib/commonjs/helper/index.js.map
#	lib/commonjs/index.js
#	lib/commonjs/index.js.map
#	lib/commonjs/typings/index.js
#	lib/commonjs/typings/index.js.map
#	lib/module/ReactNativeZoomableView.js
#	lib/module/ReactNativeZoomableView.js.map
#	lib/module/animations/index.js
#	lib/module/animations/index.js.map
#	lib/module/components/AnimatedTouchFeedback.js
#	lib/module/components/AnimatedTouchFeedback.js.map
#	lib/module/components/StaticPin.js
#	lib/module/components/StaticPin.js.map
#	lib/module/components/index.js
#	lib/module/components/index.js.map
#	lib/module/debugHelper/index.js
#	lib/module/debugHelper/index.js.map
#	lib/module/helper/calcNewScaledOffsetForZoomCentering.js
#	lib/module/helper/calcNewScaledOffsetForZoomCentering.js.map
#	lib/module/helper/coordinateConversion.js
#	lib/module/helper/coordinateConversion.js.map
#	lib/module/helper/index.js
#	lib/module/helper/index.js.map
#	lib/module/index.js
#	lib/module/index.js.map
#	lib/module/typings/index.js
#	lib/module/typings/index.js.map
#	lib/typescript/ReactNativeZoomableView.d.ts
#	lib/typescript/animations/index.d.ts
#	lib/typescript/components/AnimatedTouchFeedback.d.ts
#	lib/typescript/components/StaticPin.d.ts
#	lib/typescript/components/index.d.ts
#	lib/typescript/debugHelper/index.d.ts
#	lib/typescript/helper/calcNewScaledOffsetForZoomCentering.d.ts
#	lib/typescript/helper/coordinateConversion.d.ts
#	lib/typescript/helper/index.d.ts
#	lib/typescript/index.d.ts
#	lib/typescript/typings/index.d.ts
#	package.json
#	src/ReactNativeZoomableView.tsx
#	src/animations/index.ts
#	yarn.lock
PR #158 removed checked-in lib/ and added it to .gitignore.
These remaining tracked files are now redundant.
# Conflicts:
#	README.md
#	src/ReactNativeZoomableView.tsx
# Conflicts:
#	.github/workflows/lint.yml
#	src/ReactNativeZoomableView.tsx
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o thomas/reanimated

# Conflicts:
#	.github/workflows/lint.yml
#	README.md
#	package.json
#	src/ReactNativeZoomableView.tsx
#	src/animations/index.ts
#	src/components/StaticPin.tsx
@cursor
Copy link
Copy Markdown

cursor Bot commented Apr 29, 2026

PR Summary

Medium Risk
Touches core gesture/transform code paths (pan/pinch/decay/rotation) and coordinate conversion, so regressions are possible across existing consumers even though defaults aim to preserve prior behavior.

Overview
Adds rotation support end-to-end: contentRotation now rotates the zoom subject, pans/zoom-centering/tap-to-pan deltas are counter-rotated, and viewportPositionToImagePosition accepts contentRotation to keep hit-testing/static pin coordinates correct under rotation.

Expands gesture capabilities and APIs: introduces pinchPanEnabled (toggle pinch-pan vs “follow static pin” zoom anchoring), a two-finger onRotation(deltaRadians, fingerDist) callback, momentum scrolling via withDecay plus onMomentumEnd, and includes gestureType in ZoomableViewEvent.

Improves behavior/perf details: fixes pinch distance Y-delta bug and adds touch-angle helper, changes zoom-subject measurement to prefer onLayout (removing periodic re-measure), throttles static pin JS-bridge updates during active gestures, allows onSingleTap to consume the event to skip auto pan-to-tap, adds overlayContent rendered outside the zoom transform, and updates moveTo to optionally override zoom and cancel in-flight animations before jumping.

Reviewed by Cursor Bugbot for commit 10d48c4. Configure here.

@thomasttvo thomasttvo force-pushed the thomas/extract-diana-rnzv-patch branch from 10d48c4 to f3a60b8 Compare April 29, 2026 01:08
@thomasttvo thomasttvo changed the title Extract Diana zoomable view patch Add rotated sheet gestures and coordinate conversion Apr 29, 2026
@thomasttvo
Copy link
Copy Markdown
Collaborator Author

@claude review

@thomasttvo thomasttvo marked this pull request as draft April 29, 2026 01:09
Move the Diana patch-package changes into the reanimated zoomable-view branch so Diana can consume the library directly instead of carrying a local patch.

Co-Authored-By: Thomas Vo <2142140+thomasttvo@users.noreply.github.com>

Co-Authored-By: Codex <codex@openai.com>
@thomasttvo thomasttvo force-pushed the thomas/extract-diana-rnzv-patch branch from f3a60b8 to df11338 Compare April 29, 2026 01:10
Comment on lines +590 to +598
// Track velocity for momentum scrolling
const now = Date.now();
const dt = now - lastTouchTimestamp.value;
if (dt > 0 && dt < 100) {
velocityX.value = shift.x / (dt / 1000);
velocityY.value = shift.y / (dt / 1000);
}
lastTouchTimestamp.value = now;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Stale velocity causes an unwanted fling after a pan-pause-release. In _handleShifting, velocityX/velocityY are only updated when dt < 100, but lastTouchTimestamp is bumped unconditionally — so if the user pans quickly, holds still for >100ms, then lifts, _handlePanResponderEnd reads the pre-pause velocity and dispatches withDecay, flinging the view contrary to the user's clear stop-then-release intent. Fix by zeroing the velocities in the else branch (or using a small recent-frames ring buffer / EMA) so a long pause discards stale velocity.

Extended reasoning...

What goes wrong

The new momentum-decay path in _handlePanResponderEnd reads velocityX/velocityY and, when the resulting speed exceeds 50 px/s, dispatches withDecay for both axes. Those velocities are computed in _handleShifting:

const now = Date.now();
const dt = now - lastTouchTimestamp.value;
if (dt > 0 && dt < 100) {
  velocityX.value = shift.x / (dt / 1000);
  velocityY.value = shift.y / (dt / 1000);
}
lastTouchTimestamp.value = now;

The dt < 100 guard is there to avoid dividing tiny shift values by a huge dt (which would manufacture phantom velocity from a slow drag). That part is fine. The bug is the asymmetry: when dt >= 100, the velocities are not updated, but lastTouchTimestamp is still bumped, and there is no else clause that zeros them. So whatever the velocity was on the last <100 ms frame before the pause is carried verbatim into _handlePanResponderEnd.

Why existing code does not prevent it

Velocities are reset to 0 only after withDecay is dispatched (lines 956–957 in the modified file). Nothing resets them at the start of a new gesture, on _handlePanResponderGrant, or when a long pause is detected. While the finger is stationary, onTouchesMove does not fire (Gesture.Manual only fires move events on actual movement), so _handleShifting is not called at all during the pause — the cached velocity simply sits in the shared value until release.

Step-by-step proof

  1. User starts a single-finger pan; _handlePanResponderGrant runs, but does not reset velocities. lastTouchTimestamp is 0.
  2. First move frame at t=0 ms: dt = 0 - 0 = 0, fails the dt > 0 guard, velocity not updated, lastTouchTimestamp = 0. Fine — velocities are still 0 from the last gesture's reset.
  3. Subsequent fast pan frames at t=16 ms, 32 ms, 48 ms, … each have dt ≈ 16 ms with a meaningful shift, so velocityX/Y get set to ~500 px/s and lastTouchTimestamp advances each frame.
  4. At t=200 ms the user stops moving but keeps the finger down. onTouchesMove does not fire while the finger is stationary, so _handleShifting is not called. velocityX/Y remain at ~500 px/s and lastTouchTimestamp stays at 200 ms.
  5. At t=600 ms the user lifts. _handlePanResponderEnd runs, computes speed = sqrt(500² + 500²) ≈ 707 > 50, and dispatches withDecay({ velocity: 500, deceleration: 0.997 }) on both axes.
  6. The view flings as if the user had been moving at 500 px/s right up until the lift — directly contradicting the user's pause-then-release intent.

Even if a tiny stray move event sneaks in during the slow phase, it generally has both small shift and small dt, which writes a small but still possibly stale-direction velocity. The clean pause-then-release path (no movement events during the pause) is the worst case.

Impact

This is a UX regression visible to anyone who pans-and-holds-then-releases — a very common gesture pattern (e.g. dragging to a target and stopping precisely before lifting). The view will overshoot the user's deliberate stopping point.

Fix

The minimal fix is to add an else branch that zeros velocity on long pauses, so a stationary finger discards stale velocity:

const now = Date.now();
const dt = now - lastTouchTimestamp.value;
if (dt > 0 && dt < 100) {
  velocityX.value = shift.x / (dt / 1000);
  velocityY.value = shift.y / (dt / 1000);
} else if (dt >= 100) {
  velocityX.value = 0;
  velocityY.value = 0;
}
lastTouchTimestamp.value = now;

Note this still only catches the case where some tiny onTouchesMove event fires after the pause. A more robust fix is a small ring buffer / EMA over recent frames, or sampling velocity from the last ~50 ms of motion at release time inside _handlePanResponderEnd rather than carrying a single most-recent-frame value across the whole gesture.

Comment on lines 1006 to +1012
} else if (e.numberOfTouches === 1) {
// Don't downgrade from pinch to shift when lifting one finger
if (gestureType.value === 'pinch') {
// Reset rotation state so next 2-finger contact starts fresh
lastGestureTouchAngle.value = null;
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Lifting one finger after a pinch now leaves gestureType stuck at 'pinch' and the early return skips the shift branch entirely. Because _handlePinching only does work when numberOfTouches === 2, the view freezes between the lift and full release, breaking the common "pinch-zoom then continue panning with one finger" pattern (maps/photos) that SPECS.md §Gesture-to-Shift Transition documents. The narrower fix is to clear lastGestureTouchAngle only and fall through to the shift branch (which already recomputes lastGestureCenterPosition to prevent a jump).

Extended reasoning...

The regression

_handlePanResponderMove in src/ReactNativeZoomableView.tsx (around lines 1006-1012) now contains:

} else if (e.numberOfTouches === 1) {
  // Don't downgrade from pinch to shift when lifting one finger
  if (gestureType.value === 'pinch') {
    // Reset rotation state so next 2-finger contact starts fresh
    lastGestureTouchAngle.value = null;
    return;
  }
  ...
}

Previously, when the user pinched and then lifted one finger, control fell through into the 1-finger branch. That branch recomputes lastGestureCenterPosition, and once |dx|>2 or |dy|>2 it sets gestureType.value = 'shift' and calls _handleShifting(e), so the user could continue panning with the remaining finger. SPECS.md §'Gesture-to-Shift Transition' (line 181) explicitly documents this: "When switching from pinch to shift (or vice versa), lastGestureCenterPosition is recalculated at the transition boundary to prevent a jump."

Why the early return freezes the view

gestureType.value is only reset to undefined inside _handlePanResponderEnd, and that handler is only invoked from Gesture.Manual().onTouchesUp when e.numberOfTouches === 0. So after lifting one of two fingers, gestureType.value stays 'pinch' for every subsequent move event, and the new early return exits before any work is done.

_handlePinching is not a fallback either — it bails out at the top because calcGestureTouchDistance and calcGestureCenterPoint both require numberOfTouches === 2 and return null otherwise. The net effect: zero motion is applied to offsetX/offsetY/zoom between the lift and full release.

Step-by-step proof

  1. User puts down two fingers — onTouchesDown fires, gestureType.value becomes undefined.
  2. User pinches — _handlePanResponderMove enters the numberOfTouches === 2 branch, sets gestureType.value = 'pinch', calls _handlePinching. View zooms.
  3. User lifts one finger. onTouchesUp fires but does not call _handlePanResponderEnd because numberOfTouches is 1, not 0. gestureType.value remains 'pinch'.
  4. User drags the remaining finger. _handlePanResponderMove enters the numberOfTouches === 1 branch, hits if (gestureType.value === 'pinch'), clears lastGestureTouchAngle, and returns. No shifting.
  5. Repeat step 4 for every subsequent touch-move event. The view appears frozen.
  6. User finally lifts the last finger — numberOfTouches reaches 0, _handlePanResponderEnd runs, gestureType.value is reset to undefined. Only now is the gesture released.

User-visible impact

The classic "pinch to zoom on a map, lift one finger, keep dragging to recenter" interaction is broken. This is one of the most common multi-touch patterns on mobile (Maps, Photos, any zoomable image viewer), and the previous implementation explicitly supported it.

How to fix

The added comment "Reset rotation state so next 2-finger contact starts fresh" suggests the intent was to avoid stale lastGestureTouchAngle carrying into a future pinch. That concern is legitimate, but the cure is overbroad. The minimal fix is to drop the return and let control fall through:

} else if (e.numberOfTouches === 1) {
  // Reset rotation state so next 2-finger contact starts fresh
  if (gestureType.value === 'pinch') {
    lastGestureTouchAngle.value = null;
  }

  const { dx, dy } = gestureState;
  ...
}

The existing if (gestureType.value !== 'shift') reset of lastGestureCenterPosition further down already handles the pinch→shift transition boundary correctly, so falling through preserves both the rotation reset and the SPECS-documented continuation behavior.

This was confirmed independently by four runtime verifiers, all of which traced the same code path and reached the same conclusion.

Comment on lines +284 to +292
if (position && gestureStarted.value) {
// Throttle JS bridge calls for pin position to avoid per-frame overhead
// Only bridge during active gestures; skip during decay/animation.
const now = Date.now();
if (now - lastPinBridgeTimestamp.value > 200) {
lastPinBridgeTimestamp.value = now;
onStaticPinPositionMove?.(position);
runOnJS(debouncedOnStaticPinPositionChange)(position);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The new gestureStarted.value gate plus 200ms throttle on the pin-bridge call (src/ReactNativeZoomableView.tsx:284-292) silently drops onStaticPinPositionMove and the debounced onStaticPinPositionChange for many fire paths the prior code (and SPECS.md "Pin Position Updates") guarantees: withDecay momentum frames (this very PR sets gestureStarted=false synchronously before the decay animation runs), programmatic moveTo/moveBy/zoomTo animations, the single-tap 200ms pan-to-pin animation, and staticPinPosition/layout reactions. Even within active gestures, the throttle reduces documented per-frame updates from ~60Hz to ~5Hz. Additionally lastPinBridgeTimestamp is never reset across gestures, so the first frame of a new gesture started within 200ms of the previous bridge is also dropped. Either drop the gestureStarted/throttle gating in favour of the existing 100ms debouncedOnStaticPinPositionChange, or reset lastPinBridgeTimestamp on grant/end and add explicit fire paths for the regressed cases.

Extended reasoning...

What is broken

The new _invokeOnTransform body at src/ReactNativeZoomableView.tsx:284-292 replaced the unconditional pin-bridge with:

if (position && gestureStarted.value) {
  const now = Date.now();
  if (now - lastPinBridgeTimestamp.value > 200) {
    lastPinBridgeTimestamp.value = now;
    onStaticPinPositionMove?.(position);
    runOnJS(debouncedOnStaticPinPositionChange)(position);
  }
}

This is invoked from a useAnimatedReaction keyed on the full _getZoomableViewEventObject(), so every change to offsetX/offsetY/zoom (including from withDecay, withTiming, programmatic setters, and prop-driven useLayoutEffects) re-runs the gate. The function-level comment ("Only bridge during active gestures; skip during decay/animation") acknowledges the change but no compensating fire paths are added.

Documented contract that is now silently dropped

SPECS.md "Pin Position Updates" documents onStaticPinPositionMove as firing on every _invokeOnTransform tick — explicitly including programmatic moveTo/moveBy/moveStaticPinTo, staticPinPosition prop-change useLayoutEffect, layout reactions, and the single-tap 200ms pan-to-pin animation. The debounced onStaticPinPositionChange is documented to fire from the same surface.

Four concrete regressed paths:

  1. withDecay momentum frames. _handlePanResponderEnd schedules withDecay for offsetX/offsetY and then synchronously sets gestureStarted.value = false (last line of the function). withDecay's frames execute after that, so every momentum tick now satisfies !gestureStarted.value and is skipped. There is no compensating call at decay completion — only onMomentumEnd() fires, not the pin callbacks.
  2. Programmatic publicMoveTo/publicMoveBy/publicZoomTo. None of these set gestureStarted=true, so any change they make to offsetX/offsetY/zoom produces a _invokeOnTransform tick that fails the gate and drops the bridge.
  3. Single-tap 200ms pan-to-pin animation (_resolveAndHandleTap). The setTimeout(..., doubleTapDelay) fires after _handlePanResponderEnd has already set gestureStarted=false. The 200ms withTiming animation runs entirely with the gate closed; only the terminal _updateStaticPin() in the done callback still fires (and it only invokes onStaticPinPositionChange, not onStaticPinPositionMove).
  4. staticPinPosition prop change useLayoutEffect and the layout useAnimatedReaction. Both can call _invokeOnTransform outside any gesture; both are now dropped.

Throttle inside active gestures

Even the active-gesture path is now throttled to 5 Hz. Per SPECS.md, onStaticPinPositionMove is a worklet (Called on the UI thread. Must be a worklet.) so it has no JS-bridge cost — a 200ms cap on a UI-thread worklet contradicts both the spec and the stated rationale of the comment.

Stale-timestamp first-frame drop (bug_006)

lastPinBridgeTimestamp is useSharedValue<number>(0) and is only ever assigned via Date.now() inside the throttle. It is never reset in _handlePanResponderGrant or _handlePanResponderEnd. So if gesture A bridges at time T and gesture B's first frame fires at T+50ms (rapid pinch→pan, double-pinch), the check now - lastPinBridgeTimestamp.value > 200 fails and the first onStaticPinPositionMove/debouncedOnStaticPinPositionChange of gesture B is dropped — the consumer-visible pin lags up to one throttle window into the new gesture.

Step-by-step proof (decay path)

  1. User performs a pan-flick. _handlePanResponderGrant sets gestureStarted.value = true. Frames during the active drag bridge (subject to the 5 Hz throttle).
  2. Finger lifts. _handlePanResponderEnd runs:
    • gestureType === 'shift', speed > 50, so offsetX.value = withDecay(...) and offsetY.value = withDecay(...).
    • At the bottom of the function: gestureStarted.value = false.
  3. The next animation frame ticks. withDecay updates offsetX.value. The useAnimatedReaction re-runs _invokeOnTransform. gestureStarted.value is now false, so the if (position && gestureStarted.value) branch is skipped. The pin-bridge is silently dropped for every decay frame.
  4. Decay completes. The (finished) => { if (finished) checkMomentumEnd(); } callback fires onMomentumEnd() once both axes finish — but this never invokes onStaticPinPositionMove or onStaticPinPositionChange. The consumer's pin overlay is stuck at the last position bridged during the active drag.

Why existing code does not save us

_updateStaticPin() is called at gesture end and at single-tap withTiming completion, but: (a) it only fires onStaticPinPositionChange (not onStaticPinPositionMove); (b) it is not called at withDecay completion; (c) it is not called from any of publicMoveTo/publicMoveBy/publicZoomTo. The 100 ms debouncedOnStaticPinPositionChange already exists and is sufficient for batching JS-bridge work without the additional 200 ms shared-value throttle and worklet-side gate.

Suggested fix

Drop the gestureStarted/200ms throttle gating entirely and rely on the existing 100 ms lodash debounce on debouncedOnStaticPinPositionChange; onStaticPinPositionMove is a worklet and does not need throttling. If the throttle is kept, at minimum reset lastPinBridgeTimestamp.value = 0 in _handlePanResponderGrant/_handlePanResponderEnd and add fire paths for decay completion, the programmatic public API, and the single-tap pan animation.

Addressing the bug_006 refutation

The verifier who refuted bug_006 is correct that, in isolation, the first-frame stale-timestamp drop is microscopic (≤200 ms, on a callback whose JS-bridge half is already 100 ms-debounced). It is being merged here only as a strict subset of the broader regression: any fix that removes the gestureStarted/200 ms gate also removes it; any fix that keeps the gate must also reset the timestamp. It is not a separate review comment.

Comment on lines +927 to +946
};
offsetX.value = withDecay(
{
velocity: vx,
deceleration: 0.997,
},
(finished) => {
'worklet';
if (finished) checkMomentumEnd();
}
);
offsetY.value = withDecay(
{
velocity: vy,
deceleration: 0.997,
},
(finished) => {
'worklet';
if (finished) checkMomentumEnd();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The new onMomentumEnd callback never fires when decay is interrupted by a new gesture, which is the most common momentum-end scenario (tap-during-fling). Because _handlePanResponderGrant calls cancelAnimation(offsetX) and cancelAnimation(offsetY) on every new touch, both withDecay callbacks are invoked with finished=false and the if (finished) guard skips checkMomentumEnd for both, leaving consumers driving state machines stuck in a "decaying" state indefinitely. Fix by also firing onMomentumEnd on the cancellation path (e.g., drop the if (finished) guard or fire it from _handlePanResponderGrant when a decay is in flight).

Extended reasoning...

Bug

onMomentumEnd is a new public callback added in this PR to signal that decay/momentum has finished. It is wired up in _handlePanResponderEnd (src/ReactNativeZoomableView.tsx:917-944):

let decayFinishedCount = 0;
const checkMomentumEnd = () => {
  decayFinishedCount++;
  if (decayFinishedCount >= 2) runOnJS(onMomentumEnd)();
};
offsetX.value = withDecay({ velocity: vx, deceleration: 0.997 }, (finished) => {
  if (finished) checkMomentumEnd();
});
offsetY.value = withDecay({ velocity: vy, deceleration: 0.997 }, (finished) => {
  if (finished) checkMomentumEnd();
});

The completion callbacks gate on if (finished). Per Reanimated semantics, cancelAnimation() invokes the completion callback with finished=false, so any callback that gates on finished is skipped on cancellation.

Trigger Path

_handlePanResponderGrant runs at the start of every new touch and unconditionally cancels in-flight animations on offsetX/offsetY/zoom:

const _handlePanResponderGrant = (e) => {
  // ...
  cancelAnimation(zoom);
  cancelAnimation(offsetX);
  cancelAnimation(offsetY);
  gestureStarted.value = true;
};

So whenever the user touches the view while decay momentum is in progress (the canonical "interrupt the fling" gesture), both withDecay callbacks fire with finished=false, neither increments decayFinishedCount, and onMomentumEnd is never called for that decay.

Step-by-step proof

  1. User pans hard and lifts → _handlePanResponderEnd runs, computes speed > 50, schedules two withDecay calls on offsetX and offsetY, and synchronously fires onShiftingEnd.
  2. From the consumer's perspective, the view is now in a "decaying" state and they are waiting on onMomentumEnd to transition to "idle".
  3. Before the decay settles, the user taps the view again. onTouchesDown activates the gesture and _handlePanResponderGrant runs.
  4. _handlePanResponderGrant calls cancelAnimation(offsetX) and cancelAnimation(offsetY). Reanimated invokes both withDecay completion callbacks with finished=false.
  5. Both callbacks short-circuit on if (finished) checkMomentumEnd()decayFinishedCount stays at 0, and onMomentumEnd is not called.
  6. The consumer's state machine remains in "decaying" with no signal that the chapter is over. onShiftingEnd already fired in step 1, so it cannot serve as the recovery signal either. The state is stuck until the user triggers another gesture that ends with a slow release (the else branch where speed <= 50 calls onMomentumEnd synchronously) or a high-velocity decay that runs to natural completion.

Why existing code does not prevent it

The completion callbacks intentionally short-circuit on finished=false, but the comment trail in the code shows the design intent is that onMomentumEnd terminates the decay state — the else branch when speed <= 50 calls onMomentumEnd directly without any animation, confirming that the callback is meant to fire exactly once per shift-end. There is no guard in _handlePanResponderGrant that fires onMomentumEnd when it cancels a live decay, and there is no separate "shift-or-momentum-end" higher-level signal consumers can use instead.

Impact

onMomentumEnd is a brand new public API documented in the PR description as "momentum-end callback support". Its contract is broken in the most common interruption case (tap or pan during fling), which is the standard gesture pattern for scrollable surfaces. Any consumer using it to drive state — for example, gating snap-after-momentum or marking decay as complete for the sheet-home rotated content the PR description targets — will leak into a perpetual "decaying" state and never recover.

Suggested fix

Either drop the if (finished) guard so cancellation also counts toward checkMomentumEnd:

offsetX.value = withDecay({...}, () => { checkMomentumEnd(); });
offsetY.value = withDecay({...}, () => { checkMomentumEnd(); });

…or fire onMomentumEnd from _handlePanResponderGrant when it detects an in-flight decay (e.g., a decayInFlight shared value set when scheduling withDecay and cleared on natural completion). The first option is simpler and matches the symmetry of the slow-release path that already calls onMomentumEnd unconditionally.

onStaticPinPositionChange: undefined,
onStaticPinPositionMove: undefined,
disablePanOnInitialZoom: false,
pinchPanEnabled: true,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Default pinchPanEnabled: true silently breaks static-pin zoom anchoring. The previous code unconditionally used staticPinPosition as the pinch zoom center when a pin was set; the new gate if (staticPinPosition.value && !pinchPanEnabled.value) combined with the default pinchPanEnabled: true set in the defaults() block at line 100 means every existing consumer that passes staticPinPosition without explicitly setting pinchPanEnabled={false} loses the documented "pin stays under fingers during pinch" behavior on upgrade. Fix by defaulting pinchPanEnabled to false (preserve prior behavior) or by inverting the gate so static-pin anchoring remains the default when a pin is set.

Extended reasoning...

What the bug is

This PR adds a new prop pinchPanEnabled and gates the static-pin zoom-center anchoring on it. The defaults block at src/ReactNativeZoomableView.tsx:100 sets pinchPanEnabled: true, and _handlePinching now reads:

if (staticPinPosition.value && !pinchPanEnabled.value) {
  // Follow mode: zoom centres on the static pin position.
  zoomCenter = { x: staticPinPosition.value.x, y: staticPinPosition.value.y };
}

Previously the same site read if (staticPinPosition.value) unconditionally — with the deleted comment "When we use a static pin position, the zoom centre is the same as that position, otherwise the pin moves around way too much while zooming." That last clause is exactly the regression existing consumers will hit.

Why this is a contract break

SPECS.md documents the prior behavior in two places, and SPECS.md is not modified in this PR:

  • Line 77 (staticPinPosition prop row): "used directly as the pinch zoom center"
  • Line 188 (Pinch-to-Zoom section): "Zoom center = midpoint of two touches (or staticPinPosition if pin is active — keeps pin stable during zoom)"

Both statements are unconditional on pinchPanEnabled. So the spec promises pin-anchored zoom whenever a pin is set, but after this PR the default code path no longer honors that.

Why existing code does not prevent it

There is no migration shim, no warning, and no opt-in. The new prop is added to defaults() with value true, so callers that simply omit it silently flip from follow-mode to between-fingers mode. The PR's own Test Plan calls out: "Spot-check existing zoomable-view consumers that do not pass contentRotation, pinchPanEnabled, or onRotation to confirm default pan/zoom behavior still works" — but for the staticPinPosition path, the default value choice flips the behavior.

A separate gate later in _handlePinching compounds the impact: if (pinchPanEnabled.value && offsetShift) { newOffsetX += offsetShift.x; newOffsetY += offsetShift.y; }. So with the new default, a static-pin pinch now both (a) centers between fingers and (b) applies translational pinch-pan. That is two simultaneous behavior changes for the same pre-existing consumers.

Step-by-step proof

Consider an existing consumer that renders <ReactNativeZoomableView staticPinPosition={{ x: 200, y: 300 }} /> and pinches with two fingers whose midpoint is at (250, 350).

Before this PR (original behavior):

  1. staticPinPosition.value is truthy → zoomCenter = { x: 200, y: 300 } (the pin).
  2. calcNewScaledOffsetForZoomCentering is called with zoomCenter = (200, 300).
  3. Result: the pin stays exactly under itself on the screen during the pinch, matching SPECS line 188.

After this PR (with no source change by the consumer):

  1. pinchPanEnabled.value defaults to true (set in defaults() at line ~100).
  2. The condition staticPinPosition.value && !pinchPanEnabled.value is false, so zoomCenter stays at the gesture midpoint (250, 350).
  3. calcNewScaledOffsetForZoomCentering is now called with the midpoint, not the pin. The pin drifts on screen — "the pin moves around way too much while zooming," exactly as the deleted comment warned.
  4. Additionally, offsetShift is now applied (the second pinchPanEnabled.value gate is true), so any movement of the gesture midpoint also pans the content during the pinch — another behavior the prior code did not do for the static-pin path.

Impact

Every existing caller of react-native-zoomable-view that uses staticPinPosition and upgrades to this version silently loses the "pin stays stable under the user's fingers during pinch" guarantee. Visually the pin will drift around the screen while pinching, which is the regression the original comment was specifically warding against. Documentation (SPECS.md) continues to describe the old behavior, so this is also a documentation/code drift.

How to fix

Either:

  1. Default pinchPanEnabled to false in the defaults() block at src/ReactNativeZoomableView.tsx:100. This preserves the prior behavior for the static-pin path, and new consumers must opt into the new pinch-pan mode. Update SPECS.md and the prop JSDoc to reflect the opt-in nature.
  2. Invert the gate so static-pin anchoring is the default and pinchPanEnabled only changes the non-static-pin behavior — e.g. if (staticPinPosition.value) { zoomCenter = staticPin; } unconditionally, and use pinchPanEnabled only to gate the offsetShift translation. This keeps the spec wording on lines 77 and 188 correct without any caller changes.

Option 2 matches the spec text most cleanly; option 1 makes the new prop's intent more explicit. Either resolves the silent contract break.

Comment on lines 47 to 49
useEffect(() => {
measure();
// We've already run `grabZoomSubjectOriginalMeasurements` at various events
// to make sure the measurements are promptly updated.
// However, there might be cases we haven't accounted for, especially when
// native processes are involved. To account for those cases,
// we'll use an interval here to ensure we're always up-to-date.
// The `setState` in `grabZoomSubjectOriginalMeasurements` won't trigger a rerender
// if the values given haven't changed, so we're not running performance risk here.
measureZoomSubjectInterval.current = setInterval(measure, 1e3);

return () => {
measureZoomSubjectInterval.current &&
clearInterval(measureZoomSubjectInterval.current);
};
}, []);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Removing the 1-second remeasure interval is a deliberate refactor (onLayout is the more reliable signal), but it leaves SPECS.md §componentDidMount/§componentWillUnmount (lines 308 and 312) describing behavior that no longer exists, and removes the explicit defense the original code documented against an iOS edge case (react-navigation pop combined with keyboard dismiss returning all-zero measurements). Either restore the interval, or update SPECS.md and re-test the named iOS scenario to confirm onLayout covers it.

Extended reasoning...

What changed

src/hooks/useZoomSubject.ts previously kept a setInterval(measure, 1000) running for the lifetime of the component, with an explicit comment block:

there might be cases we haven't accounted for, especially when native processes are involved. To account for those cases, we'll use an interval here to ensure we're always up-to-date.

and a nested setTimeout defending against a specific scenario:

a weird issue on iOS where the measurements are all 0 when navigating back (react-navigation stack) from another view while closing the keyboard at the same time.

This PR removes both the interval and the comments, replacing them with an onLayout handler plus a one-shot View.measure() fallback that early-returns once originalWidth/originalHeight are non-zero (useZoomSubject.ts:28 and :38).

Why the refutation is mostly right but not fully

The refuting verifier is correct that onLayout is the canonical, more reliable layout signal in modern React Native, and that the all-zero failure mode of View.measure() was specific to that API. The PR description also explicitly says it switches zoom subject measurement to layout dimensions so transforms do not distort the source size, so this is intentional. For the routine case, the new code is an upgrade.

However, the refutation does not address the two concrete issues that remain:

Issue 1: SPECS.md drift (documentation contradicts code)

SPECS.md still documents the removed behavior as part of the lifecycle contract:

  • §componentDidMount line 308: "Starts 1-second measurement interval"
  • §componentWillUnmount line 312: "Clear measurement interval"

Neither line was updated in this PR. Future readers (and AI assistants) consulting SPECS will be misled.

Issue 2: No self-correction path if onLayout ever misses

The new fallback in useZoomSubject.ts:28 runs View.measure() exactly once on mount, and bails out immediately if originalWidth.value && originalHeight.value are already set. After that, if onLayout ever fails to re-fire after a layout-affecting event the previous code defended against (the named iOS keyboard/navigation case), there is no recovery — originalWidth/Height stay stale (or stuck at 0) forever. Most code paths in ReactNativeZoomableView.tsx early-return on if (!originalHeight.value || !originalWidth.value) (e.g. lines 493 and 549 of the new code), so the failure mode is "component appears mounted but is completely unresponsive to gestures."

Step-by-step proof of the regression in the named edge case

  1. User has a screen with a ReactNativeZoomableView inside a react-navigation stack screen, with the keyboard up.
  2. User triggers a back navigation that simultaneously dismisses the keyboard. (The exact scenario the removed comment called out.)
  3. The wrapper View is briefly off-screen / re-laid-out during the transition; iOS reports degenerate (all-zero) measurements during this window.
  4. Before this PR: the once-per-second interval re-runs measure() after the transition settles, and originalWidth/Height are restored to correct non-zero values. Component recovers.
  5. After this PR: onLayout fires once with the bad layout (or fails to re-fire after the transition completes). The fallback View.measure() is gated by if (originalWidth.value && originalHeight.value) return and will not run again. The shared values stay 0.
  6. Subsequent gesture handlers hit if (!originalHeight.value || !originalWidth.value) return and silently no-op — the zoomable view is unresponsive until unmount/remount.

Whether step 5 actually happens on modern RN is the empirical question the verifiers flagged: onLayout is generally reliable, but the original comment specifically described an iOS scenario where layout signals were unreliable. Without a re-test of that scenario, we can't assert the regression is gone.

How to fix

Either of:

  1. Restore the interval (simplest; preserves SPECS contract and edge-case defense). The original comment notes the setState is a no-op when values are unchanged, so there's no perf concern.
  2. Drop the if (originalWidth.value && originalHeight.value) return guard from the fallback, and either keep a low-frequency interval or rely on onLayout alone, AND update SPECS.md §componentDidMount/§componentWillUnmount to remove the interval references, AND verify on an iOS device that the react-navigation-pop-with-keyboard-dismiss scenario still produces a usable measurement.

Option 2 is the cleaner long-term direction but requires the verification step that this PR has not done.

thomasvo and others added 5 commits April 28, 2026 19:18
…n find typings and modules without a built lib/.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ped during the diana patch extraction. Diana POC consumers (sheetHome centering reactions) call ref.moveStaticPinToWorklet and ref.zoomToWorklet from useAnimatedReaction, so they need to be on the imperative handle.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e flag flagged. Each guarded touch (single-touch tap, single-touch pan, double-tap, gesture-handler manual onTouches*) returns early when the touch slot is undefined instead of dereferencing it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rtyTypes / newer Reanimated / React 19) compile cleanly: type singleTapTimeoutId ref slot as explicit undefined-union and assign-undefined instead of delete (the JS-thread cancel is functionally equivalent), type inverseZoomStyle as Reanimated AnimatedStyle so useAnimatedStyle return values are assignable, and rewrite the visual-touch-feedback short-circuit as a ternary so the false branch produces null rather than a non-renderable boolean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ack branch never returns 0 to JSX. Stricter consumers (React 19 types) reject 0 as a ReactNode; this rewrite produces only Element[] or null.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Base automatically changed from thomas/reanimated to master May 14, 2026 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants