feat: Enhanced Picture-in-Picture for iOS#2139
Merged
santhoshvai merged 24 commits intomainfrom Feb 26, 2026
Merged
Conversation
# Conflicts: # packages/react-native-sdk/ios/PictureInPicture/StreamAVPictureInPictureVideoCallViewController.swift # packages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureController.swift # packages/react-native-sdk/ios/RTCViewPip.swift # packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx # packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipNative.tsx # sample-apps/react-native/dogfood/ios/Podfile.lock
This reverts commit e0c43ff.
Route PiP content mutations through a single content-state snapshot path so controller and bridge updates no longer fan out into parallel direct view updates. Made-with: Cursor
Apply PiP content transitions in a guarded batch so renderer property cascades trigger one overlay visibility refresh per content state change. Made-with: Cursor
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a full iOS Picture‑in‑Picture subsystem (UI components, Combine-based content/state, delegate proxy, enforced‑stop adapter, logger), wires rich PiP state through the React Native bridge, and replaces local debounced hooks with RxJS subscriptions in several React components and tests. Changes
Sequence DiagramsequenceDiagram
participant React as RTCViewPipIOS (React)
participant RxJS as participants$ (RxJS)
participant Bridge as RN Bridge
participant iOS as StreamPictureInPictureController
participant State as PictureInPictureContentState
participant UI as PiP UI Components
React->>RxJS: subscribe (debounceTime 300ms)
RxJS-->>React: emit debounced participants
React->>React: compute metadata (name,image,isReconnecting,isScreenSharing,hasAudio,isTrackPaused,isPinned,isSpeaking,connectionQuality)
React->>Bridge: set native props (participantName,...,connectionQuality)
Bridge->>iOS: update controller/view properties
iOS->>State: apply snapshot / update content
State->>iOS: publish content change
iOS->>UI: update avatar/overlay/reconnection/quality views
UI-->>UI: re-render animations/indicators
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-native-sdk/ios/RTCViewPip.swift (1)
167-205:⚠️ Potential issue | 🟡 MinorAdd
deinitto remove notification observer for safety.SwiftLint correctly warns that an object should only remove itself as an observer in
deinit. While removing indidMoveToSuperviewwhensuperview == nilhandles the common case, the observer won't be cleaned up if the view is deallocated while still attached to a superview (e.g., during memory pressure or unexpected cleanup).🛡️ Proposed fix: Add deinit
+ deinit { + NotificationCenter.default.removeObserver(self) + } + override func didMoveToSuperview() { super.didMoveToSuperview() if self.superview == nil { PictureInPictureLogger.log("RTCViewPip has been removed from its superview.") NotificationCenter.default.removeObserver(self)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/RTCViewPip.swift` around lines 167 - 205, The view removes its NotificationCenter observer only in didMoveToSuperview when superview == nil, which can miss cleanup if the view is deallocated while still attached; add a deinit in the RTCViewPip class that calls NotificationCenter.default.removeObserver(self) to guarantee the observer is removed on deallocation, keeping the existing removal in didMoveToSuperview for the common detach path and avoiding duplicate side effects.
🧹 Nitpick comments (15)
packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx (2)
52-55: Inconsistent pattern: mixing hooks and direct subscriptions for the same data domain.The component uses
useLocalParticipant()anduseDominantSpeaker()hooks but switches to direct RxJS subscriptions for remote/all participants. This inconsistency makes the code harder to reason about and maintain.If debouncing is required, consider applying it consistently across all participant-related data or extracting the debounced subscription pattern into a reusable hook.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx` around lines 52 - 55, The component CallParticipantsGrid inconsistently mixes useLocalParticipant/useDominantSpeaker hooks with a direct RxJS subscription for remote/all participants; refactor to use the same hook-based pattern or extract a reusable hook that encapsulates the debounced subscription logic (e.g., create useParticipants or useDebouncedParticipants) and replace the direct subscription with that hook so localParticipant, dominantSpeaker and the remote/all participant list are all sourced consistently via hooks inside CallParticipantsGrid.
62-74: Direct RxJS observable access violates coding guidelines.The coding guidelines state: "Use React hooks from
@stream-io/video-react-bindings(viauseCall()anduseCallStateHooks()) instead of directly accessing RxJS observables from@stream-io/video-client."This implementation directly subscribes to
call.state.remoteParticipants$andcall.state.participants$instead of usinguseRemoteParticipants()anduseParticipants()hooks fromuseCallStateHooks().If debouncing is specifically needed for PiP performance, consider:
- Creating a dedicated hook (e.g.,
useDebouncedParticipants) that encapsulates this pattern- Or documenting why this exception is necessary and adding an eslint-disable comment with rationale
As per coding guidelines: "Use React hooks from
@stream-io/video-react-bindings(viauseCall()anduseCallStateHooks()) instead of directly accessing RxJS observables from@stream-io/video-client"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx` around lines 62 - 74, Replace the direct RxJS subscriptions inside the useEffect (which subscribe to call.state.remoteParticipants$ and call.state.participants$ and call setRemoteParticipants/setAllParticipants) with the React bindings: import and use useCallStateHooks() (useRemoteParticipants and useParticipants) to get participant lists, or encapsulate the debouncing into a new hook (e.g., useDebouncedParticipants) that internally uses useRemoteParticipants/useParticipants and returns debounced values; update CallParticipantsGrid to consume those hooks instead of subscribing to call.state.* observables, or if you must keep the exception, add a minimal eslint-disable with a short rationale and move the debouncing into a dedicated hook named useDebouncedParticipants referenced from the component.packages/react-native-sdk/__tests__/components/CallParticipantsGrid.test.tsx (1)
159-162: Consider adding a constant for the debounce timing values.The test uses hardcoded timing values (300ms and 500ms) that must stay in sync with the component implementation. Consider extracting these as named constants or importing them from a shared location to prevent drift.
Example approach
// In a shared constants file or at the top of the test const PARTICIPANT_DEBOUNCE_MS = 300; const FORCE_UPDATE_DEBOUNCE_MS = 500; // Then use in tests: jest.advanceTimersByTime(PARTICIPANT_DEBOUNCE_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/__tests__/components/CallParticipantsGrid.test.tsx` around lines 159 - 162, Replace hardcoded timing values in the test with named constants to keep them in sync with the component's debounce settings: define (or import) constants like PARTICIPANT_DEBOUNCE_MS and FORCE_UPDATE_DEBOUNCE_MS and use them instead of the literal 300 and 500 in CallParticipantsGrid.test.tsx; ensure the test uses the same source of truth as the component debounce in forceUpdateValue (or the shared constants file) so jest.advanceTimersByTime(...) and any debounceTime usages remain consistent.packages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsx (1)
108-117: Harden debounced updates against first-emission loss on mount.At Line 108 and Line 110,
Subject+useEffectcan drop an earlyforceUpdate()emission becauseSubjectdoes not replay values before subscription attaches. Consider buffering one event to make mount-time behavior deterministic.Proposed change
-import { Subject, debounceTime } from 'rxjs'; +import { ReplaySubject, debounceTime } from 'rxjs'; - const forceUpdate$ = useRef(new Subject<void>()).current; + const forceUpdate$ = useRef(new ReplaySubject<void>(1)).current; const [forceUpdateValue, setForceUpdateValue] = useState(0); useEffect(() => { const sub = forceUpdate$.pipe(debounceTime(500)).subscribe(() => { setForceUpdateValue((v) => v + 1); }); - return () => sub.unsubscribe(); + return () => { + sub.unsubscribe(); + forceUpdate$.complete(); + }; }, [forceUpdate$]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsx` around lines 108 - 117, The debounced force-update can lose an early emission because forceUpdate$ is a plain Subject; change forceUpdate$ to a replaying subject so an emission before the effect subscribes is retained (e.g., replace new Subject<void>() with a ReplaySubject<void>(1) and ensure rxjs ReplaySubject is imported), leaving the existing useEffect subscription, debounceTime, setForceUpdateValue and forceUpdate() (useCallback) logic intact so mount-time forceUpdate() calls are not dropped.packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx (2)
162-167: Consider includingCallingState.JOININGin reconnection states.The reconnection check includes
MIGRATING,RECONNECTING,RECONNECTING_FAILED, andOFFLINE. Depending on UX requirements,JOININGmight also warrant showing the reconnection indicator if the user re-enters a call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx` around lines 162 - 167, The reconnection check in RTCViewPipIOS.tsx (the isReconnecting boolean) currently covers CallingState.MIGRATING, RECONNECTING, RECONNECTING_FAILED, and OFFLINE but omits JOINING; update the isReconnecting expression to include CallingState.JOINING so that the UI treats JOINING as a reconnection state (modify the isReconnecting declaration in RTCViewPipIOS.tsx to OR in CallingState.JOINING).
57-66: Direct RxJS subscription is justified but deviates from the coding guideline.The coding guideline states: "Use React hooks from
@stream-io/video-react-bindings(viauseCall()anduseCallStateHooks()) instead of directly accessing RxJS observables from@stream-io/video-client".This direct subscription with
debounceTime(300)is intentional per the PR summary to replaceuseDebouncedValuehook for performance reasons. Consider adding a brief comment explaining this deviation for future maintainers.📝 Add explanatory comment
// we debounce the participants to avoid unnecessary rerenders // that happen when participant tracks are all subscribed simultaneously + // Note: Direct RxJS subscription used here instead of useCallStateHooks() + // to leverage RxJS debounceTime for better performance (see PR `#2139`) useEffect(() => { if (!call) return; const subscription = call.state.participants$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx` around lines 57 - 66, Add a brief explanatory comment above the useEffect that subscribes to call.state.participants$ (the block using debounceTime(300) and setting setAllParticipants) stating that this direct RxJS subscription intentionally deviates from the guideline to use useCall()/useCallStateHooks() because we need a pared-down debounced pipeline for performance and therefore replaced the previous useDebouncedValue approach; mention the tradeoff and link to the function names (useEffect, call.state.participants$, debounceTime, setAllParticipants) so future maintainers understand this deliberate exception.packages/react-native-sdk/ios/PictureInPicture/PictureInPictureParticipantOverlayView.swift (1)
197-201: Simplify the nil/empty check.The condition can be more readable using optional chaining with
isEmptynegation.♻️ Proposed simplification
private func updateVisibility() { // Show overlay only if enabled and we have a participant name - let hasName = participantName != nil && !(participantName?.isEmpty ?? true) + let hasName = !(participantName?.isEmpty ?? true) isHidden = !isOverlayEnabled || !hasName }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureParticipantOverlayView.swift` around lines 197 - 201, The nil/empty check in updateVisibility is verbose; replace the hasName calculation with an optional-chaining check (e.g., treat participantName?.isEmpty == false) so it reads clearly, then set isHidden = !isOverlayEnabled || !hasName; locate the updateVisibility() method and update the participantName check accordingly (symbols: updateVisibility, participantName, isOverlayEnabled, isHidden).packages/react-native-sdk/ios/RTCViewPip.swift (1)
22-90: Verbose logging in property setters may clutter debug output.Each property setter logs when called, which is helpful for debugging but may produce excessive output when multiple properties change in quick succession. Consider logging only at the
applyCurrentPropertiesToControllerlevel or using a more structured approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/RTCViewPip.swift` around lines 22 - 90, The property setters on RTCViewPip (participantName, participantImageURL, isReconnecting, isScreenSharing, hasAudio, isTrackPaused, isPinned, isSpeaking, connectionQuality) currently call PictureInPictureLogger.log in each didSet and flood logs; remove those per-property PictureInPictureLogger.log calls and instead emit a single, structured debug log from applyCurrentPropertiesToController (or gate logging behind a debug flag) that summarizes the current property values and controller presence before assigning them to pictureInPictureController.packages/react-native-sdk/ios/PictureInPicture/PictureInPictureEnforcedStopAdapter.swift (2)
93-106: Consider adding a max-attempts guard or success check.The timer calls
stopPictureInPicture()at every display refresh frame until PiP becomes inactive. While the comment explains this covers cases where AVKit doesn't settle on the first attempt, repeatedly calling stop at 60+ Hz could be wasteful. Consider either:
- Checking if PiP is still active before calling stop
- Adding a maximum number of attempts before giving up
♻️ Optional: Add a guard before calling stop
let operation = Timer .publish(every: refreshRate, on: .main, in: .common) .autoconnect() .filter { _ in UIApplication.shared.applicationState == .active } .sink { [weak pictureInPictureController] _ in // Calling stop repeatedly at display cadence covers cases where // AVKit does not settle PiP shutdown on the first attempt. + guard pictureInPictureController?.isPictureInPictureActive == true else { + return + } pictureInPictureController?.stopPictureInPicture() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureEnforcedStopAdapter.swift` around lines 93 - 106, The Timer sink currently calls pictureInPictureController?.stopPictureInPicture() every frame; add a guard to avoid repeated unnecessary calls and limit attempts by checking PiP state and/or counting attempts: inside the sink closure (where pictureInPictureController is captured) first verify the controller is still active (e.g., isPictureInPictureActive or equivalent) before calling stopPictureInPicture(), and track a maxAttempts counter (store it alongside DisposableKey.stopEnforceOperation) to cancel the timer after N tries if PiP hasn't stopped; ensure the timer subscription is cancelled/stored via store(...) when either the stop succeeds or max attempts are reached.
147-158: Map.inactiveto.foregroundfor more consistent enforcement behavior.When the app is in the
.inactivestate (e.g., during Control Center overlay or notification center), it's still considered "user-facing." Mapping this to.unknownmeans PiP enforcement won't trigger until the app transitions to.active. If PiP should also stop when the app is merely inactive (not fully active), consider treating.inactiveas.foreground.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureEnforcedStopAdapter.swift` around lines 147 - 158, The currentApplicationState() switch maps UIApplication.State.inactive to ApplicationState.unknown; change it to return ApplicationState.foreground instead so that `.inactive` is treated as user-facing for PiP enforcement. Update the switch in the currentApplicationState() function to map .inactive -> .foreground (leaving .active -> .foreground, .background -> .background and the `@unknown` default -> .unknown) so enforcement triggers while the app is inactive.packages/react-native-sdk/ios/PictureInPicture/PictureInPictureReconnectionView.swift (1)
38-47: Hardcoded "Reconnecting" string should be localized.The message label text "Reconnecting" is hardcoded. Per coding guidelines, UI components should use the i18n system for localized content. Consider using
NSLocalizedStringor the project's localization mechanism.🌐 Suggested fix
private let messageLabel: UILabel = { let label = UILabel() label.translatesAutoresizingMaskIntoConstraints = false - label.text = "Reconnecting" + label.text = NSLocalizedString("Reconnecting", comment: "PiP reconnection indicator message") label.textColor = .white label.textAlignment = .center label.font = UIFont.systemFont(ofSize: 16, weight: .medium) label.accessibilityIdentifier = "reconnectingMessage" return label }()As per coding guidelines:
Import translation strings from src/translations/ and use i18n system to provide localized content for UI components🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureReconnectionView.swift` around lines 38 - 47, The label text in PictureInPictureReconnectionView's messageLabel is hardcoded as "Reconnecting"; replace it with a localized string lookup (e.g., use NSLocalizedString or the project's i18n helper) and load the key from the shared translations (import strings from src/translations as per project convention) so messageLabel.text is set to the localized value; keep the accessibilityIdentifier "reconnectingMessage" unchanged.packages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureVideoRenderer.swift (1)
535-544: Consider extracting duplicatedisSameTrackInstancehelper.This helper function is duplicated in three files:
PictureInPictureContent.swift(lines 88-97)StreamPictureInPictureVideoRenderer.swift(lines 535-544)RTCViewPip.swift(lines 268-277)Consider extracting to a shared utility file to maintain DRY principles.
♻️ Suggested approach
Create a shared utility, e.g., in
PictureInPictureUtilities.swift:/// Compares two RTCVideoTrack instances by reference identity. /// Track identity must be reference-based so reconnect-created tracks /// with reused `trackId` still propagate through content updates. func isSameTrackInstance(_ lhs: RTCVideoTrack?, _ rhs: RTCVideoTrack?) -> Bool { switch (lhs, rhs) { case (nil, nil): return true case let (lhsTrack?, rhsTrack?): return lhsTrack === rhsTrack default: return false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureVideoRenderer.swift` around lines 535 - 544, Extract the duplicated isSameTrackInstance(_:_:) helper into a shared utility (e.g., create PictureInPictureUtilities.swift) and replace the three copies in PictureInPictureContent, StreamPictureInPictureVideoRenderer, and RTCViewPip to call the single function; ensure the helper is declared with module-visible access (internal/public as needed) and include the brief doc comment about comparing RTCVideoTrack instances by reference identity so existing callers like isSameTrackInstance(...) keep working without behavior changes.packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipNative.tsx (1)
76-107: Consider using object spread for cleaner prop forwarding.The individual prop assignments with repeated eslint-disable comments add verbosity. Since all props are being forwarded, consider using object spread for cleaner code.
♻️ Suggested refactor
return ( <NativeComponent style={StyleSheet.absoluteFill} pointerEvents={'none'} - // eslint-disable-next-line react/prop-types - streamURL={props.streamURL} - // eslint-disable-next-line react/prop-types - mirror={props.mirror} - // eslint-disable-next-line react/prop-types - onPiPChange={props.onPiPChange} - // eslint-disable-next-line react/prop-types - participantName={props.participantName} - // eslint-disable-next-line react/prop-types - participantImageURL={props.participantImageURL} - // eslint-disable-next-line react/prop-types - isReconnecting={props.isReconnecting} - // eslint-disable-next-line react/prop-types - isScreenSharing={props.isScreenSharing} - // eslint-disable-next-line react/prop-types - hasAudio={props.hasAudio} - // eslint-disable-next-line react/prop-types - isTrackPaused={props.isTrackPaused} - // eslint-disable-next-line react/prop-types - isPinned={props.isPinned} - // eslint-disable-next-line react/prop-types - isSpeaking={props.isSpeaking} - // eslint-disable-next-line react/prop-types - connectionQuality={props.connectionQuality} + // eslint-disable-next-line react/prop-types + {...props} // `@ts-expect-error` - types issue ref={ref} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipNative.tsx` around lines 76 - 107, The prop forwarding block in RTCViewPipNative.tsx is verbose and full of eslint-disable comments; replace the explicit per-prop assignments with a single object spread of props into <NativeComponent {...props} /> while preserving and explicitly passing style={StyleSheet.absoluteFill}, pointerEvents={'none'}, and the ref (keep the existing // `@ts-expect-error` comment above ref if needed); remove the redundant eslint-disable-next-line react/prop-types lines so props like streamURL, mirror, onPiPChange, participantName, etc. are forwarded via the spread and ref remains attached.packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift (1)
107-111: Verbose logging inlayoutSubviewsmay impact performance.
layoutSubviewscan be called frequently. While the logging is gated by#if DEBUGinPictureInPictureLogger, consider whether this level of detail is necessary for production debugging or if it should be conditional/removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift` around lines 107 - 111, The layoutSubviews implementation currently calls PictureInPictureLogger.log on every layout, which can hurt performance; modify layoutSubviews to avoid frequent verbose logging by guarding the log call behind a build/config check or a runtime verbosity flag (e.g., only call PictureInPictureLogger.log when DEBUG or when a new isVerboseLoggingEnabled flag is true), leaving updateAvatarSize() unchanged so layout behavior is preserved; reference the layoutSubviews method and the PictureInPictureLogger.log call to locate and update the code.packages/react-native-sdk/ios/PictureInPicture/PictureInPictureDelegateProxy.swift (1)
83-88: EnsurerestoreUIcompletion handler is always called.The
restoreUIevent captures thecompletionHandlerclosure. If subscribers don't call this handler, the system may exhibit undefined behavior. Consider adding documentation noting that subscribers must call the completion handler, or provide a default behavior.📝 Documentation suggestion
+ /// Enum representing each AVPictureInPictureControllerDelegate method call + /// with its respective associated values. + /// + /// - Important: For the `restoreUI` case, subscribers **must** call the completion + /// handler with `true` to restore the user interface, or `false` to remain in PiP. + /// Failure to call the handler may cause undefined system behavior. enum Event: CustomStringConvertible {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureDelegateProxy.swift` around lines 83 - 88, The restoreUI completion handler captured and forwarded via eventSubject (.restoreUI) must always be invoked to avoid undefined behavior; modify pictureInPictureController(_:restoreUserInterfaceForPictureInPictureStopWithCompletionHandler:) so after sending eventSubject.send(.restoreUI(pictureInPictureController, completionHandler)) you also install a safe fallback that will call the completionHandler (e.g. completionHandler(false)) after a short timeout if no subscriber calls it, and/or update the .restoreUI event documentation to clearly state subscribers are required to call the provided completionHandler immediately; ensure the fallback uses a weak reference to avoid retain cycles and is canceled if the completionHandler is invoked by a subscriber.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift`:
- Around line 227-260: The loadImage function can apply a stale image if
imageURL changes between task start and completion; fix by capturing the request
URL/string (e.g., let requestURL = imageURL or let requestURLString) before
creating the URLSession task, and inside the completion (on the main thread)
verify that the current imageURL still equals that captured request value before
assigning imageView.image or changing visibility; also clear
currentImageLoadTask when the task completes or fails and handle cancellations
so only the most recent task can update the UI (refer to loadImage,
currentImageLoadTask, imageURL, imageView, initialsLabel, placeholderImageView).
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureContentState.swift`:
- Around line 20-39: The Snapshot struct inside PictureInPictureContentState
should not conform to Sendable while it contains an Objective-C reference type
RTCVideoTrack; either remove the Sendable conformance from Snapshot, or
remove/relocate the RTCVideoTrack from Snapshot and keep it confined to a single
concurrency domain (for example, make the track property stored separately under
`@MainActor` or only accessed via the existing serial
stateQueue/DispatchQueue.main), ensuring all references to Snapshot,
RTCVideoTrack, PictureInPictureContentState, and the stateQueue dispatching
logic are updated accordingly.
In
`@packages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureVideoRenderer.swift`:
- Around line 267-272: The availability check in
StreamPictureInPictureVideoRenderer uses an invalid iOS version "iOS 26.0";
change the availability condition to the correct target (likely "iOS 16.0" or
whichever minimum is intended) so the branch returns 32 for that valid OS and 16
otherwise; update the if `#available`(iOS 26.0, *) to if `#available`(iOS 16.0, *)
(or the correct supported version) within the
StreamPictureInPictureVideoRenderer implementation.
In `@packages/react-native-sdk/src/components/Call/CallContent/CallContent.tsx`:
- Around line 146-151: The effect that subscribes to
call.state.remoteParticipants$ leaves previous remoteParticipants when call
becomes falsy; update the useEffect handling in CallContent (the block
referencing call and setRemoteParticipants) so that if call is null/undefined
you reset remoteParticipants (call setRemoteParticipants([]) or appropriate
empty value) before returning; otherwise subscribe to remoteParticipants$, keep
the debounceTime(300) subscription and unsubscribe in the cleanup as before.
- Around line 138-151: Replace the manual RxJS subscription to
call.state.remoteParticipants$ in the CallContent component with the provided
hook from bindings: useRemoteParticipants() (obtained via useCallStateHooks()).
Remove the useEffect that subscribes to call.state.remoteParticipants$ and the
local state remoteParticipants/setRemoteParticipants, and instead read remote
participants from useRemoteParticipants(); if you need the debounce(300)
behavior, implement it in a custom wrapper inside the bindings layer (not in
this component) so the component uses the hook directly.
In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx`:
- Around line 183-184: participantIsPinned is computed incorrectly by checking
participantInSpotlight?.pin !== undefined; update it to use the same logic as
the isPinned helper in packages/client/src/helpers/participantUtils.ts (i.e.,
treat a participant as pinned only when !!p.pin && (p.pin.isLocalPin ||
p.pin.pinnedAt > 0)). Replace the current check for participantIsPinned (and any
similar checks around participantInSpotlight) with a boolean expression that
mirrors isPinned or call the shared helper function (isPinned) so pinned state
matches the client helper.
In
`@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx`:
- Around line 56-74: Initialize participant state synchronously before the
debounced subscriptions to avoid the temporary empty-array flash: when the
component mounts (inside the existing effect that references call)
setRemoteParticipants and setAllParticipants from the call's current participant
snapshots (e.g., call.state.remoteParticipants and call.state.participants or
the observables' current value accessors) if call is defined, then create the
debounced subscriptions (remoteParticipants$ and participants$) as you do now to
update afterwards; keep the same unsubscribe cleanup. Update the effect that
contains the useState setters so it first seeds state from the synchronous
snapshot and then attaches the debounceTime(300) subscriptions for continuous
updates.
In
`@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsx`:
- Around line 64-66: In the useEffect inside the CallParticipantsSpotlight
component, when call is falsy you must reset the local state (allParticipants)
to an empty array to avoid showing stale participants; update the early-return
branch in the effect that currently checks "if (!call) return" to clear
allParticipants (or call setAllParticipants([])) and then return, and ensure the
existing subscription logic that subscribes to call.state.participants$ (and
cleans up via sub.unsubscribe or similar) remains unchanged for the truthy-call
path.
- Around line 60-73: Replace the direct subscription to call.state.participants$
with the bindings hook: useParticipants({ sortBy: speakerLayoutSortPreset })
from useCallStateHooks()/useParticipants; remove the manual
useEffect/subscription and setAllParticipants state, and instead derive
participants from useParticipants; if the 300ms debounce is still required,
apply debouncing at the component layer (e.g. useMemo with a stable equality or
a local debounce hook) rather than subscribing to call.state.participants$, or
set the sort globally via call.setSortParticipantsBy(speakerLayoutSortPreset).
---
Outside diff comments:
In `@packages/react-native-sdk/ios/RTCViewPip.swift`:
- Around line 167-205: The view removes its NotificationCenter observer only in
didMoveToSuperview when superview == nil, which can miss cleanup if the view is
deallocated while still attached; add a deinit in the RTCViewPip class that
calls NotificationCenter.default.removeObserver(self) to guarantee the observer
is removed on deallocation, keeping the existing removal in didMoveToSuperview
for the common detach path and avoiding duplicate side effects.
---
Nitpick comments:
In
`@packages/react-native-sdk/__tests__/components/CallParticipantsGrid.test.tsx`:
- Around line 159-162: Replace hardcoded timing values in the test with named
constants to keep them in sync with the component's debounce settings: define
(or import) constants like PARTICIPANT_DEBOUNCE_MS and FORCE_UPDATE_DEBOUNCE_MS
and use them instead of the literal 300 and 500 in
CallParticipantsGrid.test.tsx; ensure the test uses the same source of truth as
the component debounce in forceUpdateValue (or the shared constants file) so
jest.advanceTimersByTime(...) and any debounceTime usages remain consistent.
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift`:
- Around line 107-111: The layoutSubviews implementation currently calls
PictureInPictureLogger.log on every layout, which can hurt performance; modify
layoutSubviews to avoid frequent verbose logging by guarding the log call behind
a build/config check or a runtime verbosity flag (e.g., only call
PictureInPictureLogger.log when DEBUG or when a new isVerboseLoggingEnabled flag
is true), leaving updateAvatarSize() unchanged so layout behavior is preserved;
reference the layoutSubviews method and the PictureInPictureLogger.log call to
locate and update the code.
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureDelegateProxy.swift`:
- Around line 83-88: The restoreUI completion handler captured and forwarded via
eventSubject (.restoreUI) must always be invoked to avoid undefined behavior;
modify
pictureInPictureController(_:restoreUserInterfaceForPictureInPictureStopWithCompletionHandler:)
so after sending eventSubject.send(.restoreUI(pictureInPictureController,
completionHandler)) you also install a safe fallback that will call the
completionHandler (e.g. completionHandler(false)) after a short timeout if no
subscriber calls it, and/or update the .restoreUI event documentation to clearly
state subscribers are required to call the provided completionHandler
immediately; ensure the fallback uses a weak reference to avoid retain cycles
and is canceled if the completionHandler is invoked by a subscriber.
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureEnforcedStopAdapter.swift`:
- Around line 93-106: The Timer sink currently calls
pictureInPictureController?.stopPictureInPicture() every frame; add a guard to
avoid repeated unnecessary calls and limit attempts by checking PiP state and/or
counting attempts: inside the sink closure (where pictureInPictureController is
captured) first verify the controller is still active (e.g.,
isPictureInPictureActive or equivalent) before calling stopPictureInPicture(),
and track a maxAttempts counter (store it alongside
DisposableKey.stopEnforceOperation) to cancel the timer after N tries if PiP
hasn't stopped; ensure the timer subscription is cancelled/stored via store(...)
when either the stop succeeds or max attempts are reached.
- Around line 147-158: The currentApplicationState() switch maps
UIApplication.State.inactive to ApplicationState.unknown; change it to return
ApplicationState.foreground instead so that `.inactive` is treated as
user-facing for PiP enforcement. Update the switch in the
currentApplicationState() function to map .inactive -> .foreground (leaving
.active -> .foreground, .background -> .background and the `@unknown` default ->
.unknown) so enforcement triggers while the app is inactive.
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureParticipantOverlayView.swift`:
- Around line 197-201: The nil/empty check in updateVisibility is verbose;
replace the hasName calculation with an optional-chaining check (e.g., treat
participantName?.isEmpty == false) so it reads clearly, then set isHidden =
!isOverlayEnabled || !hasName; locate the updateVisibility() method and update
the participantName check accordingly (symbols: updateVisibility,
participantName, isOverlayEnabled, isHidden).
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureReconnectionView.swift`:
- Around line 38-47: The label text in PictureInPictureReconnectionView's
messageLabel is hardcoded as "Reconnecting"; replace it with a localized string
lookup (e.g., use NSLocalizedString or the project's i18n helper) and load the
key from the shared translations (import strings from src/translations as per
project convention) so messageLabel.text is set to the localized value; keep the
accessibilityIdentifier "reconnectingMessage" unchanged.
In
`@packages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureVideoRenderer.swift`:
- Around line 535-544: Extract the duplicated isSameTrackInstance(_:_:) helper
into a shared utility (e.g., create PictureInPictureUtilities.swift) and replace
the three copies in PictureInPictureContent,
StreamPictureInPictureVideoRenderer, and RTCViewPip to call the single function;
ensure the helper is declared with module-visible access (internal/public as
needed) and include the brief doc comment about comparing RTCVideoTrack
instances by reference identity so existing callers like
isSameTrackInstance(...) keep working without behavior changes.
In `@packages/react-native-sdk/ios/RTCViewPip.swift`:
- Around line 22-90: The property setters on RTCViewPip (participantName,
participantImageURL, isReconnecting, isScreenSharing, hasAudio, isTrackPaused,
isPinned, isSpeaking, connectionQuality) currently call
PictureInPictureLogger.log in each didSet and flood logs; remove those
per-property PictureInPictureLogger.log calls and instead emit a single,
structured debug log from applyCurrentPropertiesToController (or gate logging
behind a debug flag) that summarizes the current property values and controller
presence before assigning them to pictureInPictureController.
In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx`:
- Around line 162-167: The reconnection check in RTCViewPipIOS.tsx (the
isReconnecting boolean) currently covers CallingState.MIGRATING, RECONNECTING,
RECONNECTING_FAILED, and OFFLINE but omits JOINING; update the isReconnecting
expression to include CallingState.JOINING so that the UI treats JOINING as a
reconnection state (modify the isReconnecting declaration in RTCViewPipIOS.tsx
to OR in CallingState.JOINING).
- Around line 57-66: Add a brief explanatory comment above the useEffect that
subscribes to call.state.participants$ (the block using debounceTime(300) and
setting setAllParticipants) stating that this direct RxJS subscription
intentionally deviates from the guideline to use useCall()/useCallStateHooks()
because we need a pared-down debounced pipeline for performance and therefore
replaced the previous useDebouncedValue approach; mention the tradeoff and link
to the function names (useEffect, call.state.participants$, debounceTime,
setAllParticipants) so future maintainers understand this deliberate exception.
In
`@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipNative.tsx`:
- Around line 76-107: The prop forwarding block in RTCViewPipNative.tsx is
verbose and full of eslint-disable comments; replace the explicit per-prop
assignments with a single object spread of props into <NativeComponent
{...props} /> while preserving and explicitly passing
style={StyleSheet.absoluteFill}, pointerEvents={'none'}, and the ref (keep the
existing // `@ts-expect-error` comment above ref if needed); remove the redundant
eslint-disable-next-line react/prop-types lines so props like streamURL, mirror,
onPiPChange, participantName, etc. are forwarded via the spread and ref remains
attached.
In
`@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx`:
- Around line 52-55: The component CallParticipantsGrid inconsistently mixes
useLocalParticipant/useDominantSpeaker hooks with a direct RxJS subscription for
remote/all participants; refactor to use the same hook-based pattern or extract
a reusable hook that encapsulates the debounced subscription logic (e.g., create
useParticipants or useDebouncedParticipants) and replace the direct subscription
with that hook so localParticipant, dominantSpeaker and the remote/all
participant list are all sourced consistently via hooks inside
CallParticipantsGrid.
- Around line 62-74: Replace the direct RxJS subscriptions inside the useEffect
(which subscribe to call.state.remoteParticipants$ and call.state.participants$
and call setRemoteParticipants/setAllParticipants) with the React bindings:
import and use useCallStateHooks() (useRemoteParticipants and useParticipants)
to get participant lists, or encapsulate the debouncing into a new hook (e.g.,
useDebouncedParticipants) that internally uses
useRemoteParticipants/useParticipants and returns debounced values; update
CallParticipantsGrid to consume those hooks instead of subscribing to
call.state.* observables, or if you must keep the exception, add a minimal
eslint-disable with a short rationale and move the debouncing into a dedicated
hook named useDebouncedParticipants referenced from the component.
In
`@packages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsx`:
- Around line 108-117: The debounced force-update can lose an early emission
because forceUpdate$ is a plain Subject; change forceUpdate$ to a replaying
subject so an emission before the effect subscribes is retained (e.g., replace
new Subject<void>() with a ReplaySubject<void>(1) and ensure rxjs ReplaySubject
is imported), leaving the existing useEffect subscription, debounceTime,
setForceUpdateValue and forceUpdate() (useCallback) logic intact so mount-time
forceUpdate() calls are not dropped.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
sample-apps/react-native/dogfood/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
packages/react-native-sdk/__tests__/components/CallParticipantsGrid.test.tsxpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureConnectionQualityIndicator.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureContent.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureContentState.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureDelegateProxy.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureEnforcedStopAdapter.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureLogger.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureParticipantOverlayView.swiftpackages/react-native-sdk/ios/PictureInPicture/PictureInPictureReconnectionView.swiftpackages/react-native-sdk/ios/PictureInPicture/StreamAVPictureInPictureVideoCallViewController.swiftpackages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureController.swiftpackages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureControllerProtocol.swiftpackages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureVideoRenderer.swiftpackages/react-native-sdk/ios/RTCViewPip.swiftpackages/react-native-sdk/ios/RTCViewPipManager.mmpackages/react-native-sdk/ios/RTCViewPipManager.swiftpackages/react-native-sdk/src/components/Call/CallContent/CallContent.tsxpackages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsxpackages/react-native-sdk/src/components/Call/CallContent/RTCViewPipNative.tsxpackages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsxpackages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsxpackages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsxpackages/react-native-sdk/src/utils/hooks/index.tspackages/react-native-sdk/src/utils/hooks/useDebouncedValue.tssample-apps/react-native/dogfood/ios/AppDelegate.swift
💤 Files with no reviewable changes (2)
- packages/react-native-sdk/src/utils/hooks/index.ts
- packages/react-native-sdk/src/utils/hooks/useDebouncedValue.ts
packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift
Show resolved
Hide resolved
packages/react-native-sdk/ios/PictureInPicture/PictureInPictureContentState.swift
Outdated
Show resolved
Hide resolved
packages/react-native-sdk/ios/PictureInPicture/StreamPictureInPictureVideoRenderer.swift
Show resolved
Hide resolved
packages/react-native-sdk/src/components/Call/CallContent/CallContent.tsx
Show resolved
Hide resolved
packages/react-native-sdk/src/components/Call/CallContent/CallContent.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx
Show resolved
Hide resolved
packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsx
Show resolved
Hide resolved
packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsx
Show resolved
Hide resolved
oliverlaz
reviewed
Feb 26, 2026
packages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx (1)
62-78: Consider syncing state whencallbecomes available after mount.The state initializers only run once. If
callis initially undefined and later becomes defined, the effect subscribes to debounced observables but doesn't immediately sync the current values—leaving a 300ms window with stale (empty) data.Additionally, direct observable access (
call.state.remoteParticipants$) deviates from the guideline preferring hooks viauseCallStateHooks(). This appears intentional for the debouncing requirement; if this pattern is needed elsewhere, consider extracting it into a reusable hook (e.g.,useDebouncedParticipants).Proposed fix to sync state when call becomes available
useEffect(() => { if (!call) { setRemoteParticipants([]); setAllParticipants([]); return; } + // Sync current values immediately when call becomes available + setRemoteParticipants(call.state.remoteParticipants); + setAllParticipants(call.state.participants); + const sub1 = call.state.remoteParticipants$ .pipe(debounceTime(300)) .subscribe(setRemoteParticipants); const sub2 = call.state.participants$ .pipe(debounceTime(300)) .subscribe(setAllParticipants); return () => { sub1.unsubscribe(); sub2.unsubscribe(); }; }, [call]);As per coding guidelines: "Use React hooks from
@stream-io/video-react-bindings(viauseCall()anduseCallStateHooks()) instead of directly accessing RxJS observables from@stream-io/video-client"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx` around lines 62 - 78, The effect in CallParticipantsGrid subscribes to debounced observables but doesn't sync current participant values when call transitions from undefined to defined, leaving state empty for up to 300ms; update the effect (or extract a useDebouncedParticipants hook) to, when call becomes truthy, immediately read the current participant values (from call.state.remoteParticipants$/.participants$ or via useCallStateHooks()) and call setRemoteParticipants/setAllParticipants before creating the debounced subscriptions, and prefer using useCall() + useCallStateHooks() instead of directly accessing call.state.* where possible so initial state is in sync and debouncing only affects subsequent updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsx`:
- Around line 66-75: When the call instance changes in
CallParticipantsSpotlight's useEffect, immediately seed the UI with the new
call's current participants snapshot before subscribing to
call.state.participants$ so the UI doesn't show stale participants during the
300ms debounce; do this by calling
setAllParticipants([...call.state.participants].sort(speakerLayoutSortPreset))
(or equivalent) right after detecting a non-null call, then subscribe to
participants$ with debounceTime(300) as before, and guard updates by checking
the call instance identity (or a timestamp/instanceId on the call) in the
subscription handler to avoid races from stale emissions.
---
Nitpick comments:
In
`@packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsx`:
- Around line 62-78: The effect in CallParticipantsGrid subscribes to debounced
observables but doesn't sync current participant values when call transitions
from undefined to defined, leaving state empty for up to 300ms; update the
effect (or extract a useDebouncedParticipants hook) to, when call becomes
truthy, immediately read the current participant values (from
call.state.remoteParticipants$/.participants$ or via useCallStateHooks()) and
call setRemoteParticipants/setAllParticipants before creating the debounced
subscriptions, and prefer using useCall() + useCallStateHooks() instead of
directly accessing call.state.* where possible so initial state is in sync and
debouncing only affects subsequent updates.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/react-native-sdk/src/components/Call/CallContent/CallContent.tsxpackages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsGrid.tsxpackages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsx
packages/react-native-sdk/src/components/Call/CallLayout/CallParticipantsSpotlight.tsx
Outdated
Show resolved
Hide resolved
oliverlaz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureContentState.swift`:
- Around line 77-86: The priority check in PictureInPictureContentState that
sets newContent must be reordered so screen sharing wins over avatar when video
is disabled: change the conditional order in the block that examines snapshot
(used in the logic that assigns newContent) to check snapshot.isReconnecting
first, then snapshot.isScreenSharing, then !snapshot.isVideoEnabled (avatar with
participantName/participantImageURL), then snapshot.isVideoEnabled and lastly
avatar fallback; also update the nearby comment that currently reads "Priority
order: reconnecting > avatar (video disabled) > screen sharing > video > avatar
fallback" to the corrected "reconnecting > screen sharing > avatar (video
disabled) > video > avatar fallback".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/react-native-sdk/ios/PictureInPicture/PictureInPictureContentState.swift
packages/react-native-sdk/ios/PictureInPicture/PictureInPictureContentState.swift
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx (2)
58-67: Direct RxJS subscription is acceptable here, but consider adding a brief comment explaining the pattern deviation.The coding guidelines recommend using React hooks from
@stream-io/video-react-bindingsinstead of directly accessing RxJS observables. This direct subscription toparticipants$with customdebounceTimeand sorting is a valid exception sinceuseParticipants()doesn't support inline debouncing.The implementation correctly handles cleanup via
subscription.unsubscribe(). Consider adding a short comment noting this deviates from the typical hook pattern due to the custom operator pipeline.📝 Optional comment suggestion
// we debounce the participants to avoid unnecessary rerenders // that happen when participant tracks are all subscribed simultaneously + // Note: Direct subscription used here instead of useParticipants() hook + // to enable custom debouncing with RxJS operators. useEffect(() => {As per coding guidelines: "Use React hooks from
@stream-io/video-react-bindings(viauseCall()anduseCallStateHooks()) instead of directly accessing RxJS observables from@stream-io/video-client"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx` around lines 58 - 67, Add a short explanatory comment above the useEffect that subscribes to call.state.participants$ describing why this RxJS subscription is used instead of the usual hooks (e.g., mention that useParticipants() doesn't support inline debounce/sorting), reference the operators used (debounceTime, map) and the handler setAllParticipants so future maintainers understand the intentional deviation and that cleanup via subscription.unsubscribe() is performed; place the comment immediately above the useEffect block that contains call.state.participants$, debounceTime(300), map(...sort(speakerLayoutSortPreset)), and subscribe(setAllParticipants).
170-173: Consider whether defaultinghasAudiototruewhen no participant exists is the intended behavior.When
participantInSpotlightis undefined,participantHasAudiodefaults totrue. This might cause the PiP UI to show an "audio enabled" indicator even when there's no participant to display. If the intent is to avoid showing a muted indicator incorrectly, this is fine—but if the native UI uses this to display audio status,falseor a separate "unknown" state might be more accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx` around lines 170 - 173, The current expression setting participantHasAudio (in RTCViewPipIOS.tsx) defaults to true when participantInSpotlight is undefined, which can misrepresent audio state; update the logic in the participantHasAudio assignment (where hasAudio(participantInSpotlight) is used) to default to false (or a distinct "unknown" flag if the UI supports it) when participantInSpotlight is undefined, and propagate that change to any UI rendering code that reads participantHasAudio so the PiP audio indicator reflects the new default/unknown state instead of always true.packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift (1)
153-179: Consider extracting magic numbers to named constants.The layout logic is correct. Calling
layoutIfNeeded()after activating constraints ensuresavatarContainerView.boundsis updated before setting the corner radius.The magic numbers (0.4 for avatar ratio, 60 for minimum size) could be extracted to named constants for better maintainability, though this is optional.
💡 Optional: Extract constants
+ private static let avatarSizeRatio: CGFloat = 0.4 + private static let minimumAvatarSize: CGFloat = 60 + private func updateAvatarSize() { NSLayoutConstraint.deactivate(avatarSizeConstraints) let minDimension = min(bounds.width, bounds.height) - let avatarSize = max(minDimension * 0.4, 60) + let avatarSize = max(minDimension * Self.avatarSizeRatio, Self.minimumAvatarSize)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift` around lines 153 - 179, In updateAvatarSize(), extract the magic numbers into named constants (e.g., let avatarRatio: CGFloat = 0.4 and let minAvatarSize: CGFloat = 60) and replace the inline literals (minDimension * 0.4 and 60) with those constants; update any related log messages to reference the constants so intent is clear and maintainability is improved while keeping the existing constraint creation, activation, layoutIfNeeded(), and cornerRadius logic in avatarContainerView unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swift`:
- Around line 153-179: In updateAvatarSize(), extract the magic numbers into
named constants (e.g., let avatarRatio: CGFloat = 0.4 and let minAvatarSize:
CGFloat = 60) and replace the inline literals (minDimension * 0.4 and 60) with
those constants; update any related log messages to reference the constants so
intent is clear and maintainability is improved while keeping the existing
constraint creation, activation, layoutIfNeeded(), and cornerRadius logic in
avatarContainerView unchanged.
In `@packages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsx`:
- Around line 58-67: Add a short explanatory comment above the useEffect that
subscribes to call.state.participants$ describing why this RxJS subscription is
used instead of the usual hooks (e.g., mention that useParticipants() doesn't
support inline debounce/sorting), reference the operators used (debounceTime,
map) and the handler setAllParticipants so future maintainers understand the
intentional deviation and that cleanup via subscription.unsubscribe() is
performed; place the comment immediately above the useEffect block that contains
call.state.participants$, debounceTime(300),
map(...sort(speakerLayoutSortPreset)), and subscribe(setAllParticipants).
- Around line 170-173: The current expression setting participantHasAudio (in
RTCViewPipIOS.tsx) defaults to true when participantInSpotlight is undefined,
which can misrepresent audio state; update the logic in the participantHasAudio
assignment (where hasAudio(participantInSpotlight) is used) to default to false
(or a distinct "unknown" flag if the UI supports it) when participantInSpotlight
is undefined, and propagate that change to any UI rendering code that reads
participantHasAudio so the PiP audio indicator reflects the new default/unknown
state instead of always true.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/react-native-sdk/ios/PictureInPicture/PictureInPictureAvatarView.swiftpackages/react-native-sdk/src/components/Call/CallContent/RTCViewPipIOS.tsxpackages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-native-sdk/src/components/Call/CallParticipantsList/CallParticipantsList.tsx
oliverlaz
approved these changes
Feb 26, 2026
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
2 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.
iOS.PiP.mp4
🎫 Ticket: https://linear.app/stream/issue/RN-362
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores