fix: two reorder-correctness bugs (stale measurements after delete, dragging disabled outside viewable range) - #624
Open
alhansrisuk wants to merge 2 commits into
Open
Conversation
…math Removing an item from `data` shifts the `index` of every cell below it, but a cell's own `onLayout` doesn't necessarily refire on an index-only shift (its dimensions haven't changed) — so `cellDataRef`'s stored offset/size for cells below a deleted row went stale, corrupting drag-reorder position math for the remaining items. Previously this re-measure was only forced on web via a mount-time RAF; native FlatList cells need it too, on every index change, not just once at mount. That re-measure firing more often exposed a second bug: `heldTanslate` (the anti-flicker hold that keeps a cell's drag-end transform steady until its real settled position is confirmed — see `animStyle`) was being released the instant the re-measure was *kicked off*, not once it actually resolved. Since the re-measure is now also speculative (fired a frame after every index change, not gated behind a confirmed `onLayout`), that could release the hold before the FlatList's own relayout had actually settled the shifted cells into their new flow position — flashing/snapping visibly right as a reorder finished. Now the hold only releases once the measurement itself resolves (success or failure), preserving the original anti-flicker guarantee. Also cleans up `keyToIndexRef`/`cellDataRef` entries for keys no longer present in `data`, so a removed row's stale entry can't linger and be read by a future cell that reuses a similar key shape.
…e range useCellTranslate bailed out of computing a cell's drag translate for any cell outside [viewableIndexMin, viewableIndexMax] — populated from onViewableItemsChanged, which only updates on scroll or initial layout. A list whose container grows to reveal more content some other way (for example an outer animated height expanding a sheet, rather than the list itself scrolling) never fires a new viewability check, so those bounds stay stuck at whatever was visible in the very first — often barely-visible, still-collapsing — layout pass. Every cell beyond that range then silently can't be dragged: `drag()` still fires and sets `activeIndexAnim`, but the cell's own translate always computes to 0, so nothing visibly follows the gesture. This optimization only matters for a list long enough to be virtualized in the first place; for a short, fully-rendered list it has no benefit and can only make cells undraggable. Removing it fixes reordering for any consumer whose list's visible bounds can grow without a scroll event, without changing behavior for lists that rely on normal scrolling (virtualized or not) to reveal new cells.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent bugs we hit reordering a list that also supports deleting items, found while building a queue reorder feature:
indexof every cell below it, but a cell's ownonLayoutdoesn't necessarily refire on an index-only shift (its dimensions haven't changed). That leftcellDataRef's stored offset/size stale for cells below a deleted row, corrupting drag-reorder position math for the remaining items. The existing re-measure-on-mount workaround was gated to web only; native needs it too, and needs it on every index change, not just once at mount. Making that re-measure fire more often surfaced a second, related bug: the anti-flickerheldTanslatehold was being released the instant the re-measure was kicked off rather than once it actually resolved, which could release it before the FlatList's own relayout had settled the shifted cells into their new position — flashing/snapping visibly right as a reorder finished. Fixed by only releasing the hold once the measurement itself resolves (success or failure).useCellTranslate'sisOutsideViewableRangecheck depends onviewableIndexMin/viewableIndexMax, populated viaonViewableItemsChanged, which only fires on scroll or initial layout. A list whose visible bounds can grow some other way (e.g. an outer animated height expanding a sheet, rather than the list scrolling) never gets a fresh viewability check, so those bounds stay stuck at whatever was visible in the first, often barely-visible, layout pass — silently making every cell beyond that undraggable (drag()still fires, but the cell's own translate always computes to 0). This optimization only benefits a genuinely virtualized list; for a short, fully-rendered one it has no upside and can only break dragging, so it's removed.Split into two commits, each self-contained (
fix: stale cell measurements after a delete corrupt reorder position math,fix: dragging silently disabled for cells outside the initial viewable range) — see each commit message for more detail/reasoning.Test plan
yarn typecheckpasses on both commitsyarn test— same 3 pre-existing failures asmain(aReanimated.useEventincompatibility unrelated to this change, confirmed by running the suite on unmodifiedmain), no new failuresListHeaderComponent, a delete-then-reorder flow, and a sheet that expands via animated height rather than scrolling — all three symptoms (corrupted reorder position after a delete, a visible flash right as a reorder finished, and cells beyond the initial fold being completely undraggable) are gone after this change.