Skip to content
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

Fix crash upon SwiftUICollectionViewCell deinit and add some more breadcrumbs and optimization in HomeViewModel snapshot calculation and rendering #1053

Merged
merged 5 commits into from
Jul 29, 2024

Conversation

Gio2018
Copy link
Collaborator

@Gio2018 Gio2018 commented Jul 27, 2024

Goal

  • SwiftUICollectionViewCell cell retained unnecessary hosting controller to embed a SwiftUI view inside a UIKit UIView
  • moreover, it attempted to manually destroy the hosting vc upon denit (also not necessary), causing crashes sometimes. Removing that reference and the code inside deinit entirely, should address the crash
  • Add some more breadcrumbs to track more precisely the NSInternalInconsistencyException crash that occurs in Home
  • Fix a broken logic, still in Home, that caused repeated snapshot updates when anything in homeViewController.traitCollection changed...

… controller

- this cell retained unnecessary hosting controller to embed a SwiftUI view inside a UIKit UIView
- moreover, it attempted to manually destroy the hosting vc upon denit (also not necessary), causing crashes sometimes
@Gio2018 Gio2018 added the bug Something isn't working label Jul 27, 2024
@Gio2018 Gio2018 added this to the 8.15.0 milestone Jul 27, 2024
@Gio2018 Gio2018 self-assigned this Jul 27, 2024
@Gio2018 Gio2018 requested a review from bassrock as a code owner July 27, 2024 07:18
@pocket-ci
Copy link
Contributor

pocket-ci commented Jul 27, 2024

Messages
📖 No SwiftLint violations! 🎉
📖 Project coverage: 37.12%
📖 Checking XCode Environment Variables
📖 Edited 5 files
📖 Created 0 files

PocketKit: Coverage: 63.1

File Coverage
CollectionViewController.swift 78.9%
ItemsListViewController.swift 77.36%
HomeViewController.swift 69.92%
EmptyStateView.swift 84.24%
HomeViewModel.swift 65.01%

PocketKitTests: Coverage: 28.27

File Coverage
HomeViewModel.swift 43.84% ⚠️
HomeViewController.swift 45.4% ⚠️
CollectionViewController.swift 0.0% ⚠️
EmptyStateView.swift 0.0% ⚠️
ItemsListViewController.swift 67.3%

Generated by 🚫 Danger Swift against 292b62c

@Gio2018 Gio2018 marked this pull request as draft July 28, 2024 00:10
@Gio2018 Gio2018 marked this pull request as ready for review July 28, 2024 00:19
@Gio2018 Gio2018 changed the title Fix crash upon SwiftUICollectionViewCell deinit Fix crash upon SwiftUICollectionViewCell deinit and add some more breadcrumbs and optimization in HomeViewModel snapshot calculation and rendering Jul 29, 2024
Copy link
Collaborator

@nzeltzer nzeltzer left a comment

Choose a reason for hiding this comment

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

lgtm

@Gio2018 Gio2018 merged commit 3fda3bd into develop Jul 29, 2024
8 checks passed
@Gio2018 Gio2018 deleted the fix/crash-swiftui-cell branch July 29, 2024 18:48
Copy link

sentry-io bot commented Aug 5, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ App Hanging: App hanging for at least 2000 ms. HomeViewController View Issue
  • ‼️ App Hanging: App hanging for at least 2000 ms. HomeViewController View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants