[DON-2483] Fix busy waiting and improve thread safety in BpkModal and…

[RichSutherSky]

DON-2483 Fix busy waiting and improve thread safety in BpkModal and BpkAppSearchModal

This pull request enhances the modal dismissal flow in the Backpack Compose modal components, making it easier to respond to the end of the dismiss animation and simplifying coroutine usage. The main improvements are the addition of a callback for dismiss animation completion, refactoring of coroutine management, and simplification of modal state transitions.

See the DEMO PR of the applied changes to the main app here: https://github.com/Skyscanner/skyscanner-app/pull/44690

(NOTE: You will need to publish backpack locally and use mavenLocal in the main app branch)

Modal dismissal enhancements:

• Added an onDismissAnimationCompletion callback parameter to both BpkModal and BpkAppSearchModal, allowing consumers to react when the dismiss animation finishes. [1] [2]
• Implemented logic in BpkModal to invoke onDismissAnimationCompletion via a LaunchedEffect that observes the modal's visibility state.

State management and coroutine handling:

• Refactored BpkModalState to remove suspend functions and internal coroutine management in favor of a simpler, callback-based approach using snapshotFlow and a stored coroutine scope.
• Updated rememberBpkModalState to set up and inject a coroutine scope into the modal state for use in animation completion tracking.

Code cleanup:

• Removed unused coroutine imports and the unnecessary rememberCoroutineScope from BpkAppSearchModal.
• Simplified the onClick handler for the close button in BpkAppSearchModal to directly call state.hide().

• Check this if you have read and followed the contributing guidelines

Remember to include the following changes:

• Component README.md
• Tests

If you are curious about how we review, please read through the code review guidelines

[skyscanner-backpack-bot]

	Warnings
⚠️	One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️	One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.
⚠️	One or more component files were updated, but README.md wasn't updated. If your change contains API changes/additions or a new component please update the relevant component README.

Generated by 🚫 Danger Kotlin against 5abb65b

[skyscanner-backpack-bot]

	Warnings
⚠️	One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️	One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.
⚠️	One or more component files were updated, but README.md wasn't updated. If your change contains API changes/additions or a new component please update the relevant component README.

Generated by 🚫 Danger Kotlin against 54a79b0

[Copilot]

Pull request overview

Enhances Backpack Compose modal dismissal by removing busy-waiting, introducing a dismiss-animation completion callback, and simplifying modal coroutine usage/state transitions.

Changes:

• Added onDismissAnimationCompletion to BpkModal and BpkAppSearchModal.
• Refactored BpkModalState to use callback-based hide completion tracking instead of suspend/busy-waiting.
• Updated demo/story usages to call state.hide() directly (no per-call coroutine scopes).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/modal/BpkModalState.kt	Removes busy-waiting and adds callback-based hide completion tracking with a stored scope.
backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/modal/BpkModal.kt	Adds onDismissAnimationCompletion and attempts to observe visibility/idle state to trigger it.
backpack-compose/src/main/kotlin/net/skyscanner/backpack/compose/appsearchmodal/BpkAppSearchModal.kt	Exposes the new completion callback and simplifies close handling to state.hide().
app/src/main/java/net/skyscanner/backpack/demo/compose/ModalStory.kt	Updates demo to call non-suspending hide() directly.
app/src/main/java/net/skyscanner/backpack/demo/compose/ImageGalleryStory.kt	Updates demo to call non-suspending hide() directly.

[Copilot]

The snapshotFlow(...).collect { ... } never completes, so the launched coroutine will remain active and may invoke callback again on subsequent hide completions (even after _pendingHideAnimationCallback is cleared). Use a one-shot terminal operator (e.g., first { it } / take(1)) and consider storing/cancelling a previous Job when hide() is called again to avoid multiple concurrent collectors.

[Copilot]

_pendingHideAnimationCallback is assigned and cleared but never read/used to control behavior in the current diff, which makes the state harder to reason about. Either remove it or use it explicitly (e.g., to prevent multiple callbacks/collectors, or to no-op if a newer callback has replaced an older one).

[Copilot]

Setting the scope via LaunchedEffect(Unit) introduces a timing gap where hide(onHidden=...) can run before _scope is set, causing the completion callback to never be observed (and _pendingHideAnimationCallback to remain set). Prefer setting the scope synchronously (e.g., via SideEffect { ... } or direct assignment if acceptable) and/or handle the “scope not set yet” case by deferring the subscription until the scope becomes available.

[Henrik Sym (henrik-sky)]

Please add a stress/regression test for BpkModalState#hide to strengthen coverage around repeated visibility transitions and callback lifecycle behavior.

Scope

• Repeats show() -> hide(onHidden) many times with distinct callbacks.
• Runs additional show() -> hide() cycles without callbacks.
• Exercises rapid successive state changes to mimic real interaction patterns.

Validation

• Asserts each onHidden callback is invoked exactly once.
• Ensures callbacks from earlier cycles do not leak into later cycles or fire multiple times.

Why this is useful

hide() registers callback-related flow collection logic, so this test acts as a regression guard against stale collectors or duplicate callback invocation when hide/show is called repeatedly.

[skyscanner-backpack-bot]

	Warnings
⚠️	One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️	One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

Generated by 🚫 Danger Kotlin against 3cb3678

[skyscanner-backpack-bot]

	Warnings
⚠️	One or more component files were updated, but the tests weren't updated. If your change is not covered by existing tests please add snapshot tests.
⚠️	One or more component files were updated, but the docs screenshots weren't updated. If the changes are visual or it is a new component please regenerate the screenshots via ./gradlew recordScreenshots.

Generated by 🚫 Danger Kotlin against 0b77344