Cancel in-progress reorders when the list is torn down - #630
Conversation
60194b8 to
75c6b67
Compare
|
[AI] 🤖 Posted by Elizabeth's AI agent. Ready for a maintainer to land. Quick status to make that easy:
Context on why we'd like to land this: SPOS is currently carrying a temporary local patch of this exact diff to stop a production crash (drag a list item, navigate away mid-drag → Happy to squash, adjust the test, or split anything out if that helps it land — just say the word. |
75c6b67 to
3b71bfb
Compare
|
[AI] 🤖 Strengthened the test coverage to make the regression case more convincing:
Note I still can't run these locally (Command Line Tools only, no full Xcode / simulator SDK), so they're unverified until the fork CI run is approved. The |
kyleve
left a comment
There was a problem hiding this comment.
I apparently lost my permissions for this repo but LGTM, nice catch
johnnewman-square
left a comment
There was a problem hiding this comment.
This looks good!
|
This PR will fix the CI issues: #631 |
## Summary - Upgrade the pinned mise action from v3.6.3 to v4.3.0. - Pin the Mise binary to version 2026.9.1 for deterministic CI setup. - Move the action runtime from deprecated Node 20 to Node 24. ## Context The Listable test workflow for PR #630 failed before compilation because the Mise version endpoint advertised v2026.9.3 while the corresponding GitHub release was not yet published. All three matrix jobs received a 404 during the Set up Mise step. Pinning the Mise binary avoids relying on the latest-version publication sequence and prevents future CI runs from adopting new Mise releases without an intentional update. ## Testing - Parsed the updated workflow successfully with Ruby YAML. - Verified the patch with git diff --check. - Mise 2026.9.1 successfully ran the complete iOS 15.4, 16.2, and 17.2 test matrix on the previous revision of this PR. - The updated version-pinned workflow will be validated again by GitHub Actions. ### Checklist - [x] No changelog entry is needed because this is an internal CI-only change.
3b71bfb to
62d6960
Compare
A reorder gesture drives a native UICollectionView interactive-movement session that expects to be resolved with endInteractiveMovement or cancelInteractiveMovement. If the list leaves the window or is deallocated while a drag is still held — for example, the user navigates away mid-drag — nothing ends that session. It outlives the content it was started against, so a later content update or layout pass reads a now-stale index and traps in the layout (Index out of range in ListLayoutContent.item(at:)). Cancel any in-progress reorder in didMoveToWindow when leaving the window, and in deinit, while the data source and layout are still in sync. Both are guarded by hasInProgressReorders so they are no-ops in the common case. Add regression tests covering the window-removal and deinit teardown paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
62d6960 to
09d50f9
Compare
Summary
Fixes a crash when a list is torn down while an interactive reorder is still in progress — for example, the user navigates away (taps a nav-bar shortcut, pops the screen) while still holding a drag.
A reorder gesture drives a native
UICollectionViewinteractive-movement session, which UIKit expects to be resolved withendInteractiveMovement()orcancelInteractiveMovement(). Today nothing ends that session when the list leaves the window or is deallocated:deinitonly nilscollectionView.delegate/dataSource.didMoveToWindow()does nothing whenwindow == nil.cancelAllInProgressReorders()is the content-diff-apply path (guarded byhasInProgressReorders, and annotated as experimental/"will likely crash").So the interactive-movement session outlives the content it was started against. A later content update or layout pass then reads a now-stale index and traps:
(also observed via
CollectionViewLayout.positionForItem(at:)←ListView.setPresentationStateItemPositions().)Change
Cancel any in-progress reorder while the data source and layout are still in sync:
didMoveToWindow()when the list leaves the window (the navigate-away-mid-drag case).deinitas a backstop.Both are guarded by
hasInProgressReorders, so they are no-ops in the common case.hasInProgressReordersis relaxed fromprivateto internal so tests can assert on it.Testing
Adds
ListView.ReorderTeardownTestscovering both teardown paths (window removal and deinit). Verified the reorder state is cleared on window exit and that deallocation with a live reorder does not crash.Reported from Square Point of Sale: app crashes when a payment type is dragged between sections in Payment settings and a navigation-bar shortcut is tapped mid-drag.
Checklist
Mainsection.