Strip complicated but low priority features ahead of reanimated conversion#149
Strip complicated but low priority features ahead of reanimated conversion#149thomasttvo merged 38 commits intomasterfrom
reanimated conversion#149Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
elliottkember
left a comment
elliottkember
left a comment
There was a problem hiding this comment.
I think this should be fine, but it's a breaking change so we'll need a minor version, and a CHANGELOG.md or some kind of breaking change notice in the release. I'll be interested to see whether anybody is using these features and has trouble upgrading
|
@elliottkember that's a good point. I've created this PR to create a better setup for changelog generation. We can edit the changelog (once it's generated) to explain further which features have been dropped once we publish. Additionally, we can also publish this as part of an RC version together with the reanimated stuff and ask the community for feedback before turning it into a official version |
thomasttvo
changed the title
reanimated conversion
|
@thomasttvo Sounds like a plan. It may actually be worth making a new major version RC. Given the scope of the reanimated change I think it's justified, and will avoid anybody accidentally updating their if they use |
thomasttvo
changed the title
reanimated conversionreanimated conversion
|
@claude review |
1 similar comment
|
@claude review |
|
@claude please review this PR |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…StaticPin gesture - Use stopAnimation callback to sync offsetX/offsetY/zoomLevel when interrupting animations, preventing stale offset jumps - Store and remove animation listener IDs in componentWillUnmount to prevent memory leaks with external animated values - Fix README movementSensibility default from 1.9 to 1 (matching code) - Fix StaticPin onPanResponderMove to use || instead of && so single-axis pans on the pin are forwarded to parent
|
@claude review |
Promote the local listenerId in zoomTo() to a class field so componentWillUnmount can clean it up if the component unmounts mid-animation. Also add stopAnimation() calls to prevent post-unmount callbacks from _resolveAndHandleTap's pan animation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When maxZoom is null (unlimited zoom), _getNextZoomStep now uses a
default ceiling of 3 zoom steps from initialZoom so double-tap cycles
back instead of zooming indefinitely. Also corrects the zoomTo() JSDoc
which incorrectly claimed {x:0,y:0} is the view center — it's the
top-left corner; center is {originalWidth/2, originalHeight/2}.
Addresses Claude Code Review findings on PR #164.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/ReactNativeZoomableView.tsx:1098-1112— TheonZoomAftercallback inzoomTo()fires post-unmount whenzoomAnimatedValueis provided as an external prop, because the.start()completion callback only checksif (finished)without athis.mountedguard. WhenzoomAnimatedValueis external,componentWillUnmountcorrectly skipsstopAnimation()to avoid interrupting the owner's animation — meaning the animation runs to completion with{finished: true}andonZoomAfterfires on a dead component. Fix: changeif (finished) {at line 1105 toif (finished && this.mounted) {.Extended reasoning...
This bug was introduced by this PR, which moved
onZoomAfterfrom a synchronous call (before the animation) into the animation's.start()completion callback. While the PR correctly added athis.mountedguard elsewhere (e.g.,_resolveAndHandleTapat line 904:if (finished && this.mounted) this._updateStaticPin()), the same guard was omitted from thezoomTo()completion callback.The specific code path: in
componentWillUnmount, the teardown logic guards:if (\!this.props.zoomAnimatedValue) this.zoomAnim.stopAnimation(). This means whenzoomAnimatedValueIS provided as an external prop,stopAnimation()is intentionally skipped — the animation value is owned by the parent and should not be interrupted by the child's unmount. This is correct behavior for not clobbering the parent's animation, but it means anyzoomTo()animation in flight will run to natural completion with{finished: true}.The
zoomTo().start()callback then executes:if (finished) { this.props.onZoomAfter?.(null, null, this._getZoomableViewEventObject()); }. Sincefinishedistrueand there is nothis.mountedcheck,onZoomAfterfires on a dead component. The_getZoomableViewEventObject()reads from stalethis.state, which may produce incorrect event data in addition to the post-unmount side effect.Concrete proof: (1) Consumer passes
zoomAnimatedValueprop toReactNativeZoomableView. (2) Consumer callszoomTo(2), starting a ~300ms native animation. (3) Screen navigates away — component unmounts. (4)componentWillUnmountsetsthis.mounted = falseand skipszoomAnim.stopAnimation()due to external value guard. (5) ~300ms later, animation completes with{finished: true}. (6).start()callback fires:if (finished)is true,onZoomAfteris called on the dead instance.The fix is a one-word change:
if (finished && this.mounted) {. This is exactly the same pattern used in_resolveAndHandleTapat line 904 in this same PR. The pre-existing behavior (before this PR) was thatonZoomAfterwas called synchronously before the animation started, which always happened while the component was mounted — this PR introduced the async path without the necessary mounted guard.
The cleanup in _handlePanResponderGrant (from a716099) was deleting doubleTapFirstTapReleaseTimestamp and doubleTapFirstTap unconditionally whenever singleTapTimeoutId was set. This broke genuine double-taps because the second tap always touches down while singleTapTimeoutId is active (that's the definition of a double-tap within the delay window). Move the delete statements to the gesture transition points: - When gestureType transitions to 'shift' (drag detected) - When gestureType transitions to 'pinch' This preserves the original fix for tap-pan-tap false positives while allowing genuine double-taps to work correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review |
…unted
- Update TypeScript types for maxZoom and minZoom to accept `number | null`,
matching the implementation (which uses `!= null` guards) and README docs
('Can be set to null to allow unlimited zooming').
- Add `this.mounted` guard to zoomTo's onZoomAfter callback, preventing
post-unmount callback fire when zoomAnimatedValue is an external prop
(componentWillUnmount skips stopAnimation for external values).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review |
When longPressDuration < doubleTapDelay, a confirmed long press would leave doubleTapFirstTapReleaseTimestamp intact, causing the subsequent release to spuriously fire onDoubleTap. Clear double-tap state after onLongPress fires, mirroring the cleanup in the drag/pinch paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review |
Reverts all commits after 99b5406 that were incorrectly pushed to this branch. These include PR #164 pre-existing fixes and subsequent review fixes that should have stayed on thomas/pre-existing-fixes. Restores thomas/strip-features to its proper scope (feature stripping + 3 review fixes from 99b5406). The reverted changes will be reinstated on a separate branch for PR #164. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Seven pre-existing bugs identified during code review of #149: 1. _setNewOffsetPosition redundantly called zoomAnim.setValue during pan, causing onTransform/onStaticPinPositionMove to fire twice per frame 2. zoomTo() used truthy checks for minZoom/maxZoom, so minZoom=0 was silently skipped allowing negative zoom (flipped content) 3. _getNextZoomStep returned undefined when maxZoom=null, silently disabling double-tap zoom when consumers set unlimited zooming 4. doubleTapZoomToCenter used {0,0} as zoom center but the coordinate system is viewport-relative (0,0 = top-left, not center) 5. zoomTo() fired onZoomAfter synchronously before the animation ran, so consumers got stale zoomLevel in the callback 6. onStaticPinPositionChange had two firing paths — immediate via _updateStaticPin on gesture end (stale coords during double-tap) and debounced via listener (correct). Replaced immediate call with debounce flush 7. StaticPin froze onPress/onLongPress callbacks at mount time via useRef(PanResponder.create(...)). Added refs that update on render Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review |
6 similar comments
|
@claude review |
|
@claude review |
|
@claude review |
|
@claude review |
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
No bugs found and all previous review threads are resolved, but this is a labeled breaking change across 8 files touching core gesture handling — a human should sign off before merge.
Extended reasoning...
Overview
PR #149 strips four publicly-documented props (, , , ) and their underlying logic (, , , ), and simultaneously fixes a set of resource-leak and post-unmount safety issues in and . Eight files are modified; is deleted entirely.
Security risks
None. The PR is entirely within a React Native UI component — no auth, networking, crypto, or privilege-sensitive code paths are involved.
Level of scrutiny
High. This is a labeled breaking public-library release that removes API surface that consumers may depend on. Beyond the API removals, the PR touches the most complex internal code in the repo: the PanResponder state machine, animation listener lifecycle, gesture handoff between the parent view and , and double-tap detection. The history of this PR (20+ bugs caught and fixed across multiple review rounds, a scope-restoring revert) reflects that complexity. Even with no bugs found in the current revision, the correct level of scrutiny for a breaking library release is a human final-approval pass.
Other factors
The bug hunting system found zero issues in the current revision, and all prior inline comment threads are marked resolved (either fixed or dismissed as referencing reverted commits). The PR is well-scoped and the description clearly documents what is being removed and why. The sole reason for deferring is the breaking-change label and the size/criticality of the touched code, not any identified defect.
thomasttvo
changed the title
reanimated conversionreanimated conversion
Brings in master's feature-strip (#149) and version-requirements docs (#157). Post-merge gates: - Build: yarn typescript passes - Lint: yarn lint passes - SPECS.md: already documents removed features (bindToBorders, panBoundaryPadding, animatePin, pinAnim, disableMomentum) in "Removed Features" and "Breaking Changes" sections — no drift introduced Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Note!! Claude implementer and Claude Code Reviewer went WAY overboard with this one. Need some kind of harness to reduce this behavior in the future. Edit: harness added in `CLAUDE.md` to focus the reviewer on correctness check only and avoid scope creep, which seems to have stopped the infinite loop. ## Summary Adds `SPECS.md` as the authoritative behavior contract for `ReactNativeZoomableView` and `StaticPin`, plus a root `CLAUDE.md` that steers automated and human reviewers to treat the spec as a contract when reviewing `src/` changes. The spec documents the full API surface, gesture system, static pin behavior, animation lifecycle, coordinate system, callback fire ordering/counts, and known edge-case behavior. Landing this first gives future changes (including the stacked work on #149 and #165) a stable reference to validate against. This PR is documentation only — no runtime code changes. It is stacked on [#149](#149). ## Test Plan - Open `SPECS.md` on the PR and skim the table of contents — section anchors resolve, no dead links. - Spot-check a handful of claims against `src/ReactNativeZoomableView.tsx` and `src/components/StaticPin.tsx` to confirm the spec matches current behavior (e.g. double-tap zoom formula, `onTransform` dual-fire during gestures, `moveStaticPinTo` bypassing `onShifting`, `doubleTapZoomToCenter` top-left anchor bug note). - Open root `CLAUDE.md` and confirm the review-steering rules read cleanly (contract breaks, spec drift, non-goals, spec-only PR guidance). - Confirm no files under `src/`, `example/`, or package manifests are touched — docs-only diff. ## Risk - Low runtime risk: no source, config, or dependency changes; the library build and behavior are unaffected. - Primary risk is spec inaccuracy — a wrong claim in `SPECS.md` would mislead future reviewers into flagging correct code or accepting incorrect code. The spec has been through ~12 rounds of Claude Code Review against the source and converged. - `CLAUDE.md` changes reviewer behavior on every subsequent PR; watch the first few reviews after merge to confirm it steers rather than over-flags. ## Change details - `SPECS.md` (new, +399): full behavior contract covering Public API, Props API, Gesture Lifecycle, Tap Handling, Pinch Zoom, Double-Tap Zoom, Pan/Shift Behavior, Static Pin (drag, handoff, release, terminate), Coordinate System, Animation listener lifecycle, Pin Position Updates, Callbacks table with fire counts and caveats, and a Known Bugs / Falsy-Guard Traps catalog. - `CLAUDE.md` (new, +32): repo-level review rules directing Claude Code Review (and human reviewers) to flag contract breaks and spec drift against `SPECS.md`, with explicit non-goals to prevent over-flagging of internal implementation details. --------- Co-authored-by: thomasvo <thomas.vo@openspace.ai> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…165) ## Summary Stacks correctness and safety fixes on top of [PR #149](#149 feature strip. Targets bugs discovered during review of PR #149 that also exist in the current library codebase: stale pin-press closures, a listener leak and post-unmount `onZoomAfter` fire in `zoomTo()`, broken double-tap zoom cycle-back in two configurations, incorrect `zoomCenter` semantics (both in `doubleTapZoomToCenter` behavior and JSDoc), double-fire of `onStaticPinPositionChange` at pan-end, and an accidental `onTransform` double-fire on every pan frame. Supersedes the prematurely-merged PR #164 and incorporates five rounds of Claude Code Review feedback. ## Test Plan - Mount a `ReactNativeZoomableView` with a `StaticPin` and a pan-responder gesture; double-tap at increasing levels and confirm zoom cycles back to `initialZoom` when it hits `maxZoom`. - Repeat with `maxZoom={null}` (unlimited) — confirm zoom still cycles back after ~3 steps rather than growing indefinitely. - Repeat with `zoomStep={null}` + a configured `maxZoom` — confirm cycle-back still works on double-tap at max. - Set `doubleTapZoomToCenter={true}` and double-tap on an off-center area; confirm the zoom centers on the viewport center (`originalWidth/2, originalHeight/2`), not the top-left corner. - Pass an `onZoomAfter` callback and call `zoomTo()`; confirm the callback fires exactly once, only when the animation finishes naturally, and reads the final post-animation `zoomLevel`. - Unmount the component mid-animation (with an external `zoomAnimatedValue` supplied) and confirm `onZoomAfter` does not fire after unmount. - Call `zoomTo()` twice in rapid succession and confirm the first animation's completion callback does not remove the second animation's pan-sync listener. - Wire up `onStaticPinPositionChange` and perform a single-tap pan; confirm the callback fires once with the final position (not twice ~100 ms apart). - Swap `onPress`/`onLongPress` handlers on a `StaticPin` after mount (e.g., via state change); tap and confirm the new handler runs, not the stale one from first render. - Regression: confirm `zoomTo()` still rejects values outside `minZoom`/`maxZoom`, and that `minZoom={0}` is now respected (previously silently skipped). ## Risk - `zoomTo()` gesture/animation lifecycle: `capturedListenerId` rename plus the added `this.mounted` guard and `finished` check in `.start()` change both listener cleanup and `onZoomAfter` firing semantics — consumers relying on the previous (synchronous / always-fires) behavior may see a behavior change. - Double-tap zoom state machine in `_getNextZoomStep`: the guard reorder and new `effectiveMax` computation change the cycle-back trigger for several `zoomStep` / `maxZoom` combinations. Most likely regression surface. - `onStaticPinPositionChange` debounce flush replaces a direct call in two places (`_handlePanResponderEnd` and single-tap pan animation end); consumers that expected the pre-debounced immediate fire will now see delivery deferred to the flush tick. - `StaticPin` refs for `onPress`/`onLongPress` mean handler identity changes are now observed on every render — intentional, but worth spot-checking that no downstream consumer was relying on the old frozen-at-mount behavior. - `zoomCenter` JSDoc correction and the `{0,0} → {W/2, H/2}` change in `doubleTapZoomToCenter` flip the semantics external callers may have been coding against. ## Change details **Listener lifecycle in `zoomTo()`** - Renamed the captured outer listener id to `capturedListenerId` so the completion callback cannot accidentally remove a listener registered by a later `zoomTo()` call (no-shadow + correctness). - Added `if (!this.mounted) return` at the top of the `.start()` callback to prevent post-unmount `onZoomAfter` fires when consumers supply an external `zoomAnimatedValue` (componentWillUnmount intentionally skips `stopAnimation()` in that case). - Added `finished` check from the animation callback so `onZoomAfter` only fires when the animation completes naturally, not when interrupted or cancelled. - After natural completion, `flush()` the debounced `onStaticPinPositionChange` before calling `onZoomAfter` so consumers reading pin position in `onZoomAfter` get the final post-animation value. **Double-tap zoom cycle-back (`_getNextZoomStep`)** - Reordered guards so the configured-`maxZoom` equality check runs before the `zoomStep == null` guard — previously `zoomStep={null}` + configured `maxZoom` silently lost cycle-back on double-tap at max. - Added an `effectiveMax` fallback of `initialZoom * (1 + zoomStep)^3` when `maxZoom == null`, so unlimited-zoom consumers also cycle back instead of growing indefinitely. - Inline comment clarifies that the second equality guard is only live when `maxZoom == null`. **`doubleTapZoomToCenter` + `zoomTo` JSDoc** - `zoomCenter` coordinates are viewport-relative (top-left origin), not center-relative. Updated the JSDoc to match reality. - Changed `doubleTapZoomToCenter` to use `{ originalWidth/2, originalHeight/2 }` as the center, matching the corrected semantics. **`zoomTo()` boundary checks** - Swapped truthy checks for `!= null` so `minZoom={0}` is honored (previously silently skipped, allowing negative zoom / flipped content). **`onStaticPinPositionChange` firing** - Replaced the direct `_updateStaticPin()` call at the end of `_handlePanResponderEnd` with `debouncedOnStaticPinPositionChange.flush()` to avoid the immediate + ~100 ms debounce double-fire. - Same replacement for the single-tap pan animation's completion callback. - Deleted the now-unreachable private `_updateStaticPin` method. **`_setNewOffsetPosition`** - Removed the redundant `zoomAnim.setValue(this.zoomLevel)` call that caused `onTransform` (and therefore `onStaticPinPositionMove`) to fire twice per pan frame. **`StaticPin` stale-closure fix** - Added `onPressRef` / `onLongPressRef` refs updated on every render, and called through the refs inside the `PanResponder` callbacks so handler identity changes after mount are honored. Previously the `PanResponder` was created once inside `useRef` and froze the handlers at mount. --------- Co-authored-by: thomasvo <thomas.vo@openspace.ai> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Since we'll convert to
reanimatedin a later PR, it's important to strip some features to avoid a complicated migration. We can always reintroduce them in thereanimatedworld if we need to.Summary
Strips pan boundaries, momentum decay, and pin animation features to reduce complexity ahead of the upcoming Reanimated migration. These features add significant internal state and animation logic that would make the Animated→Reanimated conversion error-prone. They can be reintroduced in the Reanimated world if needed.
Also fixes several pre-existing resource leaks and post-unmount safety issues discovered during the simplification: animation listener leaks on rapid
zoomTo()calls, incompletecomponentWillUnmountteardown, and StaticPin gesture handoff bugs where drags could fire phantom taps or skip the parent's release handler.Breaking: Removes props
bindToBorders,panBoundaryPadding,disableMomentum, andanimatePin. RemovespinAnimfrom StaticPin. Consumers using boundary clamping or momentum will need to implement equivalents externally.Test Plan
minZoom/maxZoomand feels smoothzoomStepincrements andinitialZoomstaticPinPosition: tap the pin, long-press the pin, and drag starting from the pin — confirm tap/longPress callbacks fire correctly, and drag hands off to the parent pan gesture without phantom taps on releaseRisk
bindToBorders— content can now pan freely off-screen. This was the default (true), so most existing consumers had boundary clamping on. Most likely regression area.stopAnimationcallback pattern — gesture start now captures final animated values via callbacks. If a consumer provides externalzoomAnimatedValue/panAnimatedValueXY, the unmount path skips stopping those animations (consumer-owned), but the gesture-start path still stops them — worth verifying with external animated values.Change details
applyPanBoundariesToOffset.ts,getBoundaryCrossedAnim,getPanMomentumDecayAnim, pin raise/drop animations, and the__offsetsboundary-tracking object. SimplifyoffsetX/offsetYto plain class fields.panListenerId,zoomListenerId,panTransformListenerId,zoomTransformListenerId,zoomToListenerId) and remove them incomponentWillUnmount. GuardzoomTo()against stacking listeners on rapid calls.mountedflag with guards inmeasureZoomSubject,_removeTouch, and the single-tap pan animation callback. Stop in-flight animations and cancel debounced calls on unmount. Clear pending timeouts.dx > 5 || dy > 5). TrackparentNotifiedto correctly route release/terminate to the parent. Suppress phantom taps after 3-finger gesture termination. PasslongPressDurationandonParentRelease/onParentTerminateas explicit props instead of relying on implicit behavior.stopAnimationnow uses callbacks (panAnim.x.stopAnimation(x => ...)) to capture final values, preventing drift between Animated values and JS-side mirrors.pinchToZoomInSensitivitydefault from 3→1), removelib/build artifacts, strip Jest config (no tests in repo).🤖 Generated with Claude Code