Skip to content

Plumb presence token refresh through to the app - #57

Open
dhinesh-kumar-m wants to merge 1 commit into
mainfrom
feat/presence-token-plumbing
Open

Plumb presence token refresh through to the app#57
dhinesh-kumar-m wants to merge 1 commit into
mainfrom
feat/presence-token-plumbing

Conversation

@dhinesh-kumar-m

Copy link
Copy Markdown

Summary

  • Adds onPresenceTokenExpired prop and setNewPresenceToken command, mirroring the existing onAccessTokenExpired/setNewAccessToken pattern, so RN apps can resolve a fresh presence token on a 401 from the native heartbeat loop.
  • Android (TPStreamsRNPlayerView.kt, TPStreamsRNPlayerViewManager.kt): wires TPStreamsPlayer.Listener.onPresenceTokenExpired, exports the event, adds the setNewPresenceToken command.
  • iOS (TPStreamsRNPlayerView.swift, TPStreamsRNPlayerViewManager.m): discovered playerVC.delegate was never being set — added it, plus a TPStreamPlayerViewControllerDelegate conformance with the 4 required no-op fullscreen methods and a real presenceTokenExpired(forVideo:completion:) implementation.
  • JS/TS (TPStreamsPlayer.tsx, TPStreamsPlayerViewNativeComponent.ts): new prop type, event handler, command wiring. If the app doesn't set onPresenceTokenExpired, we resolve with '' by default (matches the native side's own backoff-not-hang default) instead of leaving the native callback dangling.
  • Bumps pinned native SDK versions with TODO comments (not real version numbers) pointing at TPStreamsAndroidPlayer#feat/presence-sdk-integration and iOSPlayerSDK#170, which need to merge and ship first.

Verification

  • yarn install — clean
  • yarn typecheck — passes
  • yarn test — passes (existing stub test suite)
  • yarn lint — pre-existing failure (structuredClone is not defined, Node 16 vs ESLint config needing 17+), unrelated to this change
  • Not build-verified against the actual native SDKs since those changes haven't shipped yet — see "Not done here."

Not done here

  • Native SDK builds/runtime testing against real TPStreamsAndroidPlayer/TPStreamsSDK releases containing the presence changes — those are still in review (see linked PRs).

Related: testpress/TPStreamsAndroidPlayer#feat/presence-sdk-integration, testpress/iOSPlayerSDK#170

Adds the RN-side half of TPStreamsAndroidPlayer/iOSPlayerSDK's new
onPresenceTokenExpired callback, mirroring exactly how onAccessTokenExpired
is already threaded through: native emits a direct event
(onPresenceTokenExpired, via TPStreamsPlayerViewNativeComponent's Fabric
codegen), the JS layer awaits the app's callback and resolves it back via a
new Command (setNewPresenceToken). No heartbeat logic lives here — that's
entirely internal to the native players now, automatic on play/pause. This
is only the request/response plumbing for the one thing that needs the
app's help: getting a fresh token on a 401.

Android: TPStreamsRNPlayerView.kt/ViewManager.kt add the event constant,
listener override, and Command handler exactly parallel to the access-token
versions. The manager interface these implement
(TPStreamsRNPlayerViewManagerInterface) is Fabric-codegen output from
TPStreamsPlayerViewNativeComponent.ts, regenerated automatically at build
time (not committed here, unlike Flutter's Pigeon) — the updated spec is
what makes setNewPresenceToken exist on it.

iOS: TPStreamsRNPlayerView.swift did not have TPStreamPlayerViewController's
delegate wired to anything before this — playerVC.delegate was never set, so
there was no way to reach the new presenceTokenExpired hook. Set it and
added conformance to TPStreamPlayerViewControllerDelegate; the four
full-screen callbacks it requires (no default body, unlike presenceTokenExpired)
are left as no-ops rather than newly surfacing full-screen events RN did not
have before. Left the pre-existing onAccessTokenExpired/TokenRequestDelegate
path (a separate, DRM-license-renewal-specific mechanism) untouched.

Verified for real: yarn typecheck and yarn test both pass (Node 16.15.0,
yarn invoked directly via .yarn/releases/yarn-3.6.1.cjs since no yarn binary
is on PATH here). yarn lint fails on a pre-existing environment mismatch
unrelated to this change (its ESLint config needs Node 17+'s
structuredClone; nothing here touches lint config). Could not verify the
native Kotlin/Swift/ObjC++ — no Android/Xcode toolchain in this environment,
and both are additionally blocked on the native SDK dependency pins
(TPStreamsAndroidPlayer 1.2.6, TPStreamsSDK 1.2.36), which predate the
presence APIs entirely. Left TODO comments at both pins rather than
guessing a future version number.
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Claude finished @dhinesh-kumar-m's task in 3m 58s —— View job


Reviewing PR #57: Plumb presence token refresh through to the app

Todo List

  • Gather context (read CLAUDE.md, diff, changed files)
  • Review Android changes (Kotlin)
  • Review iOS changes (Swift/Obj-C)
  • Review JS/TS changes
  • Post final review summary with verdict

Summary

Reviewed the full diff against origin/main (8 files, +110/-2): Android (TPStreamsRNPlayerView.kt, TPStreamsRNPlayerViewManager.kt), iOS (TPStreamsRNPlayerView.swift, TPStreamsRNPlayerViewManager.m), and JS/TS (TPStreamsPlayer.tsx, TPStreamsPlayerViewNativeComponent.ts), plus the podspec/gradle.properties TODO-comment bumps. The Android and JS/TS implementations mirror the existing onAccessTokenExpired/setNewAccessToken pattern faithfully and look correct. The playerVC.delegate = self fix on iOS is a legitimate bug fix (delegate was genuinely never wired up before).

Blocking issues found:

  1. iOS: setNewPresenceToken command is not routed to the view — will fail at runtime.
    ios/TPStreamsRNPlayerViewManager.m:40 declares RCT_EXTERN_METHOD(setNewPresenceToken:(nonnull NSNumber *)node newToken:(nonnull NSString *)newToken), and TPStreamsRNPlayerView.swift:430 adds the view-level setNewPresenceToken(_:) method. But the bridging module that actually dispatches this command, ios/TPStreamsRNPlayerViewManager.swift, was not updated in this PR. Compare setNewAccessToken (TPStreamsRNPlayerViewManager.swift:100-108), which has a manager-level @objc func setNewAccessToken(_ node: NSNumber, newToken: NSString) that resolves the view via bridge.uiManager.view(forReactTag:) and forwards the call — setNewPresenceToken has no such counterpart. Since the manager class doesn't implement the selector the extern declaration promises, calling Commands.setNewPresenceToken(...) from JS on iOS will fail (unrecognized selector / silent no-op depending on RN's dispatch handling) instead of ever reaching TPStreamsRNPlayerView.setNewPresenceToken(_:). This means the entire iOS half of this feature is non-functional as written — every presence-token refresh resolution from JS will be dropped on iOS.
    Fix: add a setNewPresenceToken(_ node: NSNumber, newToken: NSString) method to TPStreamsRNPlayerViewManager.swift mirroring setNewAccessToken.

  2. This PR will not compile against the currently pinned native SDK versions.
    android/gradle.properties and TPStreamsRNPlayerView.podspec are left at their existing pinned versions (1.2.6 / 1.2.36) with only TODO comments added — the PR body itself confirms these versions "predate" TPStreamsPlayer.Listener.onPresenceTokenExpired (Android) and TPStreamPlayerViewControllerDelegate.presenceTokenExpired (iOS). As written, override fun onPresenceTokenExpired(...) in TPStreamsRNPlayerView.kt:175 and extension TPStreamsRNPlayerView: TPStreamPlayerViewControllerDelegate in TPStreamsRNPlayerView.swift:463 reference APIs that don't exist in the pinned SDK releases yet. If this lands on main before TPStreamsAndroidPlayer#feat/presence-sdk-integration and iOSPlayerSDK#170 ship and the pinned versions here are bumped accordingly, the library will fail to build for every consumer (not just presence users) — this is a build break, not a feature-scoped risk.
    Recommendation: hold this PR out of main (or merge behind a state where CI can't pick it up) until the companion native SDK PRs are released and the version pins are bumped in the same change, so main never sits in a non-compiling state.

Non-blocking observations:

  • presenceTokenCallback (Android) and pendingPresenceTokenCompletion (iOS) are written from the native heartbeat callback and read/cleared from the JS-triggered command — same unsynchronized-var pattern as the existing accessTokenCallback/pendingTokenCompletion, so not a new regression, but worth keeping in mind since heartbeat-driven callbacks fire repeatedly/on a timer rather than once per playback session, which increases the exposure window.
  • iOS cleanupPlayer() doesn't clear pendingPresenceTokenCompletion (or the pre-existing pendingTokenCompletion) when the player is torn down/recreated — a stale completion closure is silently dropped rather than resolved. Pre-existing pattern for access token, just now duplicated for presence token.

Regression Assessment

No impact on existing playback, DRM, downloads, seeking, track selection, or other public APIs — this is purely additive (new optional prop, new command, new event). The only regression risk is the build-breakage described in finding #2, and the iOS delegate wiring fix (playerVC.delegate = self) is additive/no-op for the four fullscreen callbacks, so it doesn't change existing fullscreen behavior.

Final Verdict

CHANGES REQUESTED

The Android and JS/TS work is solid and consistent with existing conventions, but the iOS command wiring is incomplete (finding #1) and the PR is not safe to merge into main until it compiles against real, shipped native SDK versions (finding #2). Both are addressable — recommend fixing #1 now, and gating the merge/version bump on the companion SDK PRs for #2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant