Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .claude/skills/review-and-test/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,13 +338,24 @@ Run through relevant entries after any fix or review. This is the single source
- [ ] `firstItemOffset` after fix — confirm it equals `ListHeaderComponent` height/width
- [ ] `measureParentSize(view)` returns `x=0, y=0` on RN 0.84 Fabric — the #2017 bug may only manifest on other RN versions

### Snapping + maintainVisibleContentPosition (Android)
- [ ] `snapToInterval` / `snapToOffsets` / `pagingEnabled` carousel on **Android** — swipe forward until recycling starts, then swipe backward. Landing on index 0 instead of the previous card is issue #2427. Use the `Snap Carousel Repro` fixture screen; its "settled on" readout makes it unmissable.
- [ ] Prepending to a snapping list still holds the visible item in place (`Horizontal MVCP` screen with a `snapToInterval` added)
- Android snapping is gated by one predicate in RN's `ScrollView.js`: `pagingEnabled === true || snapToInterval != null || snapToOffsets != null` becomes the native `pagingEnabled`. Anything keying off "does this list snap" must match it — `snapToInterval` alone misses paged carousels.
- FlashList hands the **native** `maintainVisibleContentPosition` to the ScrollView (`RecyclerView.tsx`). Android's `MaintainVisibleScrollPositionHelper` then re-anchors on every layout change of the first visible **cell** — not of `ScrollAnchor` — so a recycling list triggers it constantly. Each one calls `scrollToPreservingMomentum` → `recreateFlingAnimation(x, Integer.MAX_VALUE)`, cancelling the snap animator. **Nothing on the JS side can suppress this; only withholding the native prop stops it.**

### Performance
- [ ] Benchmark screen shows no FPS regression (use `ManualBenchmarkExample`)

---

## Common Issues

- **A green unit suite is not evidence the fix works** — a guard can be correct, well-tested, and still sit on a code path the bug never takes. Before believing a fix for a device-only symptom, reproduce it on device, then A/B it: `git checkout main && yarn build` (bug present) vs the fix branch (bug gone). If you cannot reproduce it first, you cannot claim you fixed it. To check whether a JS path even runs, drop a temporary `console.log` and read it back with `adb logcat` — and validate the probe itself (confirm the string is in the served bundle and that some other `ReactNativeJS` line reaches logcat) before trusting a zero count.
- **Test the wiring, not just the helper** — a pure predicate can be perfect while the call site ignores it. Assert on the prop the ScrollView actually receives, then delete the gate and confirm that test goes red.
- **Metro port 8081 may be taken by another project** — do not kill it. Start ours with `yarn start --port 8092`; `adb reverse` will not help because RN on an emulator dials `10.0.2.2`, not `localhost`. Point the app at it by pushing a prefs file instead:
`adb push prefs.xml /data/local/tmp/ && adb shell "run-as <pkg> cp /data/local/tmp/prefs.xml /data/data/<pkg>/shared_prefs/<pkg>_preferences.xml"` with `<string name="debug_http_host">10.0.2.2:8092</string>`.
- **Android build filling the disk** — `./gradlew assembleDebug -PreactNativeArchitectures=arm64-v8a` builds only the emulator ABI, roughly a quarter of the NDK output.
- **Tests pass but device shows bug** — did you `yarn build` and relaunch? The dist/ folder may be stale
- **Switched branches but behavior didn't change** — `dist/` is NOT rebuilt on branch switch. You MUST run `yarn build` after every `git checkout`. Verify with `grep` in `dist/` that the expected code is present before testing.
- **RTL looks wrong but LTR is fine** — did you set `forceRTL(true)` in `index.js` and do a full kill+relaunch?
Expand Down
1 change: 1 addition & 0 deletions fixture/react-native/src/ExamplesScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const ExamplesScreen = () => {
{ title: "Sticky Header Example", destination: "StickyHeaderExample" },
{ title: "Horizontal List", destination: "HorizontalList" },
{ title: "Carousel", destination: "Carousel" },
{ title: "Snap Carousel Repro", destination: "SnapCarouselRepro" },
{ title: "Grid", destination: "Grid" },
{ title: "Masonry", destination: "Masonry" },
{ title: "Complex Masonry", destination: "ComplexMasonry" },
Expand Down
5 changes: 5 additions & 0 deletions fixture/react-native/src/NavigationTree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import DynamicItems from "./DynamicItems";
import RecyclerViewHandlerTest from "./RecyclerViewHandlerTest";
import MovieList from "./MovieList";
import Carousel from "./Carousel";
import SnapCarouselRepro from "./SnapCarouselRepro";
import { LayoutOptions } from "./LayoutOptions";
import ShowcaseApp from "./ShowcaseApp";
import LotOfItems from "./lot-of-items/LotOfItems";
Expand All @@ -47,6 +48,10 @@ const NavigationTree = () => {
<Stack.Group>
<Stack.Screen name="Examples" component={ExamplesScreen} />
<Stack.Screen name="List" component={List} />
<Stack.Screen
name="SnapCarouselRepro"
component={SnapCarouselRepro}
/>
<Stack.Screen name="Grid" component={Grid} />
<Stack.Screen
name="DynamicColumnSpan"
Expand Down
87 changes: 87 additions & 0 deletions fixture/react-native/src/SnapCarouselRepro.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import React, { useCallback, useState } from "react";
import {
StyleSheet,
Text,
View,
useWindowDimensions,
NativeSyntheticEvent,
NativeScrollEvent,
} from "react-native";
import { FlashList } from "@shopify/flash-list";

/**
* Repro screen for issue #2427 - Android horizontal snapToInterval carousel
* over-snapping to index 0 on a backward swipe.
*
* Swipe forward a few cards until recycling kicks in, then swipe backward.
* The "settled on" readout should move one card at a time. Landing on card 1
* from the middle of the list is the bug.
*/

const ITEMS = Array.from({ length: 20 }, (_, i) => String(i + 1));
const CARD_GAP = 12;

export const SnapCarouselRepro = () => {
const { width: screenWidth } = useWindowDimensions();
const cardWidth = screenWidth - 48;
const snapInterval = cardWidth + CARD_GAP;

const [active, setActive] = useState(0);
const [offset, setOffset] = useState(0);

const onSettle = useCallback(
(event: NativeSyntheticEvent<NativeScrollEvent>) => {
const x = event.nativeEvent.contentOffset.x;
setOffset(Math.round(x));
setActive(Math.round(x / snapInterval));
},
[snapInterval]
);

return (
<View style={styles.container} testID="SnapCarouselReproScreen">
<Text style={styles.readout} testID="SnapCarouselActive">
settled on: {active + 1}
</Text>
<Text style={styles.offset}>offset: {offset}</Text>
<FlashList
data={ITEMS}
keyExtractor={(item) => item}
horizontal
showsHorizontalScrollIndicator={false}
snapToInterval={snapInterval}
decelerationRate="fast"
onMomentumScrollEnd={onSettle}
onScrollEndDrag={onSettle}
renderItem={({ item }) => (
<View style={[styles.card, { width: cardWidth }]}>
<Text style={styles.cardText}>{item}</Text>
</View>
)}
/>
</View>
);
};

const styles = StyleSheet.create({
container: { flex: 1, paddingTop: 60, backgroundColor: "#111" },
readout: {
fontSize: 34,
fontWeight: "bold",
color: "#fff",
paddingHorizontal: 24,
},
offset: { fontSize: 18, color: "#9bd", paddingHorizontal: 24, marginTop: 4 },
card: {
height: 320,
marginRight: CARD_GAP,
marginTop: 20,
backgroundColor: "tomato",
alignItems: "center",
justifyContent: "center",
borderRadius: 12,
},
cardText: { fontSize: 96, fontWeight: "bold", color: "#fff" },
});

export default SnapCarouselRepro;
66 changes: 66 additions & 0 deletions src/__tests__/RecyclerView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { render } from "@quilted/react-testing";

import { FlashListRef } from "../FlashListRef";
import { FlashList } from "..";
import { PlatformConfig } from "../native/config/PlatformHelper";

// Mock measureLayout to return fixed dimensions
jest.mock("../recyclerview/utils/measureLayout", () => {
Expand Down Expand Up @@ -514,4 +515,69 @@ describe("RecyclerView", () => {
expect(scrollToEndSpy).toHaveBeenCalled();
});
});
describe("native maintainVisibleContentPosition on snapping lists", () => {
const platform = PlatformConfig as unknown as {
nativeMvcpBreaksSnapFling: boolean;
};

afterEach(() => {
platform.nativeMvcpBreaksSnapFling = false;
});

// The prop the ScrollView is actually handed. Android's
// MaintainVisibleScrollPositionHelper only runs when this is present, so
// this is the single thing that decides whether a snap fling survives.
const nativeMvcpPropOf = (root: ReturnType<typeof render>) => {
const scrollable = root.findWhere((node: any) => node.props.onScroll);
if (!scrollable) throw new Error("Could not find scrollable component");
return scrollable.prop("maintainVisibleContentPosition" as never);
};

const renderCarousel = (snapProps: Record<string, unknown>) => {
const data = Array.from({ length: 30 }, (_, i) => i);
const result = render(
<FlashList
data={data}
horizontal
{...snapProps}
keyExtractor={(item) => String(item)}
renderItem={({ item }) => <Text>{item}</Text>}
/>
);
jest.runAllTimers();
return result;
};

it("withholds it from a snapToInterval list where the fling would be retargeted", () => {
platform.nativeMvcpBreaksSnapFling = true;

expect(nativeMvcpPropOf(renderCarousel({ snapToInterval: 100 }))).toBe(
undefined
);
});

it("withholds it from a pagingEnabled list too", () => {
platform.nativeMvcpBreaksSnapFling = true;

expect(nativeMvcpPropOf(renderCarousel({ pagingEnabled: true }))).toBe(
undefined
);
});

it("still passes it to a list that does not snap", () => {
platform.nativeMvcpBreaksSnapFling = true;

expect(nativeMvcpPropOf(renderCarousel({}))).toEqual(
expect.objectContaining({ minIndexForVisible: 0 })
);
});

it("still passes it to a snapping list on platforms that are not affected", () => {
platform.nativeMvcpBreaksSnapFling = false;

expect(nativeMvcpPropOf(renderCarousel({ snapToInterval: 100 }))).toEqual(
expect.objectContaining({ minIndexForVisible: 0 })
);
});
});
});
65 changes: 65 additions & 0 deletions src/__tests__/snapping.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import {
isSnappingList,
supportsNativeMaintainVisibleContentPosition,
} from "../recyclerview/utils/snapping";
import { PlatformConfig } from "../native/config/PlatformHelper";

describe("isSnappingList", () => {
// Mirrors ScrollView.js: pagingEnabled === true || snapToInterval != null ||
// snapToOffsets != null. Anything looser or tighter would cover a different
// set of lists than Android's native snapping path does.
it.each([
["pagingEnabled snaps", { pagingEnabled: true }, true],
["snapToInterval snaps", { snapToInterval: 300 }, true],
["snapToOffsets snaps", { snapToOffsets: [0, 300] }, true],
["an empty snapToOffsets array still snaps", { snapToOffsets: [] }, true],
["a zero snapToInterval still snaps", { snapToInterval: 0 }, true],
["no snapping props does not snap", {}, false],
["pagingEnabled false does not snap", { pagingEnabled: false }, false],
["a null snapToInterval does not snap", { snapToInterval: null }, false],
["a null snapToOffsets does not snap", { snapToOffsets: null }, false],
])("%s", (_label, props, expected) => {
expect(isSnappingList(props)).toBe(expected);
});
});

describe("supportsNativeMaintainVisibleContentPosition", () => {
const setPlatform = (nativeMvcpBreaksSnapFling: boolean) => {
(
PlatformConfig as unknown as { nativeMvcpBreaksSnapFling: boolean }
).nativeMvcpBreaksSnapFling = nativeMvcpBreaksSnapFling;
};

afterEach(() => {
setPlatform(false);
});

it("withholds the native prop from a snapping list on Android", () => {
setPlatform(true);

expect(
supportsNativeMaintainVisibleContentPosition({ snapToInterval: 300 })
).toBe(false);
expect(
supportsNativeMaintainVisibleContentPosition({ pagingEnabled: true })
).toBe(false);
expect(
supportsNativeMaintainVisibleContentPosition({ snapToOffsets: [0, 300] })
).toBe(false);
});

it("keeps the native prop for a list that does not snap on Android", () => {
setPlatform(true);

expect(supportsNativeMaintainVisibleContentPosition({})).toBe(true);
});

it("keeps the native prop on platforms whose fling survives a re-anchor", () => {
setPlatform(false);

expect(
supportsNativeMaintainVisibleContentPosition({ snapToInterval: 300 })
).toBe(true);
expect(supportsNativeMaintainVisibleContentPosition({})).toBe(true);
});
});
1 change: 1 addition & 0 deletions src/native/config/PlatformHelper.android.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { isRN083OrAbove } from "./versionCheck";

const PlatformConfig = {
defaultDrawDistance: 250,
nativeMvcpBreaksSnapFling: true,
supportsOffsetCorrection: true,
trackAverageRenderTimeForOffsetProjection: true,
isRN083OrAbove: isRN083OrAbove(),
Expand Down
1 change: 1 addition & 0 deletions src/native/config/PlatformHelper.ios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { isRN083OrAbove } from "./versionCheck";

const PlatformConfig = {
defaultDrawDistance: 250,
nativeMvcpBreaksSnapFling: false,
supportsOffsetCorrection: true,
trackAverageRenderTimeForOffsetProjection: false,
isRN083OrAbove: isRN083OrAbove(),
Expand Down
1 change: 1 addition & 0 deletions src/native/config/PlatformHelper.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const PlatformConfig = {
defaultDrawDistance: 250,
nativeMvcpBreaksSnapFling: false,
supportsOffsetCorrection: false,
trackAverageRenderTimeForOffsetProjection: false,
isRN083OrAbove: true,
Expand Down
1 change: 1 addition & 0 deletions src/native/config/PlatformHelper.web.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const PlatformConfig = {
defaultDrawDistance: 500,
nativeMvcpBreaksSnapFling: false,
supportsOffsetCorrection: false,
trackAverageRenderTimeForOffsetProjection: false,
isRN083OrAbove: true,
Expand Down
17 changes: 15 additions & 2 deletions src/recyclerview/RecyclerView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { useBoundDetection } from "./hooks/useBoundDetection";
import { adjustOffsetForRTL } from "./utils/adjustOffsetForRTL";
import { useSecondaryProps } from "./hooks/useSecondaryProps";
import { getInvertedTransformStyle } from "./utils/getInvertedTransformStyle";
import { supportsNativeMaintainVisibleContentPosition } from "./utils/snapping";
import { StickyHeaders, StickyHeaderRef } from "./components/StickyHeaders";
import { ScrollAnchor, ScrollAnchorRef } from "./components/ScrollAnchor";
import { useRecyclerViewController } from "./hooks/useRecyclerViewController";
Expand Down Expand Up @@ -486,15 +487,27 @@ const RecyclerViewComponent = <T,>(
const shouldMaintainVisibleContentPosition =
recyclerViewManager.shouldMaintainVisibleContentPosition();

// A snapping list on Android cannot be given the native prop without its
// flings being retargeted, so offset corrections fall back to scrollTo there.
const canUseNativeMaintainVisibleContentPosition =
supportsNativeMaintainVisibleContentPosition(props);

const maintainVisibleContentPositionInternal = useMemo(() => {
if (shouldMaintainVisibleContentPosition) {
if (
shouldMaintainVisibleContentPosition &&
canUseNativeMaintainVisibleContentPosition
) {
return {
...maintainVisibleContentPosition,
minIndexForVisible: 0,
};
}
return undefined;
}, [maintainVisibleContentPosition, shouldMaintainVisibleContentPosition]);
}, [
maintainVisibleContentPosition,
shouldMaintainVisibleContentPosition,
canUseNativeMaintainVisibleContentPosition,
]);

const shouldRenderFromBottom =
recyclerViewManager.getDataLength() > 0 &&
Expand Down
12 changes: 11 additions & 1 deletion src/recyclerview/hooks/useRecyclerViewController.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { adjustOffsetForRTL } from "../utils/adjustOffsetForRTL";
import { RVLayout } from "../layout-managers/LayoutManager";
import { ScrollAnchorRef } from "../components/ScrollAnchor";
import { PlatformConfig } from "../../native/config/PlatformHelper";
import { supportsNativeMaintainVisibleContentPosition } from "../utils/snapping";
import { WarningMessages } from "../../errors/WarningMessages";

import { useUnmountFlag } from "./useUnmountFlag";
Expand Down Expand Up @@ -181,7 +182,16 @@ export function useRecyclerViewController<T>(
!recyclerViewManager.animationOptimizationsEnabled
) {
// console.log("diff", diff, firstVisibleItemKey.current);
if (PlatformConfig.supportsOffsetCorrection) {
// The anchor nudge only moves the scroll position through the
// native maintainVisibleContentPosition the ScrollView was given.
// A snapping list on Android is not given it (see snapping.ts), so
// there the correction has to drive the scroll position directly.
if (
PlatformConfig.supportsOffsetCorrection &&
supportsNativeMaintainVisibleContentPosition(
recyclerViewManager.props
)
) {
// console.log("scrollBy", diff);
scrollAnchorRef.current?.scrollBy(diff);
} else {
Expand Down
Loading
Loading