fix: prevent multiple onIndexChange calls during initialization
#68
Conversation
- Remove redundant currentIndex state to avoid duplicate callbacks - Use manager subscription as single source of truth for index changes - Implement ref pattern for `onIndexChange` to prevent stale closures - Ensure `onIndexChange` only fires on actual user interactions, not internal state changes Now `onIndexChange` correctly fires only once during initialization. Fixes #67
🦋 Changeset detectedLatest commit: 8eb8395 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GestureViewer
participant useGestureViewer
participant GestureViewerManager
User->>GestureViewer: Mount with initialIndex > 0
GestureViewer->>useGestureViewer: Initialize hook
useGestureViewer->>GestureViewerManager: Subscribe to index changes
GestureViewerManager-->>useGestureViewer: Notify index (once)
useGestureViewer-->>GestureViewer: Call onIndexChange(index)
Note over useGestureViewer: No duplicate calls<br>on initialization
User->>GestureViewer: Swipe to change image
GestureViewer->>useGestureViewer: Handle user interaction
useGestureViewer->>GestureViewerManager: Update index
GestureViewerManager-->>useGestureViewer: Notify index change
useGestureViewer-->>GestureViewer: Call onIndexChange(new index)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-02T17:37:07.501ZApplied to files:
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying react-native-gesture-image-viewer with
|
| Latest commit: |
8eb8395
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2d6d15c.react-native-gesture-image-viewer.pages.dev |
| Branch Preview URL: | https://fix-prevent-multiple-onindex.react-native-gesture-image-viewer.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/useGestureViewer.ts (2)
58-58: Prefer a precise ref type overanyfor listRefUsing
anyloses type-safety and IntelliSense. If this is a FlatList, type it accordingly.- const listRef = useRef<any>(null); + const listRef = useRef<FlatList<any> | null>(null);Add the import (outside this range):
import { FlatList } from 'react-native';
60-60: Initialize the callback ref with the prop to avoid a pre-effect windowTiny improvement: seed the ref with the initial prop so the first manager emission (if any) won’t hit
nullbefore the effect runs.- const onIndexChangeRef = useRef<((index: number) => void) | null>(null); + const onIndexChangeRef = useRef<((index: number) => void) | null>(onIndexChange ?? null); + const lastEmittedIndexRef = useRef<number | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/major-areas-mix.md(1 hunks)src/useGestureViewer.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: saseungmin
PR: saseungmin/react-native-gesture-image-viewer#4
File: src/useGestureImageViewer.ts:184-207
Timestamp: 2025-07-02T17:37:07.501Z
Learning: In React Native Gesture Handler, the pinch gesture event.scale always starts from 1.0 at the beginning of each new pinch gesture and is relative/multiplicative, not absolute. To maintain smooth scaling, you need to multiply the current scale by event.scale rather than directly assigning it.
📚 Learning: 2025-07-02T17:37:07.501Z
Learnt from: saseungmin
PR: saseungmin/react-native-gesture-image-viewer#4
File: src/useGestureImageViewer.ts:184-207
Timestamp: 2025-07-02T17:37:07.501Z
Learning: In React Native Gesture Handler, the pinch gesture event.scale always starts from 1.0 at the beginning of each new pinch gesture and is relative/multiplicative, not absolute. To maintain smooth scaling, you need to multiply the current scale by event.scale rather than directly assigning it.
Applied to files:
.changeset/major-areas-mix.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
.changeset/major-areas-mix.md (2)
7-12: Call out API surface changes and guard against duplicate emissionsThe note says there are no public API changes, but the hook appears to no longer return
currentIndex. If accurate, highlight this in the changelog. Also, to guarantee “only on actual index changes,” guard emissions in the subscription as suggested in code review.I can help update the Changeset text once we confirm whether the return shape changed.
Likely an incorrect or invalid review comment.
2-2: No breaking change detected; patch bump is appropriateAfter searching the codebase, the
useGestureViewerhook has never returned acurrentIndexvalue, and theuseGestureViewerControllerhook continues to exposecurrentIndex. There is no removal of that export, so this release does not introduce a breaking change and the patch-level version bump is correct.
saseungmin
deleted the
fix/prevent-multiple-onindexchange-calls-during-init
branch
- Remove redundant currentIndex state to avoid duplicate callbacks - Use manager subscription as single source of truth for index changes - Implement ref pattern for `onIndexChange` to prevent stale closures - Ensure `onIndexChange` only fires on actual user interactions, not internal state changes Now `onIndexChange` correctly fires only once during initialization. Fixes #67
onIndexChangeto prevent stale closuresonIndexChangeonly fires on actual user interactions, not internal state changesNow
onIndexChangecorrectly fires only once during initialization.Fixes #67
Summary by CodeRabbit
Bug Fixes
Refactor