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.

@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.

@kyleve kyleve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apparently lost my permissions for this repo but LGTM, nice catch

@johnnewman-square johnnewman-square left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@johnnewman-square

Copy link
Copy Markdown
Collaborator

This PR will fix the CI issues: #631

johnnewman-square added a commit that referenced this pull request Sep 8, 2026
## 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.
@elizrus
elizrus force-pushed the erussell/pcm-2178-cancel-reorder-on-teardown branch from 3b71bfb to 62d6960 Compare September 8, 2026 16:09
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 62d6960 to 09d50f9 Compare September 8, 2026 21:16
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.

3 participants