Skip to content

Cancel in-progress reorders when the list is torn down - #630

Open
elizrus wants to merge 1 commit into
square:mainfrom
elizrus:erussell/pcm-2178-cancel-reorder-on-teardown
Open

Cancel in-progress reorders when the list is torn down#630
elizrus wants to merge 1 commit into
square:mainfrom
elizrus:erussell/pcm-2178-cancel-reorder-on-teardown

Conversation

@elizrus

@elizrus elizrus commented Aug 5, 2026

Copy link
Copy Markdown

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 UICollectionView interactive-movement session, which UIKit expects to be resolved with endInteractiveMovement() or cancelInteractiveMovement(). Today nothing ends that session when the list leaves the window or is deallocated:

  • deinit only nils collectionView.delegate/dataSource.
  • didMoveToWindow() does nothing when window == nil.
  • The only caller of cancelAllInProgressReorders() is the content-diff-apply path (guarded by hasInProgressReorders, 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:

Swift runtime failure: Index out of range
Array.subscript.getter
ListLayoutContent.item(at:)
closure #1 in ListLayoutContent.move(from:to:)   // via CollectionViewLayout.invalidateLayout(with:)

(also observed via CollectionViewLayout.positionForItem(at:)ListView.setPresentationStateItemPositions().)

Change

Cancel any in-progress reorder while the data source and layout are still in sync:

  • In didMoveToWindow() when the list leaves the window (the navigate-away-mid-drag case).
  • In deinit as a backstop.

Both are guarded by hasInProgressReorders, so they are no-ops in the common case. hasInProgressReorders is relaxed from private to internal so tests can assert on it.

Testing

Adds ListView.ReorderTeardownTests covering 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

  • Ensure any public-facing changes are reflected in the changelog. Include them in the Main section.

@elizrus elizrus closed this Aug 17, 2026
@elizrus elizrus reopened this Aug 20, 2026
@elizrus
elizrus marked this pull request as ready for review August 20, 2026 15:29
@elizrus
elizrus force-pushed the erussell/pcm-2178-cancel-reorder-on-teardown branch from 60194b8 to 75c6b67 Compare August 20, 2026 20:50
@elizrus

elizrus commented Aug 20, 2026

Copy link
Copy Markdown
Author

[AI] 🤖 Posted by Elizabeth's AI agent.

Ready for a maintainer to land. Quick status to make that easy:

  • Branch is current with main (94c710a7, the 17.4.0 bump) — 0 commits behind, mergeable, no conflicts. No rebase needed.
  • CI is gated on the fork. The Tests workflow is sitting in action_required (standard fork-PR gate), so it needs a maintainer to Approve and run before it can go green. The lint checks (Semgrep, zizmor) already pass.
  • Heads up on local validation: I couldn't run the test suite locally, so I'm relying on CI to validate. The change adds ListView.ReorderTeardownTests; the window-teardown test red-greens the fix (fails on main, passes with the change). If CI surfaces anything, I'll turn it around quickly.

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 → Index out of range in ListLayoutContent). Once this merges and is released, we'll bump the pin downstream and drop the patch.

Happy to squash, adjust the test, or split anything out if that helps it land — just say the word.

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>
@elizrus
elizrus force-pushed the erussell/pcm-2178-cancel-reorder-on-teardown branch from 75c6b67 to 3b71bfb Compare August 21, 2026 20:39
@elizrus

elizrus commented Aug 21, 2026

Copy link
Copy Markdown
Author

[AI] 🤖 Strengthened the test coverage to make the regression case more convincing:

  • test_reorder_interrupted_by_navigation_does_not_crash (new) drives a real interactive-movement session through ListView.beginReorder(for:) — the same entry point a drag uses, which calls collectionView.beginInteractiveMovementForItem(at:) — then removes the list from the window mid-drag and applies an index-affecting content update. This reproduces the original crash path: before the fix, the still-open session resolves the move against now-stale index paths and traps out-of-range in ListLayoutContent; with the fix, leaving the window cancels the session first, so the update applies cleanly. Written in the existing ..._does_not_crash idiom (cf. test_changing_to_empty_frame_does_not_crash).
  • test_reorder_is_cancelled_when_list_leaves_window keeps the explicit behavioral assertion (hasInProgressReorders / item.isReordering go false on window exit).
  • test_reorder_in_progress_does_not_crash_on_deinit covers the deinit teardown path plus no-retain-cycle via a weak reference.

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 does_not_crash test is deliberately shaped so its only failure mode is an actual crash — it can't go falsely red from the headless environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant