test(paywalls): cover video KVO crash fix (#6985)#6988
Closed
facumenzella wants to merge 2 commits into
Closed
Conversation
Add PlayerLayerBackedView and VideoPlayerLayerView to render looping/auto-playing videos via AVPlayerLayer for the no-controls path, avoiding AVKit's internal AVPlayerController KVO observers. Update VideoPlayerView to choose between AVPlayerViewController when controls are shown and the new AVPlayerLayer path when hidden. Refactor VideoPlayerUIView to move player lifecycle into its Coordinator, implement manual looping for the AVPlayerViewController path (to avoid AVPlayerLooper-driven KVO issues), disable video frame analysis on iOS 16+, manage/restore audio session category, and deterministically tear down players and observers during dismantle/deinit.
Add structural tests for the looping-video crash fix: - controls path (AVPlayerViewController) never builds an AVQueuePlayer, so an AVPlayerLooper-driven queue player is never handed to AVKit's internal AVPlayerController (the #6985 teardown crash) - no-controls path uses an AVQueuePlayer via AVPlayerLayer, which has no AVPlayerController and is safe with AVPlayerLooper - PlayerLayerBackedView is backed by AVPlayerLayer Tests go through makeCoordinator(), covering the player-ownership seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 tasks
❌ Failures
The following Swift files were added but are missing from To fix: open |
Generated by 🚫 Danger |
870b3e1 to
b4575c1
Compare
Member
Author
|
Folded these directly into alex/fix-avfoundation-kvo-crash (commit 58ba665) per @alexrepty, so no need to stack. Closing. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Stacked on top of #6987 (targets
alex/fix-avfoundation-kvo-crash, notmain).#6987 fixes the looping-video KVO teardown crash but ships without automated coverage, and the core invariant is cheap to pin. This adds structural tests so the fix can't silently regress:
testControlsLoopingVideoDoesNotUseQueuePlayeris the actual root cause: theAVPlayerViewControllerpath must never build anAVQueuePlayer(anAVPlayerLooper-driven queue player handed to AVKit's internalAVPlayerControlleris what crashes on teardown, 🐛 Paywall looping video crashes with NSInternalInconsistencyException — AVQueuePlayer + AVPlayerLooper handed to AVPlayerViewController (KVO currentItem.status) #6985).testNoControlsLoopingVideoUsesQueuePlayer+testPlayerLayerBackedViewIsBackedByAVPlayerLayerguard the no-controls path: it renders viaAVPlayerLayer(noAVPlayerController), where the queue player + looper are safe.They assert through
makeCoordinator(), so they also cover the player-ownership seam #6987 introduced.Verified locally on
iPhone 16 Pro (18.4)via theRevenueCatUITestsscheme: 5 tests, 0 failures.AI session context
AI Context
Metadata
facu/paywall-video-kvo-crash-tests(basealex/fix-avfoundation-kvo-crash)Goal
Add automated coverage for the looping-video crash fix in #6987 so the fix is regression-proof. No production code changed; tests only.
Initial Prompt
Review issue #6985 (looping Paywalls V2 video crashes with
NSInternalInconsistencyException), then compare an independently-built fix against the official PR #6987, and push for test coverage on #6987 by drafting the missing tests. (Opened as "check this out https://.../issues/6985".)Important Follow-up Prompts
Agent Contribution
VideoPlayerViewTests.swift(5 tests) targeting Use AVPlayerLayer for no-controls; fix KVO crash #6987's API (VideoPlayerUIView,VideoPlayerLayerView,PlayerLayerBackedView,Coordinator.player).alex/fix-avfoundation-kvo-crash, regenerated the Tuist workspace, ran the tests.Human Decisions
Key Implementation Decisions
makeCoordinator()and checkplayer is AVQueuePlayer/ layer backing.AVQueuePlayertoAVPlayerViewController; no-controls usesAVPlayerLayer) is, and it sits on the ownership seam Use AVPlayerLayer for no-controls; fix KVO crash #6987 added.Files / Symbols Touched
Tests/RevenueCatUITests/PaywallsV2/VideoPlayerViewTests.swiftVideoPlayerUIView,VideoPlayerLayerView,PlayerLayerBackedView,Coordinator.player.Dependencies / Config / Migrations
Validation
tuist generate --no-open: successxcodebuild test ... -only-testing:RevenueCatUITests/VideoPlayerViewTests: 5 tests, 0 failuresswiftlint lint: cleanAVQueuePlayer; the layer types did not exist).Validation Gaps
iPhone 16 Pro, iOS 18.4); not exercised across the SDK's full OS matrix locally.Review Focus
Risks / Reviewer Notes
Coordinator.player).@testable importmakes this intentional and stable; it's the genuine owner of the player.Non-goals / Out of Scope
Omitted Context