Open
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
WalkthroughAdds a New Media Call feature: peer autocomplete service and Zustand store, UI components (FilterHeader, PeerList, PeerItem, SelectedPeer, CreateCall, Container), a useNewMediaCall hook and useSubscription utility, integrates ActionSheet-driven call flow across views, plus Storybook stories and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Hook as useNewMediaCall
participant Store as PeerAutocompleteStore
participant API as getPeerAutocompleteOptions
participant ActionSheet
participant Media as MediaSessionInstance
User->>UI: press call control
UI->>Hook: openNewMediaCall(rid)
Hook->>Hook: check hasMediaCallPermission
alt permission granted
Hook->>Store: setSelectedPeer(if direct DM)
Hook->>ActionSheet: showActionSheet(<NewMediaCall/>)
ActionSheet->>UI: render NewMediaCall
User->>UI: enter search term
UI->>API: getPeerAutocompleteOptions(filter, peerInfo)
API-->>Store: return [sip option, user options]
Store-->>UI: options updated
User->>UI: select peer
UI->>Store: setSelectedPeer(selected)
User->>UI: press Call
UI->>Media: startCall(peerId, type)
Media-->>ActionSheet: hideActionSheet()
else permission denied
Hook-->>UI: no ActionSheet opened
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
force-pushed
the
feat.pre-flight
branch
from
e0950b9 to
5de5f50
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
![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: 7
🧹 Nitpick comments (8)
app/containers/NewMediaCall/PeerList.tsx (1)
37-46: Extract inline style to StyleSheet for performance.The inline style object
{ flexShrink: 1 }creates a new reference on every render, causing unnecessary re-renders ofFlatList.♻️ Suggested fix
+import { FlatList, StyleSheet } from 'react-native'; -import { FlatList } from 'react-native'; + +const styles = StyleSheet.create({ + contentContainer: { + flexShrink: 1 + } +}); return ( <FlatList data={options} - contentContainerStyle={{ flexShrink: 1 }} + contentContainerStyle={styles.contentContainer} keyExtractor={item => item.value}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/PeerList.tsx` around lines 37 - 46, The inline contentContainerStyle on the FlatList (contentContainerStyle={{ flexShrink: 1 }}) creates a new object each render; move this into a static StyleSheet entry (e.g., const styles = StyleSheet.create({ contentContainer: { flexShrink: 1 } })) and replace the inline use with contentContainerStyle={styles.contentContainer} in the PeerList component where FlatList is rendered (keep keyExtractor, renderItem, ItemSeparatorComponent unchanged).app/containers/NewMediaCall/CreateCall.stories.tsx (1)
38-46: Avoid side effects during render in story components.Similar to
PeerList.stories.tsx, callingsetStoreStatedirectly in the story's render body causes state mutation on every render. Consider using a decorator pattern or wrapper component withuseEffectto initialize store state.♻️ Suggested fix using a wrapper
+import { useEffect } from 'react'; + +const StoreInitWrapper = ({ + selectedPeer, + children +}: { + selectedPeer: TPeerItem | null; + children: React.ReactNode +}) => { + useEffect(() => { + usePeerAutocompleteStore.setState({ selectedPeer }); + }, [selectedPeer]); + return <>{children}</>; +}; + export const Disabled = () => { - setStoreState(null); - return <CreateCall />; + return ( + <StoreInitWrapper selectedPeer={null}> + <CreateCall /> + </StoreInitWrapper> + ); }; export const Enabled = () => { - setStoreState(userPeer); - return <CreateCall />; + return ( + <StoreInitWrapper selectedPeer={userPeer}> + <CreateCall /> + </StoreInitWrapper> + ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/CreateCall.stories.tsx` around lines 38 - 46, The story functions Disabled and Enabled mutate global state during render by calling setStoreState directly; change them to initialize state inside a wrapper component or a story decorator that uses useEffect (or a setup function) to call setStoreState once on mount before rendering <CreateCall />. Locate the Disabled and Enabled exports and replace the direct setStoreState calls with a wrapper component or decorator that invokes setStoreState(userPeer) or setStoreState(null) inside useEffect so the store is only mutated on mount, not on every render.app/containers/NewMediaCall/PeerList.stories.tsx (1)
53-59: Avoid side effects during render in story components.Calling
setStoreStatedirectly in the story's render body causes state mutation on every render. This can lead to unexpected behavior and violates React's render purity expectations, especially with React 19's stricter concurrent rendering.♻️ Suggested fix using a decorator or wrapper
+import { useEffect } from 'react'; + +const StoreInitWrapper = ({ options, children }: { options: TPeerItem[]; children: React.ReactNode }) => { + useEffect(() => { + usePeerAutocompleteStore.setState({ + options, + selectedPeer: null + }); + }, [options]); + return <>{children}</>; +}; + export const All = () => { - setStoreState(mixedOptions); return ( - <View style={styles.container}> - <PeerList /> - </View> + <StoreInitWrapper options={mixedOptions}> + <View style={styles.container}> + <PeerList /> + </View> + </StoreInitWrapper> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/PeerList.stories.tsx` around lines 53 - 59, The story function All currently mutates state during render by calling setStoreState(mixedOptions); move that side effect out of the render path — either add a story-level decorator or wrap All in a component that applies setStoreState inside a useEffect (or a beforeRender/setup function provided by your story framework) so the mutation runs once on mount rather than on every render; update the story to render <PeerList /> (with styles.container) only and apply mixedOptions via the decorator/wrapper instead of calling setStoreState directly inside All.app/containers/NewMediaCall/NewMediaCall.stories.tsx (1)
99-133: Same side effect issue: store setup runs during render.Similar to
Container.stories.tsx,setStoreByVariantis called inside the story function body, executing on every re-render. Move the setup to decorators for consistency.♻️ Suggested approach
+const withStoreVariant = (variant: 'empty' | 'searching' | 'userSelected' | 'sipSelected') => + (Story: React.ComponentType) => { + setStoreByVariant(variant); + return <Story />; + }; + export const Empty = () => { - setStoreByVariant('empty'); return ( <View style={styles.container}> <NewMediaCall /> </View> ); }; +Empty.decorators = [withStoreVariant('empty')];Apply the same pattern to
Searching,UserSelected, andSipSelected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/NewMediaCall.stories.tsx` around lines 99 - 133, The stories call setStoreByVariant inside each story function (Empty, Searching, UserSelected, SipSelected), causing side effects on every render; move these setup calls into the Storybook decorators (following the pattern used in Container.stories.tsx) so the store is configured once per story render context. Specifically, remove setStoreByVariant from the bodies of Empty, Searching, UserSelected and SipSelected and instead add a decorator that invokes setStoreByVariant('empty'|'searching'|'userSelected'|'sipSelected') based on the story name or metadata so each story initializes the store only once before rendering NewMediaCall.app/views/RoomInfoView/components/BaseButton.tsx (2)
27-36: ClarifyshowIconbehavior or rename the prop.When
showIconisfalse, the entire component returnsnull, not just hiding the icon. This makes the prop name misleading—it controls whether the button renders at all, not just icon visibility. Consider renaming tovisibleorshow, or restructuring if the intent is to render a text-only button variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomInfoView/components/BaseButton.tsx` around lines 27 - 36, The prop showIcon on the BaseButton component is misleading because when false the whole component returns null; update the component so its behavior matches intent: either (A) rename the prop to visible or show across the component and all call sites (and keep current render logic) to indicate it controls full visibility, or (B) keep the name showIcon but change the render logic in BaseButton so the BorderlessButton always renders and only the CustomIcon is conditionally rendered based on showIcon (leave props like iconName, onPress, enabled, styles.roomButton, and styles.roomButtonText intact); pick one approach and update usages of BaseButton accordingly.
19-19: SimplifyonPresstype signature.The
onPressprop is typed as(prop: any) => void, butBorderlessButton'sonPresscallback doesn't pass arguments. Use() => voidfor accuracy and to avoid confusion.♻️ Suggested fix
- onPress?: (prop: any) => void; + onPress?: () => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/RoomInfoView/components/BaseButton.tsx` at line 19, The onPress prop in BaseButton is incorrectly typed as (prop: any) => void even though BorderlessButton (the underlying component) calls its onPress without arguments; update the onPress type on the BaseButton props interface to a no-arg callback () => void (and ensure any callers match this signature) so the prop signature accurately reflects BorderlessButton's behavior.app/containers/NewMediaCall/Container.stories.tsx (1)
59-75: Side effect during render: move store setup out of render body.Calling
setStoreStatedirectly inside the story function body executes on every re-render, which can cause flicker or unexpected behavior in Storybook. Consider using a decorator oruseEffectto set up store state before rendering.♻️ Suggested approach using decorators
+const withStoreState = (filter: string) => (Story: React.ComponentType) => { + setStoreState(filter); + return <Story />; +}; + export const Default = () => { - setStoreState(''); return ( <Container> <PlaceholderChildren /> </Container> ); }; +Default.decorators = [withStoreState('')]; export const WithFilter = () => { - setStoreState('alice'); return ( <Container> <PlaceholderChildren /> </Container> ); }; +WithFilter.decorators = [withStoreState('alice')];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/Container.stories.tsx` around lines 59 - 75, The stories call setStoreState directly during render (in Default and WithFilter), causing side effects on every re-render; move the store setup out of the render by either wrapping the story in a decorator that calls setStoreState before rendering or by replacing the story function with a small React wrapper that calls setStoreState inside useEffect (e.g., create a SetupState component used by Default and WithFilter which calls setStoreState('') or setStoreState('alice') in useEffect and then returns <Container><PlaceholderChildren/></Container>), ensuring Container and PlaceholderChildren remain untouched.app/containers/NewMediaCall/SelectedPeer.stories.tsx (1)
62-70: Consider using Storybook decorators for store setup.Calling
setStoreStatedirectly during render can cause side effects on re-renders and may behave unexpectedly with React Strict Mode. Using a decorator ensures state is set up before render and cleaned up properly between stories.♻️ Suggested approach using decorators
// Add a decorator to handle store setup const withStoreState = (peer: TPeerItem | null) => (Story: React.FC) => { React.useEffect(() => { usePeerAutocompleteStore.setState({ selectedPeer: peer }); return () => usePeerAutocompleteStore.setState({ selectedPeer: null }); }, []); return <Story />; }; export const User = () => ( <View style={styles.container}> <Text style={styles.sectionLabel}>User variant</Text> <SelectedPeerInner selectedPeer={userPeer} /> </View> ); User.decorators = [withStoreState(userPeer)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/SelectedPeer.stories.tsx` around lines 62 - 70, The story calls setStoreState(userPeer) directly inside the User render which causes render-side effects; replace this by creating a Storybook decorator that sets the store before render and cleans it up after: add a withStoreState decorator function that uses useEffect to call the store setter (e.g., usePeerAutocompleteStore.setState or the existing setStoreState helper) to set selectedPeer to userPeer on mount and reset to null on unmount, then remove the inline setStoreState call from the User component and attach User.decorators = [withStoreState(userPeer)]; ensure SelectedPeerInner remains unchanged and the decorator references the same userPeer symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/containers/NewMediaCall/CreateCall.test.tsx`:
- Around line 116-128: The test in CreateCall.test.tsx asserts the wrong call
type for a SIP peer; update the expectation so mockStartCall is expected to be
called with '+5511999999999' and 'sip' instead of 'user' (i.e., change the
expect(mockStartCall).toHaveBeenCalledWith call to use 'sip'); verify the
assertion references the same mockStartCall used by the CreateCall component and
adjust any related assertions (e.g., other tests that assume 'user') if needed.
In `@app/containers/NewMediaCall/CreateCall.tsx`:
- Around line 22-26: The type-discrimination is wrong: replace the "'number' in
selectedPeer" check with an explicit discriminator on the peer type (e.g. check
selectedPeer.type === 'sip') so that CreateCall's call to
mediaSessionInstance.startCall uses 'sip' for SIP peers and 'user' otherwise;
update the branch in the component where
mediaSessionInstance.startCall(selectedPeer.value, ...) is called (the code
surrounding selectedPeer) to use the type field as the discriminator and add a
safe fallback if selectedPeer.type is undefined.
In `@app/lib/hooks/useSubscription.ts`:
- Around line 10-19: The effect can leave the previous subscription until an
async load completes and a slower request may overwrite a newer rid; to fix,
clear the subscription when rid changes and guard against stale responses by
giving each load a token (or AbortController) and only calling
setSubscription(result) if the token matches the current request for the active
rid; update the useEffect that defines load (and calls getSubscriptionByRoomId)
to generate a unique requestId/AbortController at start, call
setSubscription(null) when starting a new load (or when rid is falsy), and skip
setting state when the requestId/abort shows the response is stale—use the
existing symbols useEffect, load, getSubscriptionByRoomId, setSubscription, and
rid to locate and update the code.
In `@app/lib/services/voip/getPeerAutocompleteOptions.ts`:
- Around line 87-93: The helper currently always prepends a SIP option
(sipOption of type TPeerItem) for non-empty terms; modify
getPeerAutocompleteOptions to accept an allowExternalVoiceCalls (boolean)
parameter and only include/return the sipOption when allowExternalVoiceCalls is
true (otherwise return just userOptions), or alternatively ensure callers
(PeerList/NewMediaCall) filter out items with type === 'sip' when
allow-external-voice-calls is false; update function signature and call sites
accordingly so permission-driven filtering is enforced inside
getPeerAutocompleteOptions or immediately after it.
In `@app/lib/services/voip/usePeerAutocompleteStore.test.ts`:
- Around line 138-151: The test and implementation use console.log for error
handling; change the implementation in usePeerAutocompleteStore (the
fetchOptions error handler) to call console.error('Failed to fetch options')
instead of console.log, and update the test usePeerAutocompleteStore.test to spy
on console.error (jest.spyOn(console, 'error').mockImplementation()) and assert
expect(consoleSpy).toHaveBeenCalledWith('Failed to fetch options'), then restore
the spy; keep the mocked rejection
(mockGetPeerAutocompleteOptions.mockRejectedValue) and the assertion that
usePeerAutocompleteStore.getState().options equals [] unchanged.
In `@app/views/NewMessageView/Item.tsx`:
- Around line 28-34: The handleCallPress function is setting the selected peer
without the optional username, so update the call to
usePeerAutocompleteStore.getState().setSelectedPeer inside handleCallPress to
include the username prop (from the Item component's TPeerItem input) alongside
type, value, and label; mirror the pattern used in PeerList where the full peer
object (including username) is passed so PeerItemInner/Avatar can receive and
display username correctly when opening the NewMediaCall action sheet via
showActionSheetRef.
---
Nitpick comments:
In `@app/containers/NewMediaCall/Container.stories.tsx`:
- Around line 59-75: The stories call setStoreState directly during render (in
Default and WithFilter), causing side effects on every re-render; move the store
setup out of the render by either wrapping the story in a decorator that calls
setStoreState before rendering or by replacing the story function with a small
React wrapper that calls setStoreState inside useEffect (e.g., create a
SetupState component used by Default and WithFilter which calls
setStoreState('') or setStoreState('alice') in useEffect and then returns
<Container><PlaceholderChildren/></Container>), ensuring Container and
PlaceholderChildren remain untouched.
In `@app/containers/NewMediaCall/CreateCall.stories.tsx`:
- Around line 38-46: The story functions Disabled and Enabled mutate global
state during render by calling setStoreState directly; change them to initialize
state inside a wrapper component or a story decorator that uses useEffect (or a
setup function) to call setStoreState once on mount before rendering <CreateCall
/>. Locate the Disabled and Enabled exports and replace the direct setStoreState
calls with a wrapper component or decorator that invokes setStoreState(userPeer)
or setStoreState(null) inside useEffect so the store is only mutated on mount,
not on every render.
In `@app/containers/NewMediaCall/NewMediaCall.stories.tsx`:
- Around line 99-133: The stories call setStoreByVariant inside each story
function (Empty, Searching, UserSelected, SipSelected), causing side effects on
every render; move these setup calls into the Storybook decorators (following
the pattern used in Container.stories.tsx) so the store is configured once per
story render context. Specifically, remove setStoreByVariant from the bodies of
Empty, Searching, UserSelected and SipSelected and instead add a decorator that
invokes setStoreByVariant('empty'|'searching'|'userSelected'|'sipSelected')
based on the story name or metadata so each story initializes the store only
once before rendering NewMediaCall.
In `@app/containers/NewMediaCall/PeerList.stories.tsx`:
- Around line 53-59: The story function All currently mutates state during
render by calling setStoreState(mixedOptions); move that side effect out of the
render path — either add a story-level decorator or wrap All in a component that
applies setStoreState inside a useEffect (or a beforeRender/setup function
provided by your story framework) so the mutation runs once on mount rather than
on every render; update the story to render <PeerList /> (with styles.container)
only and apply mixedOptions via the decorator/wrapper instead of calling
setStoreState directly inside All.
In `@app/containers/NewMediaCall/PeerList.tsx`:
- Around line 37-46: The inline contentContainerStyle on the FlatList
(contentContainerStyle={{ flexShrink: 1 }}) creates a new object each render;
move this into a static StyleSheet entry (e.g., const styles =
StyleSheet.create({ contentContainer: { flexShrink: 1 } })) and replace the
inline use with contentContainerStyle={styles.contentContainer} in the PeerList
component where FlatList is rendered (keep keyExtractor, renderItem,
ItemSeparatorComponent unchanged).
In `@app/containers/NewMediaCall/SelectedPeer.stories.tsx`:
- Around line 62-70: The story calls setStoreState(userPeer) directly inside the
User render which causes render-side effects; replace this by creating a
Storybook decorator that sets the store before render and cleans it up after:
add a withStoreState decorator function that uses useEffect to call the store
setter (e.g., usePeerAutocompleteStore.setState or the existing setStoreState
helper) to set selectedPeer to userPeer on mount and reset to null on unmount,
then remove the inline setStoreState call from the User component and attach
User.decorators = [withStoreState(userPeer)]; ensure SelectedPeerInner remains
unchanged and the decorator references the same userPeer symbol.
In `@app/views/RoomInfoView/components/BaseButton.tsx`:
- Around line 27-36: The prop showIcon on the BaseButton component is misleading
because when false the whole component returns null; update the component so its
behavior matches intent: either (A) rename the prop to visible or show across
the component and all call sites (and keep current render logic) to indicate it
controls full visibility, or (B) keep the name showIcon but change the render
logic in BaseButton so the BorderlessButton always renders and only the
CustomIcon is conditionally rendered based on showIcon (leave props like
iconName, onPress, enabled, styles.roomButton, and styles.roomButtonText
intact); pick one approach and update usages of BaseButton accordingly.
- Line 19: The onPress prop in BaseButton is incorrectly typed as (prop: any) =>
void even though BorderlessButton (the underlying component) calls its onPress
without arguments; update the onPress type on the BaseButton props interface to
a no-arg callback () => void (and ensure any callers match this signature) so
the prop signature accurately reflects BorderlessButton's behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f27baa9e-8b14-417a-b622-97a64f11ee3f
⛔ Files ignored due to path filters (16)
app/containers/Avatar/__snapshots__/Avatar.test.tsx.snapis excluded by!**/*.snapapp/containers/Header/components/HeaderButton/__snapshots__/HeaderButtons.test.tsx.snapis excluded by!**/*.snapapp/containers/List/__snapshots__/List.test.tsx.snapis excluded by!**/*.snapapp/containers/MediaCallHeader/__snapshots__/MediaCallHeader.test.tsx.snapis excluded by!**/*.snapapp/containers/NewMediaCall/__snapshots__/CreateCall.test.tsx.snapis excluded by!**/*.snapapp/containers/NewMediaCall/__snapshots__/PeerItem.test.tsx.snapis excluded by!**/*.snapapp/containers/NewMediaCall/__snapshots__/PeerList.test.tsx.snapis excluded by!**/*.snapapp/containers/NewMediaCall/__snapshots__/SelectedPeer.test.tsx.snapis excluded by!**/*.snapapp/containers/RoomHeader/__snapshots__/RoomHeader.test.tsx.snapis excluded by!**/*.snapapp/containers/RoomItem/__snapshots__/RoomItem.test.tsx.snapis excluded by!**/*.snapapp/containers/RoomTypeIcon/__snapshots__/RoomTypeIcon.test.tsx.snapis excluded by!**/*.snapapp/containers/Status/__snapshots__/Status.test.tsx.snapis excluded by!**/*.snapapp/views/DiscussionsView/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snapapp/views/RoomInfoView/components/__snapshots__/RoomInfoABAC.test.tsx.snapis excluded by!**/*.snapapp/views/RoomView/LoadMore/__snapshots__/LoadMore.test.tsx.snapis excluded by!**/*.snapapp/views/ThreadMessagesView/__snapshots__/Item.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (39)
app/containers/NewMediaCall/Container.stories.tsxapp/containers/NewMediaCall/Container.tsxapp/containers/NewMediaCall/CreateCall.stories.tsxapp/containers/NewMediaCall/CreateCall.test.tsxapp/containers/NewMediaCall/CreateCall.tsxapp/containers/NewMediaCall/NewMediaCall.stories.tsxapp/containers/NewMediaCall/NewMediaCall.tsxapp/containers/NewMediaCall/PeerItem.stories.tsxapp/containers/NewMediaCall/PeerItem.test.tsxapp/containers/NewMediaCall/PeerItem.tsxapp/containers/NewMediaCall/PeerItemInner.tsxapp/containers/NewMediaCall/PeerList.stories.tsxapp/containers/NewMediaCall/PeerList.test.tsxapp/containers/NewMediaCall/PeerList.tsxapp/containers/NewMediaCall/SelectedPeer.stories.tsxapp/containers/NewMediaCall/SelectedPeer.test.tsxapp/containers/NewMediaCall/SelectedPeer.tsxapp/containers/NewMediaCall/index.tsxapp/containers/Status/Status.tsxapp/i18n/locales/en.jsonapp/lib/constants/colors.tsapp/lib/constants/defaultSettings.tsapp/lib/hooks/useMediaCallPermission/useMediaCallPermission.test.tsapp/lib/hooks/useNewMediaCall/index.tsapp/lib/hooks/useNewMediaCall/useNewMediaCall.test.tsxapp/lib/hooks/useNewMediaCall/useNewMediaCall.tsxapp/lib/hooks/useSubscription.tsapp/lib/services/voip/getPeerAutocompleteOptions.tsapp/lib/services/voip/usePeerAutocompleteStore.test.tsapp/lib/services/voip/usePeerAutocompleteStore.tsapp/views/NewMessageView/Item.test.tsxapp/views/NewMessageView/Item.tsxapp/views/RoomActionsView/components/CallSection.tsxapp/views/RoomInfoView/components/BaseButton.tsxapp/views/RoomInfoView/components/RoomInfoButtons.test.tsxapp/views/RoomInfoView/components/RoomInfoButtons.tsxapp/views/RoomView/RightButtons.tsxapp/views/RoomView/components/HeaderCallButton.tsxapp/views/SidebarView/components/Stacks.tsx
💤 Files with no reviewable changes (1)
- app/views/RoomView/RightButtons.tsx
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
app/views/RoomInfoView/components/RoomInfoButtons.tsx
🔇 Additional comments (23)
app/i18n/locales/en.json (1)
332-332: Localization additions look consistent.These keys fit the existing locale naming scheme and the copy reads well for the new pre-flight/new-call flow.
Also applies to: 577-577
app/lib/constants/defaultSettings.ts (1)
315-317: LGTM!The new
VoIP_TeamCollab_SIP_Integration_For_Internal_Callssetting follows the established naming convention and type definition pattern. It's appropriately placed alongside other VoIP-related settings and supports the Pre-flight feature for differentiating internal vs external calls.app/containers/Status/Status.tsx (1)
19-27: LGTM!The addition of
textAlign: 'center'properly complements the existingtextAlignVertical: 'center'to ensure the status icon is centered both horizontally and vertically within its bounds. This is the correct approach for centering icon fonts in React Native.app/lib/hooks/useNewMediaCall/index.ts (1)
1-1: LGTM!Standard barrel export pattern for module organization.
app/views/RoomActionsView/components/CallSection.tsx (1)
6-6: LGTM!Clean migration from direct
mediaSessionInstanceusage to theuseNewMediaCallhook. The permission gating and existing functionality are preserved.Also applies to: 16-16, 29-29
app/containers/NewMediaCall/NewMediaCall.tsx (1)
9-21: LGTM!The component correctly resets the peer autocomplete store on unmount via the cleanup function. The composition of sub-components is clean and well-organized.
app/containers/NewMediaCall/PeerItem.tsx (1)
1-22: LGTM!The component properly handles platform-specific press feedback (iOS pressed state vs Android ripple) and delegates content rendering to
PeerItemInner. The JSX transform handles React imports automatically.app/containers/NewMediaCall/PeerItem.stories.tsx (1)
1-57: LGTM!The story file is well-structured with good coverage of different
PeerItemvariants (user, SIP, and long label for truncation testing). The static data approach without render-time side effects is appropriate for Storybook.app/containers/NewMediaCall/PeerItem.test.tsx (1)
1-80: LGTM!The test file provides good coverage for
PeerItem:
- Validates rendering with correct testIDs and labels for both user and SIP types
- Verifies press interactions trigger the callback with the correct item
- Includes snapshot generation for visual regression testing
app/containers/NewMediaCall/SelectedPeer.test.tsx (1)
1-87: LGTM!Solid test coverage for
SelectedPeer:
- Correctly tests null rendering when no peer is selected
- Validates user and SIP peer label display
- Verifies the clear button invokes
clearSelection- Proper store state initialization in
beforeEachprevents test pollutionapp/containers/NewMediaCall/PeerList.test.tsx (1)
1-130: LGTM!Comprehensive test coverage for
PeerList:
- Tests conditional rendering based on
selectedPeerstate- Validates option selection triggers correct store actions (
setSelectedPeer,setFilter,fetchOptions)- Covers both user and SIP item selection paths
- Properly isolates tests with
beforeEachstore resetapp/lib/hooks/useNewMediaCall/useNewMediaCall.test.tsx (1)
1-106: LGTM!Well-structured tests for the
useNewMediaCallhook:
- Properly mocks all dependencies (useSubscription, useMediaCallPermission, ActionSheet, store)
- Tests the three key scenarios: DM peer exists, no DM peer, and missing room
- The helper
expectNewMediaCallActionSheetreduces duplication and improves readability- Validates that the action sheet opens regardless of permission state, allowing users to access the pre-flight UI
app/containers/NewMediaCall/PeerList.tsx (1)
30-33: Both function calls serve distinct purposes and are both necessary.
fetchOptions('')clears the options array (line 28-30 in usePeerAutocompleteStore.ts checks for empty term and sets options to empty array), whilesetFilter('')updates the filter state. These are complementary operations, not redundant ones. When a peer is selected, clearing both the options and the filter is the correct behavior.> Likely an incorrect or invalid review comment.app/views/RoomInfoView/components/RoomInfoButtons.tsx (1)
40-154: Clean refactor to use the new hook and BaseButton.The migration from
useMediaCallPermissiontouseNewMediaCallis well-implemented. The button visibility logic usinghasMediaCallPermissionand the action wiring toopenNewMediaCallare correctly integrated. The extraction ofBaseButtonimproves reusability.app/containers/NewMediaCall/Container.tsx (1)
12-50: Well-structured container with proper debouncing.The debounced fetch pattern and store integration are correctly implemented. The component cleanly separates concerns between UI rendering and state management.
app/containers/NewMediaCall/PeerItemInner.tsx (1)
10-32: Clean implementation differentiating SIP and user peers.The component appropriately handles the two peer types with distinct visual treatments: phone icon for SIP and avatar with status for users. The layout and styling are well-organized.
app/containers/NewMediaCall/SelectedPeer.tsx (1)
16-40: Good separation between container and presentation.The split between
SelectedPeer(store connection) andSelectedPeerInner(presentation) enables easier testing. Accessibility is properly handled withaccessibilityLabelandaccessibilityRole.One minor consideration:
SelectedPeerInnerstill accesses the store directly forclearSelection(line 18), which slightly couples it to the store. If you want full testability, consider passingclearSelectionas a prop.app/views/RoomInfoView/components/RoomInfoButtons.test.tsx (2)
14-26: Tests properly updated for the new hook.The mock structure correctly reflects the new
useNewMediaCallAPI withopenNewMediaCallandhasMediaCallPermission. UsingnoopOpenNewMediaCallfor non-call-verification tests andmockOpenNewMediaCallfor assertion is a clean pattern.
114-126: Voice call test correctly verifies the new flow.The test properly sets up the mock with
mockOpenNewMediaCalland verifies it's called when pressing the voice call button.app/views/SidebarView/components/Stacks.tsx (1)
5-38: LGTM!The refactoring cleanly replaces the previous VOIP logic with the new
useNewMediaCallhook. The permission-gated conditional rendering is well-structured, and the Fragment wrapper correctly groups the item with its separator.app/containers/NewMediaCall/SelectedPeer.stories.tsx (1)
26-60: Good test fixture coverage.The fixtures effectively cover the main peer types (user, sip) and edge cases (long username for truncation testing). The
Allstory provides a convenient overview for visual testing.app/views/NewMessageView/Item.test.tsx (2)
10-35: LGTM!The mock setup is well-structured. Jest hoists
jest.mockcalls, so theNewMediaCallimport on line 10 correctly receives the mocked version, making the type comparison on line 116 work as expected.
129-139: Good guard test for falsy userId.This properly validates that the call flow is short-circuited when
userIdis empty, preventing invalid peer data from being set.
There was a problem hiding this comment.
Pull request overview
Adds a new “pre-flight” media-call flow (action sheet with peer search/selection) and wires existing call entry points to open this flow, alongside related theming and snapshot updates.
Changes:
- Introduces
NewMediaCallaction sheet UI with a Zustand-backed peer autocomplete store and a REST-backed autocomplete option builder (user + SIP). - Refactors call triggers (sidebar, room header/actions/info, new message) to use
useNewMediaCallinstead of directly calling the media session. - Updates dark theme
fontDefault, centers icon text alignment, adds i18n strings, and regenerates snapshots.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/views/ThreadMessagesView/snapshots/Item.test.tsx.snap | Snapshot update for dark theme font color. |
| app/views/SidebarView/components/Stacks.tsx | Sidebar “Voice call” entry now opens new media call flow (permission-gated). |
| app/views/RoomView/components/HeaderCallButton.tsx | Header call button now opens new media call pre-flight for media calls. |
| app/views/RoomView/RightButtons.tsx | Removes room prop from HeaderCallButton usage. |
| app/views/RoomView/LoadMore/snapshots/LoadMore.test.tsx.snap | Snapshot update for dark theme font color. |
| app/views/RoomInfoView/components/snapshots/RoomInfoABAC.test.tsx.snap | Snapshot update for dark theme font color. |
| app/views/RoomInfoView/components/RoomInfoButtons.tsx | Refactors voice call button to open new media call flow; extracts BaseButton. |
| app/views/RoomInfoView/components/RoomInfoButtons.test.tsx | Updates tests to mock and assert useNewMediaCall behavior. |
| app/views/RoomInfoView/components/BaseButton.tsx | New shared base button component extracted from RoomInfo buttons. |
| app/views/RoomActionsView/components/CallSection.tsx | Room actions voice call now opens new media call flow via useNewMediaCall. |
| app/views/NewMessageView/Item.tsx | New message list call button now sets selected peer and opens NewMediaCall sheet. |
| app/views/NewMessageView/Item.test.tsx | Updates tests to assert new action sheet flow + selected peer state. |
| app/views/DiscussionsView/snapshots/Item.test.tsx.snap | Snapshot update for dark theme font color. |
| app/lib/services/voip/usePeerAutocompleteStore.ts | New Zustand store for filter/selection/options and async fetching. |
| app/lib/services/voip/usePeerAutocompleteStore.test.ts | Unit tests for the new peer autocomplete store. |
| app/lib/services/voip/getPeerAutocompleteOptions.ts | New autocomplete option builder (SIP option + user options), supports SIP routing setting. |
| app/lib/hooks/useSubscription.ts | New hook to load a subscription model by room id. |
| app/lib/hooks/useNewMediaCall/useNewMediaCall.tsx | New hook that opens the NewMediaCall sheet and preselects DM peer when possible. |
| app/lib/hooks/useNewMediaCall/useNewMediaCall.test.tsx | Unit tests for useNewMediaCall. |
| app/lib/hooks/useNewMediaCall/index.ts | Barrel export for useNewMediaCall. |
| app/lib/hooks/useMediaCallPermission/useMediaCallPermission.test.ts | Minor formatting change in expectations. |
| app/lib/constants/defaultSettings.ts | Adds VoIP_TeamCollab_SIP_Integration_For_Internal_Calls boolean setting. |
| app/lib/constants/colors.ts | Updates dark theme fontDefault color. |
| app/i18n/locales/en.json | Adds “New call” + “Enter username or number” strings. |
| app/containers/Status/snapshots/Status.test.tsx.snap | Snapshot update for centered icon text alignment. |
| app/containers/Status/Status.tsx | Adds textAlign: 'center' to status icon text style. |
| app/containers/RoomTypeIcon/snapshots/RoomTypeIcon.test.tsx.snap | Snapshot update for centered icon text alignment. |
| app/containers/RoomItem/snapshots/RoomItem.test.tsx.snap | Snapshot update for centered icon text alignment. |
| app/containers/RoomHeader/snapshots/RoomHeader.test.tsx.snap | Snapshot update for dark theme font color. |
| app/containers/NewMediaCall/index.tsx | Barrel export for NewMediaCall. |
| app/containers/NewMediaCall/snapshots/SelectedPeer.test.tsx.snap | New snapshots for SelectedPeer stories. |
| app/containers/NewMediaCall/snapshots/PeerList.test.tsx.snap | New snapshots for PeerList stories. |
| app/containers/NewMediaCall/snapshots/PeerItem.test.tsx.snap | New snapshots for PeerItem stories. |
| app/containers/NewMediaCall/snapshots/CreateCall.test.tsx.snap | New snapshots for CreateCall stories. |
| app/containers/NewMediaCall/SelectedPeer.tsx | New component rendering the currently selected peer chip with clear action. |
| app/containers/NewMediaCall/SelectedPeer.test.tsx | Tests for SelectedPeer behavior. |
| app/containers/NewMediaCall/SelectedPeer.stories.tsx | Stories for SelectedPeer variants. |
| app/containers/NewMediaCall/PeerList.tsx | New list of peer options that sets selection in the store. |
| app/containers/NewMediaCall/PeerList.test.tsx | Tests for PeerList rendering and selection behavior. |
| app/containers/NewMediaCall/PeerList.stories.tsx | Stories for PeerList. |
| app/containers/NewMediaCall/PeerItemInner.tsx | Shared inner rendering for user vs SIP items (avatar/status vs icon). |
| app/containers/NewMediaCall/PeerItem.tsx | Pressable option row component for peers. |
| app/containers/NewMediaCall/PeerItem.test.tsx | Tests for PeerItem selection behavior. |
| app/containers/NewMediaCall/PeerItem.stories.tsx | Stories for PeerItem variants. |
| app/containers/NewMediaCall/NewMediaCall.tsx | New action sheet body wiring selection + list + create call, with store reset on unmount. |
| app/containers/NewMediaCall/NewMediaCall.stories.tsx | Stories for NewMediaCall flow states. |
| app/containers/NewMediaCall/CreateCall.tsx | Call CTA button that starts the call based on selected peer and closes sheet. |
| app/containers/NewMediaCall/CreateCall.test.tsx | Tests for CreateCall enabled/disabled and call invocation behavior. |
| app/containers/NewMediaCall/CreateCall.stories.tsx | Stories for CreateCall enabled/disabled. |
| app/containers/NewMediaCall/Container.tsx | New container with search input, debounced fetching, and title/label. |
| app/containers/NewMediaCall/Container.stories.tsx | Stories for Container with/without filter. |
| app/containers/MediaCallHeader/snapshots/MediaCallHeader.test.tsx.snap | Snapshot update for centered icon text alignment and updated timer text. |
| app/containers/List/snapshots/List.test.tsx.snap | Snapshot update for dark theme font color. |
| app/containers/Header/components/HeaderButton/snapshots/HeaderButtons.test.tsx.snap | Snapshot update for dark theme font color. |
| app/containers/Avatar/snapshots/Avatar.test.tsx.snap | Snapshot update for centered icon text alignment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…anagement and adding FilterHeader
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
app/views/NewMessageView/Item.test.tsx:1
- This assertion will fail because
Item.tsxnow callssetSelectedPeerwith an additionalusernamefield. Update the expectation to includeusername, or switch toexpect.objectContaining({ type, value, label, username })to avoid brittle exact-shape matching.
app/lib/hooks/useSubscription.ts:1 - The async
load()can resolve afterridchanges or after unmount, causing stale updates (and potential 'setState on unmounted component' warnings). Add a cancellation guard (e.g.,let cancelled = falsein the effect cleanup) and only callsetSubscriptionwhen the effect instance is still current.
app/lib/hooks/useNewMediaCall/useNewMediaCall.tsx:1 - Because
useSubscription(rid)populates asynchronously,roomcan beundefinedat press time even for DMs, so the DM peer won’t be preselected (regression vs the prior direct-room call behavior). Consider makingopenNewMediaCallresolve the subscription on-demand (awaitgetSubscriptionByRoomId(rid)inside the handler) or accept theroomobject from callers that already have it.
app/lib/services/voip/usePeerAutocompleteStore.ts:1 - Concurrent
fetchOptionscalls can race (e.g., a slower request for an older term resolves after a newer one) and overwriteoptionswith stale results. Track a request id / latest term in state and only apply results if they match the most recent request, or cancel in-flight requests where possible.
app/views/RoomInfoView/components/BaseButton.tsx:1 - The
onPresstype usesany, which weakens type safety for a shared UI primitive. Prefer a narrower type such as() => void(if you ignore the event) or(event: GestureResponderEvent) => voidto match React Native press handlers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+31
| setSelectedPeer(peerItem); | ||
| setFilter(''); | ||
| fetchOptions(''); |
There was a problem hiding this comment.
fetchOptions('') immediately clears options, but the store already clears options on empty terms; depending on how fetchOptions is implemented/used elsewhere, this can be redundant work. Consider replacing with a direct set({ options: [] }) action or extending reset/clear behavior to avoid the extra async call path.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/lib/services/voip/getPeerAutocompleteOptions.ts (1)
84-90:⚠️ Potential issue | 🔴 CriticalGate the SIP option behind external-call permission.
Lines 84-90 still prepend a
type: 'sip'entry for every non-empty term. In the current flow, that value is stored as-is, rendered byapp/containers/NewMediaCall/PeerList.tsx, and then passed straight tomediaSessionInstance.startCall(...)byapp/containers/NewMediaCall/CreateCall.tsx. That means internal-only users can still be presented an external-call path unless the caller filters it first.🔒 Suggested direction
export const getPeerAutocompleteOptions = async ({ filter, - peerInfo + peerInfo, + allowExternalVoiceCalls }: { filter: string; peerInfo?: TPeerItem | null; + allowExternalVoiceCalls: boolean; }): Promise<TPeerItem[]> => { @@ - const sipOption: TPeerItem = { - type: 'sip', - value: term, - label: term - }; - - return [sipOption, ...userOptions]; + const sipOptions: TPeerItem[] = allowExternalVoiceCalls + ? [ + { + type: 'sip', + value: term, + label: term + } + ] + : []; + + return [...sipOptions, ...userOptions]; };If you prefer not to thread the permission into this helper, the equivalent filter needs to happen immediately after this function returns, before
PeerListrenders the options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/getPeerAutocompleteOptions.ts` around lines 84 - 90, The function getPeerAutocompleteOptions currently always prepends a sipOption (type: 'sip') which enables an external-call path for all users; modify getPeerAutocompleteOptions to only include the sipOption when the caller has external-call permission by either: 1) adding a boolean parameter (e.g., allowExternalCall or canExternalCall) and only prepending sipOption when true, or 2) querying the existing permission helper (e.g., hasPermission / canMakeExternalCalls) inside getPeerAutocompleteOptions before creating sipOption; update any call sites to pass the permission flag if you choose the parameter approach so only authorized users see the SIP option.
🧹 Nitpick comments (1)
app/containers/NewMediaCall/PeerList.tsx (1)
17-31: Avoid rebuildingTPeerItembefore storing it.Line 17 already receives a
TPeerItem. Reconstructing it here duplicates the union logic and makes future shape changes easier to miss. Passing the original option keeps the type source in one place.♻️ Proposed simplification
const handleSelectOption = (option: TPeerItem) => { - const peerItem: TPeerItem = - option.type === 'sip' - ? { type: 'sip', value: option.value, label: option.label } - : { - type: 'user', - value: option.value, - label: option.label, - username: option.username, - callerId: option.callerId - }; - - setSelectedPeer(peerItem); + setSelectedPeer(option); setFilter(''); fetchOptions(''); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/PeerList.tsx` around lines 17 - 31, The handler handleSelectOption rebuilds a TPeerItem unnecessarily; instead of reconstructing the union object, pass the received option directly to setSelectedPeer (i.e., call setSelectedPeer(option)), then clear the filter and call fetchOptions as before (keep setFilter('') and fetchOptions('')); update handleSelectOption to remove the manual TPeerItem construction so future changes to TPeerItem shape are not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/lib/services/voip/getPeerAutocompleteOptions.ts`:
- Around line 84-90: The function getPeerAutocompleteOptions currently always
prepends a sipOption (type: 'sip') which enables an external-call path for all
users; modify getPeerAutocompleteOptions to only include the sipOption when the
caller has external-call permission by either: 1) adding a boolean parameter
(e.g., allowExternalCall or canExternalCall) and only prepending sipOption when
true, or 2) querying the existing permission helper (e.g., hasPermission /
canMakeExternalCalls) inside getPeerAutocompleteOptions before creating
sipOption; update any call sites to pass the permission flag if you choose the
parameter approach so only authorized users see the SIP option.
---
Nitpick comments:
In `@app/containers/NewMediaCall/PeerList.tsx`:
- Around line 17-31: The handler handleSelectOption rebuilds a TPeerItem
unnecessarily; instead of reconstructing the union object, pass the received
option directly to setSelectedPeer (i.e., call setSelectedPeer(option)), then
clear the filter and call fetchOptions as before (keep setFilter('') and
fetchOptions('')); update handleSelectOption to remove the manual TPeerItem
construction so future changes to TPeerItem shape are not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04d13d37-efae-42f3-9525-52e566f5d030
⛔ Files ignored due to path filters (1)
app/containers/NewMediaCall/__snapshots__/FilterHeader.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
app/containers/NewMediaCall/Container.stories.tsxapp/containers/NewMediaCall/Container.tsxapp/containers/NewMediaCall/CreateCall.test.tsxapp/containers/NewMediaCall/CreateCall.tsxapp/containers/NewMediaCall/FilterHeader.stories.tsxapp/containers/NewMediaCall/FilterHeader.test.tsxapp/containers/NewMediaCall/FilterHeader.tsxapp/containers/NewMediaCall/NewMediaCall.tsxapp/containers/NewMediaCall/PeerList.tsxapp/lib/hooks/useSubscription.tsapp/lib/services/voip/getPeerAutocompleteOptions.tsapp/lib/services/voip/usePeerAutocompleteStore.test.tsapp/lib/services/voip/usePeerAutocompleteStore.tsapp/views/NewMessageView/Item.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/containers/NewMediaCall/NewMediaCall.tsx
- app/lib/hooks/useSubscription.ts
- app/lib/services/voip/usePeerAutocompleteStore.ts
- app/containers/NewMediaCall/Container.stories.tsx
- app/containers/NewMediaCall/Container.tsx
📜 Review details
🔇 Additional comments (4)
app/views/NewMessageView/Item.tsx (1)
28-33: Pre-flight flow wiring looks correct.Setting the selected peer before opening
NewMediaCall, and carryingusernamethrough with the peer payload, keeps the downstream action-sheet UI consistent with the selected user.app/containers/NewMediaCall/FilterHeader.tsx (1)
20-28: Debounced search handoff looks correct.Lines 24-27 update the local filter immediately, clear any stale selection, and defer the fetch, which matches the expected autocomplete UX.
app/containers/NewMediaCall/FilterHeader.test.tsx (1)
39-69: Good coverage for the debounce contract.This test pins the important behavior: filter updates happen immediately, while
fetchOptionswaits fortextInputDebounceTime.app/containers/NewMediaCall/CreateCall.tsx (1)
17-24: Correctly routing the selected peer type now.Lines 22-23 forward
selectedPeer.typedirectly intostartCall(...), which matches both theTPeerItemdiscriminator and theMediaSessionInstancecontract.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/containers/MediaCallHeader/__snapshots__/MediaCallHeader.test.tsx.snap
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/containers/NewMediaCall/PeerList.tsx (2)
32-41: FlatList configuration looks good; minor style optimization available.The
keyboardShouldPersistTaps='always'is correct for allowing selection while the keyboard is visible. ThekeyExtractorusingitem.valueis safe sinceTPeerItemguaranteesvalueis always a string for both union members.The inline
contentContainerStyleobject creates a new reference on each render. Extracting it is a minor optimization.♻️ Optional: extract style constant
+const contentStyle = { flexShrink: 1 }; + export const PeerList = () => { ... return ( <FlatList data={options} - contentContainerStyle={{ flexShrink: 1 }} + contentContainerStyle={contentStyle} keyExtractor={item => item.value} ... /> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/PeerList.tsx` around lines 32 - 41, The inline contentContainerStyle object passed to FlatList causes a new reference on each render; extract it to a stable constant (e.g., const flatListContentContainerStyle = { flexShrink: 1 }) and use that constant in the FlatList's contentContainerStyle prop to avoid unnecessary re-renders; update the component where FlatList is rendered (look for FlatList and contentContainerStyle usage in PeerList.tsx) to reference the new constant.
15-28: Implementation is correct; consider memoizing for stability.The type normalization for 'sip' vs 'user' items is handled correctly, ensuring all required fields are passed. For optimal FlatList performance,
handleSelectOptioncould be wrapped inuseCallbackto maintain a stable reference, though the impact is minimal since the component returnsnullafter selection anyway.♻️ Optional: wrap in useCallback
+import { useCallback } from 'react'; +import { FlatList } from 'react-native'; -import { FlatList } from 'react-native'; ... - const handleSelectOption = (option: TPeerItem) => { + const handleSelectOption = useCallback((option: TPeerItem) => { const peerItem: TPeerItem = option.type === 'sip' ? { type: 'sip', value: option.value, label: option.label } : { type: 'user', value: option.value, label: option.label, username: option.username, callerId: option.callerId }; setSelectedPeer(peerItem); - }; + }, [setSelectedPeer]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/NewMediaCall/PeerList.tsx` around lines 15 - 28, The handler handleSelectOption should be memoized to provide a stable reference for consumers (e.g., FlatList); wrap handleSelectOption in React.useCallback, keep its logic identical, and include setSelectedPeer in the dependency array (and any other external values if used). Locate the function handleSelectOption and replace its declaration with a useCallback-wrapped version that returns the same TPeerItem normalization and calls setSelectedPeer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/containers/NewMediaCall/PeerList.tsx`:
- Around line 32-41: The inline contentContainerStyle object passed to FlatList
causes a new reference on each render; extract it to a stable constant (e.g.,
const flatListContentContainerStyle = { flexShrink: 1 }) and use that constant
in the FlatList's contentContainerStyle prop to avoid unnecessary re-renders;
update the component where FlatList is rendered (look for FlatList and
contentContainerStyle usage in PeerList.tsx) to reference the new constant.
- Around line 15-28: The handler handleSelectOption should be memoized to
provide a stable reference for consumers (e.g., FlatList); wrap
handleSelectOption in React.useCallback, keep its logic identical, and include
setSelectedPeer in the dependency array (and any other external values if used).
Locate the function handleSelectOption and replace its declaration with a
useCallback-wrapped version that returns the same TPeerItem normalization and
calls setSelectedPeer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d5f00e7-838b-420f-bee3-da0643e381db
⛔ Files ignored due to path filters (1)
app/containers/MediaCallHeader/__snapshots__/MediaCallHeader.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
app/containers/NewMediaCall/PeerList.test.tsxapp/containers/NewMediaCall/PeerList.tsxapp/lib/services/voip/usePeerAutocompleteStore.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/containers/NewMediaCall/PeerList.test.tsx
- app/lib/services/voip/usePeerAutocompleteStore.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (2)
app/containers/NewMediaCall/PeerList.tsx (2)
8-12: Good use of granular Zustand selectors.Using separate
usePeerAutocompleteStorecalls with individual selectors is the recommended pattern for Zustand—it ensures the component only re-renders when the specific subscribed state changes, rather than on any store update.
1-42: Overall implementation is solid.The component correctly integrates with the Zustand store, handles both peer types properly, and follows standard React Native patterns for list rendering. The early return when a peer is selected keeps the UI clean. Good work.
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
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.
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-15
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests