fix(sortable): make onItemSnapEnd a ref so finalizeDrag is actually called - #283
Open
MFA-G wants to merge 1 commit into
Open
fix(sortable): make onItemSnapEnd a ref so finalizeDrag is actually called#283MFA-G wants to merge 1 commit into
MFA-G wants to merge 1 commit into
Conversation
…alled
`SortableItem` destructures `onItemSnapEnd` from `sortable._internal` at render
time, but `SortableContainer` only assigns it in a `useLayoutEffect` that runs
after render. Since `useSortableList` rebuilds `_internal` on every render with
`onItemSnapEnd: undefined`, the destructured value is always `undefined`.
The result is that `finalizeDrag` is never invoked when the snap animation ends,
so the reorder is not committed and the item visually snaps back to its original
position.
The type already documented the intent ("stored as a ref so the latest
finalizeDrag is always called"), but a plain property on a per-render object
cannot provide that. Store the callback in a stable `useRef` instead:
- `useSortableList` owns `onItemSnapEndRef` (created once).
- `SortableContainer` writes `onItemSnapEndRef.current = finalizeDrag`.
- `SortableItem` reads `onItemSnapEndRef.current` at call time, so it always
sees the registered callback regardless of when `_internal` was captured.
Fixes nuclearpasta#236
|
@MFA-G is attempting to deploy a commit to the Nuclear Pasta Team on Vercel. A member of the Team first needs to authorize it. |
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
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.
Fixes #236.
Problem
SortableItemdestructuresonItemSnapEndfromsortable._internalat render time:but
SortableContaineronly assigns it in auseLayoutEffect, which runs after render:useSortableListrebuilds_internalon every render withonItemSnapEnd: undefined, so the valueSortableItemcaptured is alwaysundefined— the assignment from the previous render is written onto an object that has already been replaced.Net effect:
finalizeDragis never called fromonSnapEnd, the reorder is never committed, and the item visually snaps back to its original position after the drop.Why the previous approach could not work
The type already documented the intended semantics:
That contract requires a stable identity across renders, which a plain property on a per-render object cannot provide. This PR makes it an actual ref.
Change
useSortableListnow ownsonItemSnapEndRef— auseRefcreated once, so its identity is stable for the lifetime of the hook.SortableContainerwritessortable._internal.onItemSnapEndRef.current = finalizeDrag.SortableItemcallsonItemSnapEndRef.current?.()— read at call time, so it sees the registered callback no matter when_internalwas captured.SortableListInternal.onItemSnapEnd?: () => voidbecomesonItemSnapEndRef: RefObject<(() => void) | undefined>.Destructuring the ref in
SortableItemis safe now precisely because the ref object is stable; only its.currentchanges.This also covers the stale-
_internalcase the original comment was worried about (e.g. the MATCH path skipping aFlatListre-render): even aSortableItemholding an older_internalreaches the same ref object and therefore the latestfinalizeDrag.Note on the report in the issue thread that a one-line patch (
sortable._internal.onItemSnapEnd?.()) did not fully help: that variant reads the current render's_internal, which has just been rebuilt withundefinedand is only patched by theuseLayoutEffectafterwards — so it still misses depending on timing. The ref removes the ordering dependency entirely.SortableListInternalis an internal type (_internal), so this is not a public API change.Validation
I also updated the two
sortable-item.mdxarchitecture notes that referenced the old name so the docs match the code.