fix: multi screen views action handling #920
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. |
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report vs. main branch |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #920 +/- ##
==========================================
+ Coverage 61.04% 62.95% +1.90%
==========================================
Files 165 165
Lines 7281 7304 +23
==========================================
+ Hits 4445 4598 +153
+ Misses 2836 2706 -130 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Shahroz16 Can we please add steps to reproduce the bug we are fixing so we can help test the fix 😇 |
@mahmoud-elmorabea added 🙌 |
There was a problem hiding this comment.
The changes look good but unfortunately, I am still unable to reproduce the issue on main even on a real device and triggering the message mentioned in PR description multiple times.
Can you please add a video or steps to reproduce to make sure that this branch fixes the issue?
|
@mahmoud-elmorabea added the video in description. |
mrehan27
approved these changes
May 23, 2025
|
@claude review |
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes view removal handling via a new lifecycle delegate, moves dismissal logic into context-specific managers, and adds tests to ensure correct behavior.
- Introduce
GistViewLifecycleDelegateto notify managers when aGistViewis removed. - Refactor
GistView.removeFromSuperview()to remove direct dismissal and delegate action toInlineMessageManagerorModalMessageManager. - Enhance test coverage with lifecycle and delegate tests and improve
ThreadUtilStubto track and reset thread calls.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/Shared/Stub/ThreadUtilStub.swift | Added flags (runMainCalled, runBackgroundCalled) and a reset() method to support testing thread execution. |
| Tests/MessagingInApp/Gist/Views/GistViewLifecycleDelegateTests.swift | New tests verifying that GistView notifies its lifecycle delegate when removed. |
| Tests/MessagingInApp/Gist/GistDelegateImplTests.swift | Updated tests to inject ThreadUtilStub and confirm UI-thread dispatch in delegate. |
| Sources/MessagingInApp/Gist/Views/GistView.swift | Added GistViewLifecycleDelegate protocol and delegate call in removeFromSuperview(). |
| Sources/MessagingInApp/Gist/Managers/InlineMessageManager.swift | Set lifecycleDelegate on the GistView and dismiss inline messages on removal. |
| Sources/MessagingInApp/Gist/Managers/ModalMessageManager.swift | Implement GistViewLifecycleDelegate to skip auto-dismiss for modal messages. |
Comments suppressed due to low confidence (1)
Tests/Shared/Stub/ThreadUtilStub.swift:17
- Add a unit test to verify that ThreadUtilStub.runBackground(_:) correctly sets the runBackgroundCalled flag when the block is executed.
public func runBackground(_ block: @escaping () -> Void) {
Comment on lines
29
to
+34
| super.removeFromSuperview() | ||
| if let _ = message { | ||
| DIGraphShared.shared.gistProvider.dismissMessage() | ||
| } | ||
|
|
||
| // Notify lifecycle delegate that this view is being removed | ||
| // The delegate (InlineMessageManager or ModalMessageManager) can decide | ||
| // what action to take based on the context | ||
| lifecycleDelegate?.gistViewWillRemoveFromSuperview(self) |
There was a problem hiding this comment.
[nitpick] Consider notifying the lifecycle delegate before calling super.removeFromSuperview() so the delegate can act while the view hierarchy is still intact.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses multi screen views action handling issues by refactoring view dismissal logic and introducing a new lifecycle delegate protocol. The changes include:
- Adding the GistViewLifecycleDelegate protocol and integrating it into GistView for managing view removal events.
- Refactoring message dismissal logic to differentiate between modal and inline message managers.
- Enhancing test coverage for Gist components covering lifecycle events and delegate interactions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Shared/Stub/ThreadUtilStub.swift | Added public flags and a reset function for better thread execution testing. |
| Tests/MessagingInApp/Gist/Views/GistViewLifecycleDelegateTests.swift | Introduced tests to validate the new lifecycle delegate behavior. |
| Tests/MessagingInApp/Gist/GistDelegateImplTests.swift | Extended test coverage incorporating the updated event handling flow. |
| Sources/MessagingInApp/Gist/Views/GistView.swift | Updated removeFromSuperview to notify the lifecycle delegate instead of calling dismissMessage directly. |
| Sources/MessagingInApp/Gist/Managers/ModalMessageManager.swift | Configured lifecycle delegate handling for modal messages with no auto-dismissal. |
| Sources/MessagingInApp/Gist/Managers/InlineMessageManager.swift | Ensured inline messages trigger dismissal via the lifecycle delegate when removed. |
Comments suppressed due to low confidence (1)
Sources/MessagingInApp/Gist/Views/GistView.swift:28
- Consider calling lifecycleDelegate?.gistViewWillRemoveFromSuperview(self) before calling super.removeFromSuperview() to better reflect the 'will remove' semantics of the protocol.
override public func removeFromSuperview() {
github-actions bot
pushed a commit
that referenced
this pull request
May 27, 2025
## [3.10.2](3.10.1...3.10.2) (2025-05-27) ### Bug Fixes * multi screen views action handling ([#920](#920)) ([0426e06](0426e06))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Changes:
Testing:
Testing Instructions:
Here is a link to the multi screen message that we can use to reproduce on
mainbranch, when tested on real device and in this branch you should not face it.Video created from
mainbefore the fixin-app.mov