Skip to content

Commit

Permalink
Fix memory leak in Chat module
Browse files Browse the repository at this point in the history
This commit introduces:
- Fixing strong reference cycle in ChatView;
- DismissManager struct allowing to take UIKit dismiss completion block execution under control;
- new unit test;
- getting rid of expectation usage in EngagementCoordinator tests.

MOB-3843
  • Loading branch information
Egor Egorov committed Dec 3, 2024
1 parent 59d21b6 commit 380017f
Show file tree
Hide file tree
Showing 16 changed files with 157 additions and 116 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ extension SecureConversations.TranscriptModel {
}

if let item = items.last, case .gvaQuickReply(_, let button, _, _) = item.kind {
let props = button.options.map { self.quickReplyOption($0) }
let props = button.options.compactMap { [weak self] in self?.quickReplyOption($0) }
self.action?(.quickReplyPropsUpdated(.shown(props)))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ extension EngagementCoordinator.Environment {
alertManager: .mock(),
queuesMonitor: .mock(),
pendingSecureConversationStatusUpdates: { $0(.success(false)) },
createEntryWidget: { _ in .mock() }
createEntryWidget: { _ in .mock() },
dismissManager: .init { _, _, _ in }
)
}
#endif
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ extension EngagementCoordinator {
var queuesMonitor: QueuesMonitor
var pendingSecureConversationStatusUpdates: CoreSdkClient.PendingSecureConversationStatusUpdates
var createEntryWidget: EntryWidgetBuilder
var dismissManager: GliaPresenter.DismissManager
}
}

Expand Down Expand Up @@ -103,7 +104,8 @@ extension EngagementCoordinator.Environment {
alertManager: alertManager,
queuesMonitor: queuesMonitor,
pendingSecureConversationStatusUpdates: environment.coreSdk.pendingSecureConversationStatusUpdates,
createEntryWidget: createEntryWidget
createEntryWidget: createEntryWidget,
dismissManager: environment.dismissManager
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ extension Glia {
var processInfo: ProcessInfoHandling
var cameraDeviceManager: CoreSdkClient.GetCameraDeviceManageable
var isAuthenticated: () -> Bool
var dismissManager: GliaPresenter.DismissManager
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ extension Glia.Environment {
debugPrint(#function, "isAuthenticated:", error.localizedDescription)
return false
}
},
dismissManager: .init { viewController, animated, completion in
viewController.dismiss(animated: animated, completion: completion)
}
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ extension Glia.Environment {
snackBar: .mock,
processInfo: .mock(),
cameraDeviceManager: { .mock },
isAuthenticated: { false }
isAuthenticated: { false },
dismissManager: .init { _, _, _ in }
)
}

Expand Down
7 changes: 5 additions & 2 deletions GliaWidgets/Sources/Presenter/GliaPresenter.Environment.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ extension GliaPresenter {
struct Environment {
var appWindowsProvider: AppWindowsProvider
var log: CoreSdkClient.Logger
var dismissManager: DismissManager
}
}

Expand All @@ -17,7 +18,8 @@ extension GliaPresenter.Environment {
uiApplication: environment.uiApplication,
sceneProvider: sceneProvider
),
log: environment.log
log: environment.log,
dismissManager: environment.dismissManager
)
}

Expand All @@ -31,7 +33,8 @@ extension GliaPresenter.Environment {
uiApplication: environment.uiApplication,
sceneProvider: sceneProvider
),
log: log
log: log,
dismissManager: environment.dismissManager
)
}
}
35 changes: 27 additions & 8 deletions GliaWidgets/Sources/Presenter/GliaPresenter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,32 @@ final class GliaPresenter {
completion?()
return
}
presentingViewController.dismiss(
environment.dismissManager.dismiss(
presentingViewController,
animated: animated,
completion: completion
)
}
}

#if DEBUG
extension GliaPresenter.Environment {
static let mock = Self(
appWindowsProvider: .mock,
log: .mock
)
extension GliaPresenter {
// DismissManaged is used to control dismiss completion delayed by CoreAnimation.
struct DismissManager {
var dismissViewControllerAnimateWithCompletion: (
_ viewController: UIViewController,
_ animated: Bool,
_ completion: (() -> Void)?
) -> Void

func dismiss(
_ viewController: UIViewController,
animated: Bool,
completion: (() -> Void)? = nil
) {
dismissViewControllerAnimateWithCompletion(viewController, animated, completion)
}
}
}
#endif

extension GliaPresenter {
struct AppWindowsProvider {
Expand All @@ -111,6 +122,14 @@ extension GliaPresenter.AppWindowsProvider {
}

#if DEBUG
extension GliaPresenter.Environment {
static let mock = Self(
appWindowsProvider: .mock,
log: .mock,
dismissManager: .init { _, _, _ in }
)
}

extension GliaPresenter.AppWindowsProvider {
static let mock = Self(windows: { [] })
}
Expand Down
6 changes: 3 additions & 3 deletions GliaWidgets/Sources/View/Chat/ChatView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,8 @@ extension ChatView: WebMessageCardViewDelegate {
}

private func isLastMessage(_ messageId: MessageRenderer.Message.Identifier) -> Bool {
let numberOfSections = { self.numberOfSections?() ?? 0 }
let numberOfRows = { section in self.numberOfRows?(section) ?? 0 }
let numberOfSections = { [weak self] in self?.numberOfSections?() ?? 0 }
let numberOfRows = { [weak self] section in self?.numberOfRows?(section) ?? 0 }
let sections = (0 ..< numberOfSections()).reversed()

guard let section = sections.first(where: { numberOfRows($0) > 0 }) else { return false }
Expand Down Expand Up @@ -723,7 +723,7 @@ extension ChatView {
let choiceCard = ChoiceCard(with: message, isActive: isActive)
view.showsOperatorImage = showsImage
view.setOperatorImage(fromUrl: imageUrl, animated: false)
view.onOptionTapped = { self.choiceOptionSelected($0, message.id) }
view.onOptionTapped = { [weak self] in self?.choiceOptionSelected($0, message.id) }
view.appendContent(.choiceCard(choiceCard), animated: false)
return .choiceCard(view)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ extension EngagementCoordinator.Environment {
createEntryWidget: { _ in
fail("\(Self.self).createEntryWidget")
return .mock()
}
},
dismissManager: .failing
)
}
3 changes: 3 additions & 0 deletions GliaWidgetsTests/Glia.Environment.Failing.swift
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ extension Glia.Environment {
isAuthenticated: {
fail("\(Self.self).isAuthenticated")
return false
},
dismissManager: .init { _, _, _ in
fail("\(Self.self).dismissManager")
}
)
}
Expand Down
58 changes: 58 additions & 0 deletions GliaWidgetsTests/Sources/ChatView/ChatViewTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,62 @@ final class ChatViewTest: XCTestCase {
XCTFail("Content should be .choiceCard")
}
}

func test_viewIsReleasedOnceModuleIsClosedWithResponseCardsInTranscript() throws {
guard #available(iOS 17, *) else {
throw XCTSkip("""
This test does not pass on OS lower than iOS 17, but actual fix work well.
So it was decided to leave it for local usage and for future,
once target device version will be bumped.
""")
}
var coordinatorEnv = EngagementCoordinator.Environment.failing
coordinatorEnv.dismissManager.dismissViewControllerAnimateWithCompletion = { _, _, completion in
completion?()
}
coordinatorEnv.fileManager.urlsForDirectoryInDomainMask = { _, _ in [.mock] }
coordinatorEnv.fileManager.createDirectoryAtUrlWithIntermediateDirectories = { _, _, _ in }
coordinatorEnv.createFileUploadListModel = { _ in .mock() }
let window = UIWindow(frame: .zero)
window.rootViewController = .init()
window.makeKeyAndVisible()
coordinatorEnv.uiApplication.windows = { [window] }
coordinatorEnv.loadChatMessagesFromHistory = { true }
coordinatorEnv.getCurrentEngagement = { nil }
coordinatorEnv.isAuthenticated = { true }
coordinatorEnv.maximumUploads = { 1 }
var logger = CoreSdkClient.Logger.failing
logger.prefixedClosure = { _ in logger }
logger.infoClosure = { _, _, _, _ in }
logger.warningClosure = { _, _, _, _ in }
coordinatorEnv.log = logger

let options: [ChatChoiceCardOption] = [try .mock()]
coordinatorEnv.fetchChatHistory = {
$0(
.success(
[
.mock(attachment: .mock(
type: .singleChoice,
files: [],
imageUrl: nil,
options: options
))
]
)
)
}
let coordinator = EngagementCoordinator.mock(
engagementLaunching: .direct(kind: .chat),
environment: coordinatorEnv
)
coordinator.start()

weak var controller = try XCTUnwrap(coordinator.navigationPresenter.viewControllers.last as? ChatViewController)
weak var viewModel = try XCTUnwrap(controller?.viewModel.engagementModel as? ChatViewModel)
viewModel?.event(.closeTapped)

XCTAssertNil(controller)
XCTAssertNil(viewModel)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ extension EngagementCoordinatorTests {
let callCoordinator = coordinator.coordinators.first { $0 is CallCoordinator } as? CallCoordinator
callCoordinator?.delegate?(.back)

callAfterTimeout {
XCTAssertTrue(calledEvents.contains(.minimized))
}
XCTAssertTrue(calledEvents.contains(.minimized))
}

func test_callDelegateBackCallFromChat() throws {
Expand All @@ -66,19 +64,16 @@ extension EngagementCoordinatorTests {
let callCoordinator = coordinator.coordinators.first { $0 is CallCoordinator } as? CallCoordinator
callCoordinator?.delegate?(.back)

callAfterTimeout {
let topmostViewController = coordinator.navigationPresenter.viewControllers.first as? ChatViewController
let topmostViewController = coordinator.navigationPresenter.viewControllers.first as? ChatViewController

XCTAssertNotNil(topmostViewController)
}
XCTAssertNotNil(topmostViewController)
}

func test_callDelegateEngaged() throws {
coordinator = createCoordinator(withKind: .videoCall)

coordinator.start()

try showGliaViewController()
let callCoordinator = try XCTUnwrap(coordinator.coordinators.first { $0 is CallCoordinator } as? CallCoordinator)

callCoordinator.delegate?(.engaged(operatorImageUrl: URL.mock.absoluteString))
Expand All @@ -103,8 +98,6 @@ extension EngagementCoordinatorTests {
let callCoordinator = coordinator.coordinators.first { $0 is CallCoordinator } as? CallCoordinator
callCoordinator?.delegate?(.minimize)

callAfterTimeout {
XCTAssertEqual(calledEvents.last, .minimized)
}
XCTAssertEqual(calledEvents.last, .minimized)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@ extension EngagementCoordinatorTests {

let flowCoordinator = coordinator.coordinators.last

callAfterTimeout {
XCTAssertNil(flowCoordinator)
XCTAssertEqual(coordinator.coordinators.count, 0)
}
XCTAssertNil(flowCoordinator)
XCTAssertEqual(coordinator.coordinators.count, 0)
}

func test_secureConversationsDelegateBackTapped() throws {
Expand All @@ -45,9 +43,7 @@ extension EngagementCoordinatorTests {
let secureConversationsCoordinator = coordinator.coordinators.first as? SecureConversations.Coordinator
secureConversationsCoordinator?.delegate?(.backTapped)

callAfterTimeout {
XCTAssertTrue(calledEvents.contains(.minimized))
}
XCTAssertTrue(calledEvents.contains(.minimized))
}

func test_secureConversationsDelegateChat() throws {
Expand All @@ -60,14 +56,11 @@ extension EngagementCoordinatorTests {
}

coordinator.start()
try showGliaViewController()

let secureConversationsCoordinator = coordinator.coordinators.first as? SecureConversations.Coordinator
secureConversationsCoordinator?.delegate?(.chat(.back))

callAfterTimeout {
XCTAssertTrue(calledEvents.contains(.minimized))
}
XCTAssertTrue(calledEvents.contains(.minimized))
}

// MARK: - Leave Conversation
Expand Down
Loading

0 comments on commit 380017f

Please sign in to comment.