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

[CIS-1213] Fix custom Components being not retrievable #1519

Merged
merged 4 commits into from
Oct 8, 2021

Conversation

evsaev
Copy link
Contributor

@evsaev evsaev commented Oct 6, 2021

🔗 Issue

CIS-1213

🎯 Goal

Fix responder chain being cutoff when ComposerVC/ChatMessageListVC controllers lifecycle is running which leads to the subviews of wrong types being instantiated.

Fix the lifecycle of ChatMessageContentView being triggered before a message cell is part of view hierarchy.

🛠 Implementation

The base view controller was updated to fallback to parent view controller if the responder chain backed by view hierarchy has not yet been established.

🧪 Testing

The PR contains unit tests added that check the responder chain is not cutoff at the moment when child controllers instantiate their subviews. The unit test that at the moment makes CI red is fixed by the changes done in ChatMessageCell/ChatMessageContentView.

🎨 Changes

Screenshot 2021-10-07 at 20 48 08

☑️ Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Affected documentation updated (docusaurus, tutorial, CMS (task created)

@evsaev evsaev added 🐞 Bug An issue or PR related to a bug 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK labels Oct 6, 2021
@evsaev evsaev self-assigned this Oct 6, 2021
@evsaev evsaev changed the title Fix Components being not retrievable Fix custom Components being not retrievable Oct 6, 2021
@evsaev evsaev force-pushed the CIS-1213-broken-responder-chain branch from 937b91b to 99936ea Compare October 6, 2021 13:22
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #1519 (118e43f) into main (48313cb) will decrease coverage by 0.03%.
The diff coverage is 29.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1519      +/-   ##
==========================================
- Coverage   74.36%   74.32%   -0.04%     
==========================================
  Files         231      231              
  Lines       10781    10796      +15     
==========================================
+ Hits         8017     8024       +7     
- Misses       2764     2772       +8     
Flag Coverage Δ
llc-tests 74.32% <29.41%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ListController/ChannelListController+Combine.swift 68.42% <0.00%> (-31.58%) ⬇️
...ListController/ChannelListController+SwiftUI.swift 68.42% <0.00%> (-31.58%) ⬇️
.../ChannelListController/ChannelListController.swift 33.50% <ø> (+0.34%) ⬆️
...ources/StreamChat/Workers/ChannelListUpdater.swift 93.33% <100.00%> (+1.33%) ⬆️
...StreamChat/Database/DTOs/ChannelListQueryDTO.swift 90.00% <0.00%> (+10.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f10fe03...118e43f. Read the comment docs.

@nuno-vieira
Copy link
Member

It seems that some components still have problems 🤔

@evsaev evsaev force-pushed the CIS-1213-broken-responder-chain branch from 99936ea to 118e43f Compare October 6, 2021 15:47
@evsaev
Copy link
Contributor Author

evsaev commented Oct 6, 2021

It seems that some components still have problems 🤔

ChatMessageContentView was updated to trigger the layout when the it's added as a subview. Two tests were red because the content view was not added to the view hierarchy. It's fixed now so let's see if CI is green🤞

nuno-vieira
nuno-vieira previously approved these changes Oct 6, 2021
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

b-onc
b-onc previously approved these changes Oct 7, 2021
Copy link
Contributor

@b-onc b-onc left a comment

Choose a reason for hiding this comment

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

CHANGELOG?

Sources/StreamChatUI/CommonViews/BaseViews.swift Outdated Show resolved Hide resolved
@evsaev evsaev changed the title Fix custom Components being not retrievable [CIS-1213] Fix custom Components being not retrievable Oct 7, 2021
@evsaev evsaev dismissed stale reviews from b-onc and nuno-vieira via 09a1022 October 7, 2021 16:21
@evsaev evsaev force-pushed the CIS-1213-broken-responder-chain branch from 118e43f to 09a1022 Compare October 7, 2021 16:21
@evsaev evsaev force-pushed the CIS-1213-broken-responder-chain branch from 09a1022 to f823686 Compare October 7, 2021 17:29
nuno-vieira
nuno-vieira previously approved these changes Oct 7, 2021
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

LGTM

@nuno-vieira
Copy link
Member

@evsaev I still find it weird, why this happens. Customers are not complaining they can't customize these components. Also, we have these components customized in our sample apps. Why is this happening only on the tests?

@evsaev
Copy link
Contributor Author

evsaev commented Oct 7, 2021

@evsaev I still find it weird, why this happens. Customers are not complaining they can't customize these components. Also, we have these components customized in our sample apps. Why is this happening only on the tests?

Customers do not face with this issues because they inject components via Components.default and it works well. If they tried to use custom components in specific view sub-tree they would see it is broken.

In most of snapshot tests UI components are sand-boxed so the ChannelListVC was actually the only place where the responder chain is long enough.

@nuno-vieira
Copy link
Member

@evsaev I still find it weird, why this happens. Customers are not complaining they can't customize these components. Also, we have these components customized in our sample apps. Why is this happening only on the tests?

Customers do not face with this issues because they inject components via Components.default and it works well. If they tried to use custom components in specific view sub-tree they would see it is broken.

In most of snapshot tests UI components are sand-boxed so the ChannelListVC was actually the only place where the responder chain is long enough.

Ahhhh now it all makes sense 🙌

Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@evsaev evsaev merged commit aafbdb0 into main Oct 8, 2021
@evsaev evsaev deleted the CIS-1213-broken-responder-chain branch October 8, 2021 10:08
nuno-vieira pushed a commit that referenced this pull request Oct 21, 2021
* Fix `willMove(to parent)` called twice

* Fix responder chain being cutoff for child controllers

* Fix responder chain being cutoff for message cell

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug An issue or PR related to a bug 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants