From 9b13455edcdef04669883488ae122727967e1adf Mon Sep 17 00:00:00 2001 From: Muukii Date: Thu, 9 Apr 2026 22:51:14 +0900 Subject: [PATCH 1/4] Fix BatchRemovingTransitionContext completion being called multiple times When multiple StackingPlatterViews shared the same BatchRemovingTransitionContext, each swapTransitionContext call would invoke invalidate() on the shared context, causing completion callbacks to fire N times instead of once. Changes: - Add hasNotifiedCompletion flag to ensure callbacks fire exactly once - Add isInvalidated/isCompleted guard to notifyCompleted() (matching RemovingTransitionContext) - Add isInvalidated guard in onCompleted closure (matching _startRemoving pattern) - Add tests verifying completion is called exactly once Co-Authored-By: Claude Opus 4.6 (1M context) --- .../BatchRemovingTransitionContext.swift | 9 ++- .../ViewController/FluidStackController.swift | 12 ++-- .../FluidStackControllerTests.swift | 62 +++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift b/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift index fc03e5f6c..de6abd12c 100644 --- a/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift +++ b/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift @@ -36,6 +36,7 @@ public final class BatchRemovingTransitionContext: TransitionContext { private let onCompleted: (BatchRemovingTransitionContext) -> Void private var childContexts: [ChildContext] = [] + private var hasNotifiedCompletion: Bool = false private var callbacks: [(CompletionEvent) -> Void] = [] init( @@ -56,6 +57,7 @@ public final class BatchRemovingTransitionContext: TransitionContext { public func notifyCompleted() { assert(Thread.isMainThread) + guard isInvalidated == false, isCompleted == false else { return } isCompleted = true onCompleted(self) } @@ -91,6 +93,8 @@ public final class BatchRemovingTransitionContext: TransitionContext { override func invalidate() { assert(Thread.isMainThread) isInvalidated = true + guard hasNotifiedCompletion == false else { return } + hasNotifiedCompletion = true callbacks.forEach { $0(.interrupted) } } @@ -106,6 +110,9 @@ public final class BatchRemovingTransitionContext: TransitionContext { Triggers ``addCompletionEventHandler(_:)`` with ``TransitionContext/CompletionEvent/succeeded`` */ func transitionSucceeded() { - callbacks.forEach{ $0(.succeeded) } + assert(Thread.isMainThread) + guard hasNotifiedCompletion == false else { return } + hasNotifiedCompletion = true + callbacks.forEach { $0(.succeeded) } } } diff --git a/Sources/FluidStack/ViewController/FluidStackController.swift b/Sources/FluidStack/ViewController/FluidStackController.swift index 90e2582da..a41e4d120 100644 --- a/Sources/FluidStack/ViewController/FluidStackController.swift +++ b/Sources/FluidStack/ViewController/FluidStackController.swift @@ -718,13 +718,17 @@ open class FluidStackController: UIViewController { fromViewControllers: itemsToRemove.map(\.viewController), toViewController: targetTopItem?.viewController, onCompleted: { [weak self] context in - + assert(Thread.isMainThread) - + guard let self = self else { - return + return } - + + guard context.isInvalidated == false else { + return + } + /** Completion of transition, cleaning up */ diff --git a/Tests/FluidStackTests/FluidStackControllerTests.swift b/Tests/FluidStackTests/FluidStackControllerTests.swift index 0d1194a71..9c181fded 100644 --- a/Tests/FluidStackTests/FluidStackControllerTests.swift +++ b/Tests/FluidStackTests/FluidStackControllerTests.swift @@ -565,6 +565,68 @@ final class FluidStackControllerTests: XCTestCase { } + /// Tests that batch removing completion is called exactly once. + /// Previously, when multiple StackingPlatterViews shared the same + /// BatchRemovingTransitionContext, each swapTransitionContext call + /// would fire invalidate() on the shared context, causing completion + /// to be called N times. + func testBatchRemovingCompletionCalledOnce() { + + let stack = FluidStackController() + + let vc1 = UIViewController() + let vc2 = UIViewController() + let vc3 = UIViewController() + let vc4 = UIViewController() + + stack.addContentViewController(vc1, transition: .disabled) + stack.addContentViewController(vc2, transition: .disabled) + stack.addContentViewController(vc3, transition: .disabled) + stack.addContentViewController(vc4, transition: .disabled) + + XCTAssertEqual(stack.stackingViewControllers.count, 4) + + var completionCallCount = 0 + + // Remove vc2, which is not on top -> triggers batch removing of [vc2, vc3, vc4] + stack.removeViewController( + vc2, + transition: nil, + transitionForBatch: .disabled, + completion: { event in + completionCallCount += 1 + } + ) + + XCTAssertEqual(stack.stackingViewControllers.count, 1) + XCTAssertEqual(completionCallCount, 1, "Completion should be called exactly once") + } + + /// Tests that removeAllViewController completion is called exactly once. + func testRemoveAllViewControllerCompletionCalledOnce() { + + let stack = FluidStackController() + + stack.addContentViewController(UIViewController(), transition: .disabled) + stack.addContentViewController(UIViewController(), transition: .disabled) + stack.addContentViewController(UIViewController(), transition: .disabled) + stack.addContentViewController(UIViewController(), transition: .disabled) + + XCTAssertEqual(stack.stackingViewControllers.count, 4) + + var completionCallCount = 0 + + stack.removeAllViewController( + transition: .disabled, + completion: { event in + completionCallCount += 1 + } + ) + + XCTAssertEqual(stack.stackingViewControllers.count, 1) + XCTAssertEqual(completionCallCount, 1, "Completion should be called exactly once") + } + final class ContentTypeOption: UIViewController { init(contentType: FluidStackContentConfiguration.ContentType) { super.init(nibName: nil, bundle: nil) From 60e0e1f75065e9a67b879dd972ef3626fe854613 Mon Sep 17 00:00:00 2001 From: Muukii Date: Thu, 9 Apr 2026 23:21:10 +0900 Subject: [PATCH 2/4] Simplify guards to match RemovingTransitionContext pattern and fix test - Remove hasNotifiedCompletion flag, use isInvalidated guard instead - Use XCTestExpectation for async batch transition test (crossDissolve) - Place stack in UIWindow so animation completes properly Co-Authored-By: Claude Opus 4.6 (1M context) --- .../BatchRemovingTransitionContext.swift | 7 +---- .../FluidStackControllerTests.swift | 26 +++++++++---------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift b/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift index de6abd12c..188abe309 100644 --- a/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift +++ b/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift @@ -36,7 +36,6 @@ public final class BatchRemovingTransitionContext: TransitionContext { private let onCompleted: (BatchRemovingTransitionContext) -> Void private var childContexts: [ChildContext] = [] - private var hasNotifiedCompletion: Bool = false private var callbacks: [(CompletionEvent) -> Void] = [] init( @@ -92,9 +91,8 @@ public final class BatchRemovingTransitionContext: TransitionContext { /// Triggers ``addCompletionEventHandler(_:)`` with ``TransitionContext/CompletionEvent/interrupted`` override func invalidate() { assert(Thread.isMainThread) + guard isInvalidated == false else { return } isInvalidated = true - guard hasNotifiedCompletion == false else { return } - hasNotifiedCompletion = true callbacks.forEach { $0(.interrupted) } } @@ -110,9 +108,6 @@ public final class BatchRemovingTransitionContext: TransitionContext { Triggers ``addCompletionEventHandler(_:)`` with ``TransitionContext/CompletionEvent/succeeded`` */ func transitionSucceeded() { - assert(Thread.isMainThread) - guard hasNotifiedCompletion == false else { return } - hasNotifiedCompletion = true callbacks.forEach { $0(.succeeded) } } } diff --git a/Tests/FluidStackTests/FluidStackControllerTests.swift b/Tests/FluidStackTests/FluidStackControllerTests.swift index 9c181fded..11a7b78a0 100644 --- a/Tests/FluidStackTests/FluidStackControllerTests.swift +++ b/Tests/FluidStackTests/FluidStackControllerTests.swift @@ -566,13 +566,14 @@ final class FluidStackControllerTests: XCTestCase { } /// Tests that batch removing completion is called exactly once. - /// Previously, when multiple StackingPlatterViews shared the same - /// BatchRemovingTransitionContext, each swapTransitionContext call - /// would fire invalidate() on the shared context, causing completion - /// to be called N times. + /// When fluidPop triggers batch removing (VC is not on top), + /// the completion must fire exactly once after the batch transition completes. func testBatchRemovingCompletionCalledOnce() { + let window = UIWindow() let stack = FluidStackController() + window.rootViewController = stack + window.makeKeyAndVisible() let vc1 = UIViewController() let vc2 = UIViewController() @@ -586,17 +587,16 @@ final class FluidStackControllerTests: XCTestCase { XCTAssertEqual(stack.stackingViewControllers.count, 4) + let exp = expectation(description: "completion called") + exp.assertForOverFulfill = true var completionCallCount = 0 - // Remove vc2, which is not on top -> triggers batch removing of [vc2, vc3, vc4] - stack.removeViewController( - vc2, - transition: nil, - transitionForBatch: .disabled, - completion: { event in - completionCallCount += 1 - } - ) + vc2.fluidPop { _ in + completionCallCount += 1 + exp.fulfill() + } + + wait(for: [exp], timeout: 2) XCTAssertEqual(stack.stackingViewControllers.count, 1) XCTAssertEqual(completionCallCount, 1, "Completion should be called exactly once") From ab0fd5fda2b8a01882a7977586c9f01651093ec3 Mon Sep 17 00:00:00 2001 From: Muukii Date: Fri, 10 Apr 2026 01:03:20 +0900 Subject: [PATCH 3/4] Use hasNotifiedCompletion flag for mutual exclusion between invalidate and transitionSucceeded Ensures callbacks fire exactly once regardless of which path triggers first. Both invalidate() and transitionSucceeded() check and set the flag before firing callbacks, preventing re-entry from either direction. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Transition/BatchRemovingTransitionContext.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift b/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift index 188abe309..6e1f31e1c 100644 --- a/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift +++ b/Sources/FluidStack/Transition/BatchRemovingTransitionContext.swift @@ -36,6 +36,7 @@ public final class BatchRemovingTransitionContext: TransitionContext { private let onCompleted: (BatchRemovingTransitionContext) -> Void private var childContexts: [ChildContext] = [] + private var hasNotifiedCompletion: Bool = false private var callbacks: [(CompletionEvent) -> Void] = [] init( @@ -91,8 +92,9 @@ public final class BatchRemovingTransitionContext: TransitionContext { /// Triggers ``addCompletionEventHandler(_:)`` with ``TransitionContext/CompletionEvent/interrupted`` override func invalidate() { assert(Thread.isMainThread) - guard isInvalidated == false else { return } isInvalidated = true + guard hasNotifiedCompletion == false else { return } + hasNotifiedCompletion = true callbacks.forEach { $0(.interrupted) } } @@ -108,6 +110,8 @@ public final class BatchRemovingTransitionContext: TransitionContext { Triggers ``addCompletionEventHandler(_:)`` with ``TransitionContext/CompletionEvent/succeeded`` */ func transitionSucceeded() { + guard hasNotifiedCompletion == false else { return } + hasNotifiedCompletion = true callbacks.forEach { $0(.succeeded) } } } From 2e9b976a5c10d9392b17f9a63d775ab13577e80d Mon Sep 17 00:00:00 2001 From: Muukii Date: Fri, 10 Apr 2026 01:04:46 +0900 Subject: [PATCH 4/4] Apply same hasNotifiedCompletion guard to RemovingTransitionContext Prevent re-entry between invalidate(), transitionSucceeded(), and notifyCancelled() so callbacks fire exactly once. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../FluidStack/Transition/RemovingTransitionContext.swift | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Sources/FluidStack/Transition/RemovingTransitionContext.swift b/Sources/FluidStack/Transition/RemovingTransitionContext.swift index 4b2666283..2c80d1091 100644 --- a/Sources/FluidStack/Transition/RemovingTransitionContext.swift +++ b/Sources/FluidStack/Transition/RemovingTransitionContext.swift @@ -26,6 +26,7 @@ public final class RemovingTransitionContext: TransitionContext { private let onRequestedDisplayOnTop: (DisplaySource) -> FluidStackController.DisplayingOnTopSubscription + private var hasNotifiedCompletion: Bool = false private var callbacks: [(CompletionEvent) -> Void] = [] init( @@ -57,6 +58,7 @@ public final class RemovingTransitionContext: TransitionContext { assert(Thread.isMainThread) guard isInvalidated == false, isCompleted == false else { return } isInvalidated = true + hasNotifiedCompletion = true callbacks.forEach { $0(.cancelled) } onAnimationCompleted(self) } @@ -97,6 +99,8 @@ public final class RemovingTransitionContext: TransitionContext { override func invalidate() { assert(Thread.isMainThread) isInvalidated = true + guard hasNotifiedCompletion == false else { return } + hasNotifiedCompletion = true callbacks.forEach { $0(.interrupted) } } @@ -115,6 +119,8 @@ public final class RemovingTransitionContext: TransitionContext { Triggers ``addCompletionEventHandler(_:)`` with ``TransitionContext/CompletionEvent/succeeded`` */ func transitionSucceeded() { + guard hasNotifiedCompletion == false else { return } + hasNotifiedCompletion = true callbacks.forEach { $0(.succeeded) } }