Skip to content

fix: Ensure we're referencing up-to-date swipe state when configuring interaction enabled #595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 31, 2025

Conversation

robmaceachern
Copy link
Member

@robmaceachern robmaceachern commented Jul 30, 2025

For https://block.atlassian.net/browse/UI-9091

This resolves an issue where a cell would not respond to taps after the "full swipe" swipe action trigger.

The issue was that we were referencing stale swipe state when configuring isUserInteractionEnabled. It was quite unintuitive when debugging this, but the line swipeState = state immediately resulted in those variables holding different values while processing the swipe action 🙃. We would incorrectly fall into the .willPerformFirstActionAutomatically case, making the content view unresponsive, despite the swipeState reporting as .closed.

The SwipeActionsViewController has also been updated to demonstrate this is now working as expected.

Checklist

Please do the following before merging:

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

@robmaceachern robmaceachern marked this pull request as ready for review July 30, 2025 21:51
@@ -286,8 +286,11 @@ extension ItemCell {
private func set(state: SwipeActionState, animated: Bool = false) {
swipeState = state

// Warning! : `swipeState` may be mutated to a new value via it's property
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's an example of this happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the original case it was: willPerformFirstActionAutomatically(ListableUI.SwipeActionsView.Side.right) becoming closed.

We get into this kind of re-entrant set(state:) situation if the swipe action completion is invoked synchronously.

image

I'm not sure if there's a better/safer way to restructure this stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh interesting – synchronous completion handlers strike again... Got it!

There's probably some improvement, but this also seems like a conceptually fine/correct fix too!

Copy link
Contributor

@johnnewman-square johnnewman-square left a comment

Choose a reason for hiding this comment

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

Nice find!

@robmaceachern robmaceachern merged commit 1d15048 into main Jul 31, 2025
3 checks passed
@robmaceachern robmaceachern deleted the robmaceachern/swipe-action-fix branch July 31, 2025 14:06
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