Skip to content

Fix for infinite loop when combining Stack and Tree navigation #2289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 16, 2023
Merged

Fix for infinite loop when combining Stack and Tree navigation #2289

merged 4 commits into from
Jul 16, 2023

Conversation

@benlings
Copy link
Contributor

@benlings benlings commented Jul 14, 2023

This fixes #2288

@benlings
Copy link
Contributor Author

benlings commented Jul 14, 2023

I'm happy to add some tests for this, but would need some pointers on where they should go. Maybe extending NavigationStackTestCaseView?

@mbrandonw
Copy link
Member

mbrandonw commented Jul 14, 2023

Hi @benlings, thanks for the PR! This looks like a great fix, and we'd definitely like to take you up on your offer to write a test. We actually have a slim (but growing) integration test suite in the project:

https://github.com/pointfreeco/swift-composable-architecture/tree/main/Examples/Integration/Integration

How do you feel about adding a new "…TestCase.swift" file to the project that demonstrates the problem? You can even just get it most of the way and then we can do any last bits of clean up.

Thanks!

@benlings benlings force-pushed the fix-navigation-infinite-loop branch from 9a25eb5 to 9201982 Compare July 15, 2023 18:52
@benlings
Copy link
Contributor Author

benlings commented Jul 16, 2023 •
edited
Loading

Thanks for the pointers @mbrandonw . I looked at creating a new test case, but it would have duplicated most of the existing NavigationStackTestCase for the initial setup. I've instead added the navigationDestination alongside the existing alert presentation. I'm happy to separate it if you prefer through 🙂.

benlings
benlings commented Jul 16, 2023
View reviewed changes
Sources/ComposableArchitecture/SwiftUI/NavigationStackStore.swift Outdated
var body: some View {
self.destination(component)
.environment(\.navigationDestinationType, State.self)
.id(component.id)
Copy link
Contributor Author

@benlings benlings Jul 16, 2023

Choose a reason for hiding this comment

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

Looking at this again, I'm not sure if this id modifier should go here or inside the destination block of navigationDestination(...)? What is this id used for @mbrandonw ?

@mbrandonw
Copy link
Member

mbrandonw commented Jul 16, 2023

Hi @benlings, thanks for taking a stab at the test. However, if I revert your NavigationDestinationView change and run the integration app, the drill down works fine. I open the nav stack sheet, then tap "Go to counter", then tap "Open navigation destination", and it drills down. No infinite loop.

Is there something else that needs to be done to trigger the bug?

@stephencelis
Copy link
Member

stephencelis commented Jul 16, 2023

@mbrandonw I think the bug is fixed in iOS 17, but if you try in Xcode 14.3/iOS 16, it should happen.

@mbrandonw
Copy link
Member

mbrandonw commented Jul 16, 2023

Ahhh right ok. Will take another look in a minute.

@mbrandonw
Copy link
Member

mbrandonw commented Jul 16, 2023

Ok, I was able to confirm the bug in 14.3 and confirm that the test passes with the change. Will merge once it's green!

@benlings
Copy link
Contributor Author

benlings commented Jul 16, 2023

Thanks @mbrandonw and @stephencelis! I hadn’t tested this on iOS 17, so wasn’t aware of the fix there. I had seen the bug on 16.2-16.4.

@stephencelis stephencelis merged commit c113ba4 into pointfreeco:main Jul 16, 2023
cgrindel-self-hosted-renovate bot referenced this pull request in cgrindel/rules_swift_package_manager Jul 18, 2023
@cgrindel-self-hosted-renovate
chore(deps): update dependency pointfreeco/swift-composable-architect…
da65028 
…ure to from: "0.56.0" (#489)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[pointfreeco/swift-composable-architecture](https://togithub.com/pointfreeco/swift-composable-architecture)
| minor | `from: "0.55.1"` -> `from: "0.56.0"` |

---

### Release Notes

<details>
<summary>pointfreeco/swift-composable-architecture
(pointfreeco/swift-composable-architecture)</summary>

###
[`v0.56.0`](https://togithub.com/pointfreeco/swift-composable-architecture/releases/tag/0.56.0)

[Compare
Source](https://togithub.com/pointfreeco/swift-composable-architecture/compare/0.55.1...0.56.0)

#### What's Changed

- Added: `TestStore.useMainSerialExecutor`, a configurable option to
make testing code using Swift concurrency more reliable
([https://github.com/pointfreeco/swift-composable-architecture/pull/2301](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2301)
). This feature uses a newly-extracted library, [Concurrency
Extras](https://togithub.com/pointfreeco/swift-concurrency-extras).
- Added: `Store.send` can now be configured with an animation or
transaction (thanks
[@&#8203;HarshilShah](https://togithub.com/HarshilShah),
[https://github.com/pointfreeco/swift-composable-architecture/pull/2241](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2241)).
- Added: `TestStoreOf` type alias for specifying a test store of a
reducer (thanks [@&#8203;brzzdev](https://togithub.com/brzzdev),
[https://github.com/pointfreeco/swift-composable-architecture/pull/2277](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2277)).
- Changed: `ViewStoreTask.cancel` is now synchronous
([https://github.com/pointfreeco/swift-composable-architecture/pull/2282](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2282)).
- Changed: Closure arguments for various APIs have been given names,
improving documentation and Xcode completion
([https://github.com/pointfreeco/swift-composable-architecture/pull/2295](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2295)).
- Fixed: Effect cancellation IDs should no longer nest into deep
`AnyHashable`s, fixing potential bugs
([https://github.com/pointfreeco/swift-composable-architecture/pull/2283](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2283)).
- Fixed: `navigationDestination(store:)` dismissal should now work more
reliably (thanks [@&#8203;tplaymeow](https://togithub.com/tplaymeow),
[https://github.com/pointfreeco/swift-composable-architecture/pull/2210](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2210);
[https://github.com/pointfreeco/swift-composable-architecture/pull/2284](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2284)).
- Fixed: Added a workaround for a SwiftUI bug involving nested stack and
tree navigation (thanks
[@&#8203;benlings](https://togithub.com/benlings),
[https://github.com/pointfreeco/swift-composable-architecture/pull/2289](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2289)).
- Deprecated: `TestStore` scoping has been deprecated
([https://github.com/pointfreeco/swift-composable-architecture/pull/2292](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2292)).
- Infrastructure: Added case study for tree-based navigation with for
multiple destinations using enum state (thanks
[@&#8203;tiagopigatto](https://togithub.com/tiagopigatto),
[https://github.com/pointfreeco/swift-composable-architecture/pull/2231](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2231))

#### New Contributors

- [@&#8203;HarshilShah](https://togithub.com/HarshilShah) made their
first contribution in
[https://github.com/pointfreeco/swift-composable-architecture/pull/2241](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2241)
- [@&#8203;benlings](https://togithub.com/benlings) made their first
contribution in
[https://github.com/pointfreeco/swift-composable-architecture/pull/2289](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2289)
- [@&#8203;tplaymeow](https://togithub.com/tplaymeow) made their first
contribution in
[https://github.com/pointfreeco/swift-composable-architecture/pull/2210](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2210)
- [@&#8203;brzzdev](https://togithub.com/brzzdev) made their first
contribution in
[https://github.com/pointfreeco/swift-composable-architecture/pull/2277](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2277)
- [@&#8203;tiagopigatto](https://togithub.com/tiagopigatto) made their
first contribution in
[https://github.com/pointfreeco/swift-composable-architecture/pull/2231](https://togithub.com/pointfreeco/swift-composable-architecture/pull/2231)

**Full Changelog**:
pointfreeco/swift-composable-architecture@0.55.1...0.56.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi45LjEiLCJ1cGRhdGVkSW5WZXIiOiIzNi45LjEiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Combining Stack-based and Tree-based .navigationDestination navigation leads to infinite loop

3 participants

@benlings @mbrandonw @stephencelis