fix(ios): stop RN loading overlay sticking after license renewal

[Balamurugan-100]

Summary

• Limit the RN bridge loading indicator to initial player setup only
• Stop driving the overlay from timeControlStatus == .waitingToPlayAtSpecifiedRate during playback/renewal
• Hide the overlay in setNewAccessToken after offline DRM renewal credentials are delivered

Why

After offline DRM license expiry, JS refreshes the token and native renewal succeeds, but the RN overlay spinner (added in ba94ee0) stays stuck. The native SDK spinner recovers correctly via AVPlayerItem buffer KVO; the bridge overlay does not.

Back-navigation works because remounting restarts the initial-load spinner cycle.

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @Balamurugan-100's task in 2m 45s —— View job

---

Todo

• Gather context (PR body, diff, changed files)
• Read ios/TPStreamsRNPlayerView.swift full file and diff
• Review for correctness, regressions, lifecycle issues
• Post final review summary with verdict

Summary

Reviewed ios/TPStreamsRNPlayerView.swift (the only changed file, +10/-19) against origin/main. The change replaces isPlayerReady with hasCompletedInitialLoad and:

• Stops the timeControlStatus KVO observer (setupPlayingStateObserver, line 256) from touching the loading overlay — it now only emits onIsPlayingChanged.
• Gates showLoadingIndicator()/hideLoadingIndicator() in the player.status observer (setupPlayerStateObserver, lines 264-296) behind !hasCompletedInitialLoad, so the overlay only reacts during the very first load.
• Adds an explicit hideLoadingIndicator() in setNewAccessToken (line 419) after an offline DRM renewal completion is delivered.

Root cause check: The old bug is real — .waitingToPlayAtSpecifiedRate fires during offline license renewal stalls just like it does during initial buffering, and isPlayerReady/timeControl-driven show/hide had no way to distinguish "waiting to start" from "waiting to resume after renewal," so the overlay could get stuck showing (or drift out of sync) after renewal. Decoupling the overlay from timeControlStatus entirely and confining it to the one-shot hasCompletedInitialLoad flag removes that class of bug. hasCompletedInitialLoad is correctly reset to false at the top of setupPlayer() (line 99), so remounts/re-setup restore the initial-load spinner cycle as intended.

Threading: All mutations of hasCompletedInitialLoad and calls to showLoadingIndicator()/hideLoadingIndicator() happen inside DispatchQueue.main.async KVO callbacks or from setNewAccessToken, which the RN bridge invokes on the main thread — no data races introduced.

Regression risk: Low. Online (non-offline) mid-stream buffering/token-refresh stalls will no longer trigger the RN bridge overlay (only the native SDK's own buffering UI will be visible), which matches the PR's stated intent that the bridge overlay is for initial setup only and the native SDK spinner already handles in-flight buffering/renewal recovery. Worth confirming visually that there's no jarring gap: on initial load the overlay now hides as soon as player.status == .readyToPlay, without waiting for timeControlStatus to leave .waitingToPlayAtSpecifiedRate (previously it waited). If the native player UI's own spinner doesn't cover that brief window, there could be a short flash of unstyled/buffering content between the RN overlay disappearing and playback actually starting — worth eyeballing during manual testing (per the PR's own test plan).

Pre-existing (not introduced by this PR): In setupPlayerStateObserver (lines 264-296), a second timeControlStatusObserver = player.observe(\.timeControlStatus, ...) assignment at the end of the method (lines 290-295) overwrites the reference set by setupPlayingStateObserver, which runs right after it in observePlayerChanges(). This causes whichever timeControlStatusObserver closure was assigned first to be released/invalidated almost immediately, so the corresponding onPlayerStateChanged buffering-state updates driven by timeControlStatus (via mapPlayerStateToAndroid(status:timeControlStatus:)) likely never fire after the initial call. This predates this PR and is unrelated to the overlay fix's correctness, but flagging it since it sits directly in the code this PR touches — may be worth a follow-up ticket.

Not verified: No Swift toolchain/build available in this environment, so the change wasn't compiled or run on-device/simulator. The logic was verified by static reading only; recommend the manual test plan in the PR description (short-lived offline license expiry → renewal → confirm overlay behavior) be executed before merge.

Final Verdict

⚠️ APPROVED WITH SUGGESTIONS

The fix is correctly scoped, addresses the stated root cause, and introduces no thread-safety or lifecycle regressions. The two notes above (initial-load hide timing, pre-existing duplicate KVO observer) are non-blocking — worth a look but not reasons to hold this PR.

[claude]

Claude finished @Balamurugan-100's task in 2m 0s —— View job

---

Todo

• Gather context (PR body, diff, changed files)
• Read ios/TPStreamsRNPlayerView.swift full file and diff against origin/main
• Review for correctness, regressions, lifecycle/thread-safety issues
• Post final review summary with verdict

Summary

Reviewed ios/TPStreamsRNPlayerView.swift (only changed file, +10/-19). The change replaces isPlayerReady with a one-shot hasCompletedInitialLoad flag and:

• Stops setupPlayingStateObserver's timeControlStatus KVO (line 256) from touching the loading overlay — it now only emits onIsPlayingChanged.
• Gates showLoadingIndicator()/hideLoadingIndicator() in setupPlayerStateObserver's player.status KVO (lines 264-296) behind !hasCompletedInitialLoad, so the bridge overlay only reacts during the first load.
• Adds an explicit hideLoadingIndicator() in setNewAccessToken (line 419) once an offline DRM renewal completion is delivered.

Root cause fix: Correct and well-scoped. .waitingToPlayAtSpecifiedRate fires during offline license renewal stalls the same way it does during initial buffering, so the old isPlayerReady/timeControlStatus-driven logic had no way to tell "waiting to start" from "waiting to resume after renewal" — that's why the overlay could stick after renewal. Decoupling the overlay entirely from timeControlStatus and confining it to the initial-load window removes that class of bug. hasCompletedInitialLoad is correctly reset at the top of setupPlayer() (line 99), so remounts/back-navigation restore the initial-load spinner cycle as intended.

Findings (non-blocking):

1. Unconditional hideLoadingIndicator() in setNewAccessToken (line 419) — this fires whenever pendingOfflineCredentialsCompletion resolves, with no hasCompletedInitialLoad check. onRequestOfflineLicenseRenewal (line 167) is wired once per setupPlayer() call, and its naming/PR description imply it's only for post-initial renewal — but since TPAVPlayer/DRM credential exchange is implemented in the closed-source TPStreamsSDK dependency, it can't be confirmed statically whether this same callback can also fire during the very first playback session of an offline asset (e.g. if cached credentials are stale) before .readyToPlay. If it can, this line would hide the overlay prematurely mid-initial-load, causing a flash of the black/unstyled view. Worth a quick manual check (fresh offline asset, first play, expired/near-expired cached credentials) alongside the PR's stated renewal test plan.

2. Pre-existing duplicate KVO property (not introduced by this PR, but sits directly in code touched here): lines 256 and 290 both assign to the same timeControlStatusObserver property. observePlayerChanges() (line 225-230) calls setupPlayerStateObserver() before setupPlayingStateObserver(), so the observation created at line 290 is overwritten and released almost immediately when line 256 runs — silently invalidating the buffering-driven onPlayerStateChanged updates that block (via mapPlayerStateToAndroid(status:timeControlStatus:)) was meant to deliver after the first callback. Flagging again since this PR's diff sits right next to it; may be worth splitting into two distinct observer properties in a follow-up.

3. Initial-load hide timing: the overlay now hides as soon as player.status == .readyToPlay (lines 275-279), without waiting for timeControlStatus to leave .waitingToPlayAtSpecifiedRate as it did before. Low risk since the native SDK has its own buffering UI, but worth eyeballing that there's no jarring gap between the RN overlay disappearing and playback actually starting, per the PR's own manual test plan.

Thread-safety: All mutations of hasCompletedInitialLoad and calls to showLoadingIndicator()/hideLoadingIndicator() happen inside DispatchQueue.main.async KVO callbacks or from bridge-invoked methods (main thread) — no data races introduced.

Regression risk: Low. Online (non-offline) mid-stream buffering/token-refresh stalls no longer trigger the RN bridge overlay post-initial-load — intended per the PR description, since the native SDK's own spinner covers in-flight buffering/renewal recovery.

Not verified: No Swift toolchain/Xcode available in this CI environment, so the change wasn't compiled or run on-device/simulator — review is static-reading only. Recommend executing the manual test plan (short-lived offline license expiry → renewal → confirm overlay behavior, plus finding #1 above) before merge.

Final Verdict

⚠️ APPROVED WITH SUGGESTIONS

The fix correctly targets the stated root cause and introduces no thread-safety or lifecycle regressions. The three notes above are non-blocking — #1 is worth a quick manual confirmation given it touches the initial-offline-load path, and #2/#3 are pre-existing/low-risk observations, not reasons to hold this PR.

[claude]

Claude finished @Balamurugan-100's task in 2m 2s —— View job

---

Todo

• Gather context (PR body, diff, changed files)
• Read ios/TPStreamsRNPlayerView.swift full file and diff against origin/main
• Review for correctness, regressions, lifecycle/thread-safety issues
• Post final review summary with verdict

Summary

Reviewed ios/TPStreamsRNPlayerView.swift (only changed file, +10/-19, single commit 2694112, unchanged since prior runs on this PR). The change replaces the isPlayerReady flag with a one-shot hasCompletedInitialLoad flag and:

• Stops setupPlayingStateObserver's timeControlStatus KVO (line 254) from touching the loading overlay — it now only emits onIsPlayingChanged.
• Gates showLoadingIndicator()/hideLoadingIndicator() in setupPlayerStateObserver's player.status KVO (lines 262-294) behind !hasCompletedInitialLoad, so the RN bridge overlay only reacts during the first load.
• Adds an explicit hideLoadingIndicator() in setNewAccessToken (line 412) once an offline DRM renewal completion is delivered.

Root cause: Correctly diagnosed and fixed. .waitingToPlayAtSpecifiedRate fires during offline license renewal stalls the same way it does during initial buffering, so the old logic couldn't distinguish "waiting to start" from "waiting to resume after renewal," which is why the overlay could get stuck. Decoupling the bridge overlay entirely from timeControlStatus and confining it to the initial-load window (via hasCompletedInitialLoad, correctly reset in setupPlayer() line 97) removes that class of bug and matches the PR's stated intent — the native SDK's own spinner already covers in-flight buffering/renewal recovery.

Findings (non-blocking):

1. ios/TPStreamsRNPlayerView.swift:412 — hideLoadingIndicator() in setNewAccessToken fires unconditionally whenever pendingOfflineCredentialsCompletion resolves, with no hasCompletedInitialLoad guard. The callback name onRequestOfflineLicenseRenewal implies it's only wired for post-initial renewal, but since credential exchange lives in the closed-source TPStreamsSDK, it can't be confirmed statically that this same path never fires on the very first playback of an offline asset (e.g. an already-stale cached license on first play). If it can, this line would hide the overlay before player.status reaches .readyToPlay, causing a brief flash of the black/unstyled view. Worth a quick manual check (fresh offline asset, first play, near-expired cached credentials) alongside the PR's stated renewal test plan.
2. Pre-existing, not introduced by this PR but sits in touched code (ios/TPStreamsRNPlayerView.swift:254 and :288) — both setupPlayingStateObserver() and setupPlayerStateObserver() assign to the same timeControlStatusObserver property, and observePlayerChanges() (line 223) calls setupPlayerStateObserver() before setupPlayingStateObserver(). The observation created at line 288 (which drives buffering-aware onPlayerStateChanged updates via mapPlayerStateToAndroid(status:timeControlStatus:)) is overwritten/deallocated almost immediately by the line 254 assignment, so it likely only ever fires its one .initial callback and never reflects subsequent buffering transitions. Consider splitting into two distinct observer properties in a follow-up.

Thread-safety: All mutations of hasCompletedInitialLoad and calls to showLoadingIndicator()/hideLoadingIndicator() happen inside DispatchQueue.main.async KVO callbacks or from bridge-invoked methods on the main thread — no data races introduced.

Regression risk: Low. Online (non-offline) mid-stream buffering/token-refresh stalls no longer trigger the RN bridge overlay post-initial-load — this is intentional per the PR description, since the native SDK's own buffering UI handles that window.

Not verified: No Swift toolchain/Xcode available in this environment, so the change wasn't compiled or run on-device/simulator — review is static-reading only. Recommend the manual test plan (short-lived offline license expiry → renewal → confirm overlay behavior, plus finding #1 above) before merge.

Final Verdict

⚠️ APPROVED WITH SUGGESTIONS

The fix correctly targets the stated root cause and introduces no thread-safety or lifecycle regressions. Finding #1 is worth a quick manual confirmation since it touches the initial-offline-load path; finding #2 is pre-existing and non-blocking.

[claude]

Claude finished @Balamurugan-100's task in 1m 36s —— View job

---

Todo

• Gather context (PR body, diff since origin/main, prior review history on this PR)
• Read ios/TPStreamsRNPlayerView.swift full file and new diff (this is an updated commit — synchronize event)
• Verify whether the two non-blocking findings from earlier reviews on this PR were addressed
• Check CI status
• Post final review summary with verdict

Summary

Reviewed the updated commit on ios/TPStreamsRNPlayerView.swift (+42/-28, single commit, sole changed file) against origin/main. This revision refines the earlier hasCompletedInitialLoad approach and — importantly — resolves both non-blocking findings raised in prior review rounds on this same PR:

1. Duplicate KVO observer overwrite (previously flagged, now fixed): the two timeControlStatus observers previously shared a single timeControlStatusObserver property, so one silently invalidated the other. This revision splits them into distinct properties — playingStateObserver (line 24, used only for onIsPlayingChanged) and playbackStateTimeControlObserver (line 25, used for onPlayerStateChanged + completeInitialLoadIfReady) — each correctly invalidated in removeObservers() (lines 135-140). Both now fire on every subsequent timeControlStatus transition as intended.
2. Premature overlay hide on early renewal (previously flagged, now fixed): setNewAccessToken (lines 426-436) now guards hideLoadingIndicator() behind if hasCompletedInitialLoad. If an offline license renewal resolves before the initial load finishes (e.g., stale cached credentials on first play of a fresh offline asset), the overlay correctly stays up until completeInitialLoadIfReady naturally completes it via the status/timeControl observers — no more risk of a premature flash of unstyled content.

Logic re-verified: completeInitialLoadIfReady(_:) (lines 305-312) is a single, idempotent gate (guard !hasCompletedInitialLoad, status == .readyToPlay, timeControlStatus != .waitingToPlayAtSpecifiedRate) invoked from both the .status observer (on reaching .readyToPlay) and the .timeControlStatus observer (on leaving .waitingToPlayAtSpecifiedRate), so it correctly fires whichever transition completes second. Once set, showLoadingIndicator() is never called again by these paths (the default branch at line 283 is now gated by !hasCompletedInitialLoad), which is exactly what prevents the overlay from reappearing/sticking during mid-stream buffering or DRM renewal — the stated root cause of this PR.

Thread-safety: all mutation of hasCompletedInitialLoad and calls to show/hideLoadingIndicator() still happen inside DispatchQueue.main.async KVO callbacks or from bridge-invoked methods (main thread) — no races introduced by the restructuring.

Regression risk: Low, and lower than the previous revision — the two edge cases identified in earlier review rounds (duplicate observer starvation, premature hide on early renewal) are now closed. Behavior for online playback, non-DRM offline playback, and the normal pendingTokenCompletion refresh path is unchanged.

CI: passing (build/lint job green; this progress-tracking workflow run itself still in progress at review time).

Not verified: No Swift toolchain/Xcode available in this environment, so still static-reading only — no on-device build. Recommend the PR's own manual test plan (short-lived offline license expiry → renewal, plus first-play-with-stale-cached-credentials) be run before merge, though the logic now correctly handles both.

Final Verdict

✅ APPROVED

No blocking issues found. This revision directly addresses both non-blocking suggestions from earlier review rounds on this PR (duplicate KVO observer, premature overlay hide on early renewal), and the core fix correctly resolves the stated root cause without introducing new thread-safety, lifecycle, or regression risk.