feat: Dialpad#7000
Merged
Merged
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-02-05T13:55:00.974ZApplied to files:
🔇 Additional comments (3)
WalkthroughAdds a Dialpad feature: new Dialpad UI (component, button, styles), store state + action for dialpadValue and DTMF sending, CallView integration via ActionSheet, Storybook and unit tests, small VoIP answer-call UUID check, haptics/additional test mocks, and a translations entry. Changes
Sequence DiagramsequenceDiagram
participant User
participant CallButtons
participant ActionSheet
participant Dialpad
participant Store
participant VoIP
User->>CallButtons: tap Dialpad button
CallButtons->>ActionSheet: showActionSheetRef({ children: Dialpad })
ActionSheet->>Dialpad: render Dialpad UI
Dialpad->>Store: read useDialpadValue
User->>Dialpad: press key "5"
Dialpad->>Store: setDialpadValue("...5")
Store->>VoIP: sendDTMF("5")
VoIP->>Store: emit DTMF event
Store->>Dialpad: update dialpadValue
Dialpad->>User: display updated input
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 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 |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/lib/services/voip/MediaSessionInstance.ts (2)
70-72:⚠️ Potential issue | 🟠 MajorUnhandled promise rejection from unawaited
answerCall.
answerCallisasyncand internallyawaitsmainCall.accept(). Calling it withoutawait(or.catch()) inside the synchronous'newCall'event handler silently swallows any rejection (e.g., network failure during the accept handshake).🐛 Proposed fix
- this.answerCall(existingCallUUID); + this.answerCall(existingCallUUID).catch(e => console.error('[VoIP] Failed to answer existing call:', e));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 70 - 72, The 'newCall' handler is calling the async method answerCall(existingCallUUID) without handling its promise, which can cause unhandled rejections (answerCall awaits mainCall.accept()); make the handler handle the promise by either marking the 'newCall' event handler async and awaiting this.answerCall(existingCallUUID) or by explicitly calling this.answerCall(existingCallUUID).catch(err => /* log or handle */), and ensure errors from mainCall.accept() are logged/handled (use existing logger) to avoid silent failures.
70-72:⚠️ Potential issue | 🟠 MajorUnhandled promise rejection —
answerCallis called withoutawaitor.catch().
answerCallisasyncand internally awaitsmainCall.accept(). Calling it bare inside the synchronous'newCall'event handler means any rejection (e.g., WebRTC accept failure) is silently swallowed, leaving the call in an indeterminate state with no user feedback.🐛 Proposed fix
- this.answerCall(existingCallUUID); + this.answerCall(existingCallUUID).catch(e => console.error('[VoIP] Failed to answer existing call:', e));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 70 - 72, The 'newCall' event handler calls the async method answerCall(existingCallUUID) without awaiting or handling rejections; update the handler so answerCall is awaited or its returned promise has a .catch to log/handle errors (so failures from mainCall.accept() inside answerCall are not unhandled). Locate the 'newCall' event block where existingCallUUID is checked and modify it to either make the event handler async and await this.answerCall(existingCallUUID) or append .catch(...) to this.answerCall(existingCallUUID) to surface errors, ensuring any exceptions are logged and the call state is cleaned up.app/views/CallView/index.tsx (1)
1-28:⚠️ Potential issue | 🟠 MajorScreen may sleep during an active call — keep-awake was removed with no replacement.
The AI-generated summary confirms
activateKeepAwakeAsyncon mount anddeactivateKeepAwakeon unmount were deleted from this file. During an active VoIP call the device idle timer will dim and eventually lock the screen, preventing the user from accessing Hold, Mute, Speaker, and the new Dialpad controls entirely.RNCallKeepmanages OS-level call state but does not prevent the screen from sleeping for the in-app UI.🔒 Restore keep-awake lifecycle
import React from 'react'; +import { useEffect } from 'react'; import { View } from 'react-native'; +import { activateKeepAwakeAsync, deactivateKeepAwake } from 'expo-keep-awake'; const CallView = (): React.ReactElement | null => { 'use memo'; const { colors } = useTheme(); const call = useCallStore(state => state.call); + useEffect(() => { + activateKeepAwakeAsync(); + return () => { + deactivateKeepAwake(); + }; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/index.tsx` around lines 1 - 28, CallView currently no longer prevents screen sleep during an active call; restore a keep-awake lifecycle by importing and calling activateKeepAwakeAsync when the CallView mounts and calling deactivateKeepAwake on unmount (use a React useEffect inside CallView to run these), ensuring the calls only run while call exists (use the existing call from useCallStore). Add the necessary import for the keep-awake API (e.g., activateKeepAwakeAsync / deactivateKeepAwake) and ensure cleanup on unmount to avoid leaks.
🧹 Nitpick comments (9)
app/lib/services/voip/MediaSessionInstance.ts (1)
100-103: Replacealert()withAlert.alert()fromreact-native.React Native polyfills
global.alertinternally, so this call functions today, but it is non-idiomatic for React Native and relies on a fragile internal implementation detail. UseAlert.alert()instead for consistency with RN conventions.Suggested refactor
+import { Alert } from 'react-native'; ... } else { RNCallKeep.endCall(callUUID); - alert('Call not found'); // TODO: Show error message? + Alert.alert('Error', 'Call not found'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 100 - 103, Replace the non-idiomatic global alert call with React Native's Alert API: change the alert('Call not found') call inside the MediaSessionInstance method where RNCallKeep.endCall(callUUID) is invoked to use Alert.alert(...) instead and add/ensure import { Alert } from 'react-native' at the top of MediaSessionInstance.ts; this removes reliance on global.alert and uses the idiomatic Alert API for consistent RN behavior.app/views/CallView/components/Dialpad/Dialpad.tsx (1)
37-37: Missing'use memo'directive — inconsistent with sibling components.
CallView(line 11),CallButtons(line 13), andDialpadButton(line 14) all declare'use memo'at the top of their function bodies.Dialpaddoes not.♻️ Add 'use memo'
const Dialpad = ({ testID }: IDialpad): React.ReactElement => { + 'use memo'; + const { colors } = useTheme();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/Dialpad.tsx` at line 37, The Dialpad component is missing the 'use memo' directive at the top of its function body; update the Dialpad function (const Dialpad = ({ testID }: IDialpad): React.ReactElement => { ... }) to include the 'use memo' directive as the first statement inside the function so it matches sibling components (CallView, CallButtons, DialpadButton) and preserves consistent memoization behavior across components.app/views/CallView/components/Dialpad/styles.ts (1)
6-9:paddingBottom: 32is redundant.
padding: 32already sets all four sides to 32; the subsequentpaddingBottom: 32has no effect.♻️ Remove redundant property
container: { - padding: 32, - paddingBottom: 32 + padding: 32 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/styles.ts` around lines 6 - 9, The container style object in styles.ts redundantly sets paddingBottom after a blanket padding:32; remove the duplicate paddingBottom property from the container object so only padding: 32 remains (or if a different bottom padding was intended, replace padding:32 with explicit paddingTop/Left/Right and the intended paddingBottom in the container definition).app/views/CallView/components/Dialpad/DialpadButton.tsx (1)
32-32:letters || ''is always a no-op —lettersis typed asstring.An empty string is already falsy, so
|| ''falls through to the same empty string. The prop can be rendered directly.♻️ Simplify
- <Text style={[styles.letters, { color: colors.fontSecondaryInfo }]}>{letters || ''}</Text> + <Text style={[styles.letters, { color: colors.fontSecondaryInfo }]}>{letters}</Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/DialpadButton.tsx` at line 32, In DialpadButton.tsx inside the DialpadButton component, remove the unnecessary fallback on the Text render—replace the expression {letters || ''} with simply {letters} (since letters is typed as string), so update the JSX in the Text element that currently uses styles.letters and colors.fontSecondaryInfo to render letters directly.app/views/CallView/components/Dialpad/Dialpad.test.tsx (2)
47-65:'should render correctly'and'should display the input field'are functionally identical.Both render
<Dialpad testID='dialpad' />and assertgetByTestId('dialpad-input')is truthy. One of them should be removed or the remaining test repurposed to cover a distinct assertion (e.g., the grid renders 12 buttons).♻️ Remove or differentiate the duplicate test
- it('should render correctly', () => { - setStoreState(); - const { getByTestId } = render( - <Wrapper> - <Dialpad testID='dialpad' /> - </Wrapper> - ); - expect(getByTestId('dialpad-input')).toBeTruthy(); - }); - it('should display the input field', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/Dialpad.test.tsx` around lines 47 - 65, The two tests 'should render correctly' and 'should display the input field' are duplicates; remove one of them or change one to assert a distinct behavior: keep the test that checks getByTestId('dialpad-input') in Dialpad and update the other (e.g., rename to 'should render 12 dialpad buttons') to render <Dialpad testID='dialpad' /> and assert the dialpad buttons are present by querying the button elements (use the existing test helpers such as getAllByTestId('dialpad-button') or similar) and expect the length to equal the expected count (12); update the test name accordingly.
81-96: Consider adding a test for the accumulateddialpadValuedisplayed in the input.The current DTMF test verifies
sendDTMFis called but not that the input field actually reflects the accumulated value. An additional assertion like:expect(getByTestId('dialpad-input').props.value).toBe('52');after pressing
5then2would cover the store → UI binding end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/Dialpad.test.tsx` around lines 81 - 96, Add an assertion to the "should call sendDTMF and update value when digit is pressed" test to verify the accumulated dialpad value is rendered: after firing presses on '5' and '2' in the Dialpad test (the test that uses Wrapper and Dialpad and mocks sendDTMFMock), query the input via getByTestId('dialpad-input') and assert its props.value (or text content depending on your component) equals '52' to cover the store → UI binding. Ensure this assertion is placed after the two fireEvent.press calls and existing sendDTMFMock expectations.app/views/CallView/index.test.tsx (1)
265-269: Consider tightening the action sheet content assertion.
children: expect.anything()passes for any truthy or falsy value includingnull. Checkingexpect.any(Function)(for a render prop) or using a more specific matcher would give better confidence the Dialpad is actually wired in.✏️ Suggested refinement
- expect(mockShowActionSheetRef).toHaveBeenCalledWith( - expect.objectContaining({ - children: expect.anything() - }) - ); + expect(mockShowActionSheetRef).toHaveBeenCalledWith( + expect.objectContaining({ + children: expect.any(Function) + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/index.test.tsx` around lines 265 - 269, The test currently asserts mockShowActionSheetRef was called with children: expect.anything(), which allows null; tighten it by asserting children is a render function or more specific shape—update the expectation in the CallView test to use children: expect.any(Function) (or assert the provided function returns a Dialpad element) so the Dialpad render-prop wiring is actually validated; target the mockShowActionSheetRef call site and change the expect.objectContaining matcher accordingly.app/views/CallView/components/Dialpad/Dialpad.stories.tsx (1)
40-57: Store mutations inside render functions are a React antipattern.Both the decorator (line 42) and
WithValue(line 55) calluseCallStore.setState(viasetStoreState) directly in their render bodies. In React Strict Mode, render functions are invoked twice in development, which can cause the store to be set multiple times and produce subtle state race conditions between stories. Storybook'sloaders(orbeforeEachin story parameters for Storybook 7+) is the idiomatic way to perform setup side effects.♻️ Suggested refactor using Storybook `loaders`
export default { title: 'Dialpad', component: Dialpad, + loaders: [ + async () => { + setStoreState(); + } + ], decorators: [ (Story: React.ComponentType) => { - setStoreState(); return ( <Wrapper> <Story /> </Wrapper> ); } ] }; -export const WithValue = () => { - setStoreState({ dialpadValue: '123' }); - return <Dialpad testID='dialpad' />; -}; +export const WithValue = { + loaders: [ + async () => { + setStoreState({ dialpadValue: '123' }); + } + ], + render: () => <Dialpad testID='dialpad' /> +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/Dialpad.stories.tsx` around lines 40 - 57, The stories mutate the store during render (via setStoreState/useCallStore.setState in the decorator and the WithValue story), which is an antipattern and causes double-run issues in Strict Mode; move those side effects into Storybook loaders (add a loaders array on the default export for the decorator initialization and add a loader on the WithValue story or story.parameters.loaders) so that setStoreState is invoked once before rendering, and remove all calls to setStoreState from the decorator/WithValue render functions (use the existing Wrapper/Story and Dialpad components unchanged).app/lib/services/voip/useCallStore.ts (1)
166-166:setDialpadValueappends rather than sets — misleading name.The function appends
valuetodialpadValue(line 171:get().dialpadValue + value), but the name implies replacing the current value. Compare withsetCallUUIDwhich truly replaces. A name likeappendDialpadDigitorsendDTMFTonewould better reflect the behaviour, reducing future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/useCallStore.ts` at line 166, The current setDialpadValue function actually appends to get().dialpadValue instead of replacing it, so rename it to reflect intent (e.g., appendDialpadDigit or sendDTMFTone) and update its signature/usage accordingly: change the store key from setDialpadValue to appendDialpadDigit, keep the implementation using get().dialpadValue + value, and update all call sites to use the new name; also update any exported types/interfaces that reference setDialpadValue (compare with setCallUUID which truly replaces to ensure consistency).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
app/containers/MediaCallHeader/__snapshots__/MediaCallHeader.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/__snapshots__/index.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/components/Dialpad/__snapshots__/Dialpad.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/components/__snapshots__/CallerInfo.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
app/i18n/locales/en.jsonapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.tsapp/views/CallView/components/CallButtons.tsxapp/views/CallView/components/Dialpad/Dialpad.stories.tsxapp/views/CallView/components/Dialpad/Dialpad.test.tsxapp/views/CallView/components/Dialpad/Dialpad.tsxapp/views/CallView/components/Dialpad/DialpadButton.tsxapp/views/CallView/components/Dialpad/styles.tsapp/views/CallView/index.test.tsxapp/views/CallView/index.tsxjest.setup.js
🧰 Additional context used
🧬 Code graph analysis (6)
app/views/CallView/components/Dialpad/DialpadButton.tsx (4)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useCallStore(58-203)app/views/CallView/components/Dialpad/styles.ts (1)
styles(5-45)app/lib/constants/colors.ts (1)
colors(280-302)
app/views/CallView/components/CallButtons.tsx (1)
app/containers/ActionSheet/Provider.tsx (1)
showActionSheetRef(80-82)
app/lib/services/voip/useCallStore.ts (1)
app/containers/ActionSheet/Provider.tsx (1)
hideActionSheetRef(84-86)
app/views/CallView/components/Dialpad/Dialpad.stories.tsx (1)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(58-203)
app/views/CallView/components/Dialpad/Dialpad.test.tsx (2)
app/lib/services/voip/useCallStore.ts (1)
useCallStore(58-203).rnstorybook/generateSnapshots.tsx (1)
generateSnapshots(10-22)
app/views/CallView/components/Dialpad/Dialpad.tsx (5)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useDialpadValue(211-211)app/views/CallView/components/Dialpad/styles.ts (1)
styles(5-45)app/lib/constants/colors.ts (1)
colors(280-302)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
🔇 Additional comments (2)
app/i18n/locales/en.json (1)
244-244: LGTM — new i18n key added in correct alphabetical order.jest.setup.js (1)
163-165: LGTM — necessary mock forBottomSheetTextInputused byFormTextInputvia thebottomSheetprop.
🤖 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/lib/services/voip/MediaSessionInstance.ts`:
- Around line 100-103: Replace the browser/global alert call with React Native's
Alert API: in the else branch where RNCallKeep.endCall(callUUID) is called and
alert('Call not found') is used, change to use Alert.alert(...) from
'react-native' (ensure Alert is imported at the top of MediaSessionInstance.ts).
Update the call to Alert.alert with an appropriate title/message (e.g., 'Call
not found') to match the codebase pattern.
In `@app/lib/services/voip/useCallStore.ts`:
- Around line 166-173: setDialpadValue currently calls call.sendDTMF(value)
without error handling; wrap the sendDTMF invocation in a try/catch so DTMF
transmission failures don't propagate to the UI handler, e.g. call sendDTMF
inside try { ... } catch (err) { /* log via existing logger or console.error and
return or continue as appropriate */ }; still update dialpadValue via
get()/set() only after successful send or decide to append regardless but ensure
errors are caught; adjust references: setDialpadValue, call.sendDTMF, get(),
set({ dialpadValue }).
In `@app/views/CallView/components/CallButtons.tsx`:
- Line 80: The Dialpad CallActionButton is missing the disabled prop and allows
DTMF to be sent while connecting; update the Dialpad button in CallButtons.tsx
(the <CallActionButton ... label={I18n.t('Dialpad')} ... />) to pass
disabled={isConnecting} (consistent with Hold and Mute) and ensure
handleDialpad/setDialpadValue flow respects call state by checking isConnecting
or the call's active state before calling call.sendDTMF in useCallStore so DTMF
cannot be dispatched until the call is active.
In `@app/views/CallView/components/Dialpad/Dialpad.tsx`:
- Around line 43-52: The FormTextInput is rendered controlled via
value={dialpadValue} but lacks an onChangeText handler, so
clearButtonMode='while-editing' and keyboardType='phone-pad' produce misleading,
non-functional UI on iOS; update the Dialpad FormTextInput (component
FormTextInput) to be explicitly non-editable for display-only DTMF by removing
or disabling the interactive props (clearButtonMode and keyboardType) and add
editable={false}, or alternatively wire up onChangeText to update dialpadValue
in the store if the intent is to allow typing/clearing; adjust TestID if needed
(testID/dialpad-input) to reflect the new behavior.
In `@app/views/CallView/components/Dialpad/DialpadButton.tsx`:
- Around line 23-35: The Pressable rendering the dial key (the component using
Pressable with onPress={handleDigitPress}) lacks accessibilityRole and
accessibilityLabel; update that Pressable to include
accessibilityRole="keyboardkey" and an accessibilityLabel that combines the
digit and letters (use the existing digit and letters props/variables, e.g.
`${digit}${letters ? ' ' + letters : ''}`) so screen readers announce the key as
a single element; keep other props (style, onPress) unchanged and ensure you
reference the same Pressable, handleDigitPress, digit, and letters identifiers
when making the change.
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 70-72: The 'newCall' handler is calling the async method
answerCall(existingCallUUID) without handling its promise, which can cause
unhandled rejections (answerCall awaits mainCall.accept()); make the handler
handle the promise by either marking the 'newCall' event handler async and
awaiting this.answerCall(existingCallUUID) or by explicitly calling
this.answerCall(existingCallUUID).catch(err => /* log or handle */), and ensure
errors from mainCall.accept() are logged/handled (use existing logger) to avoid
silent failures.
- Around line 70-72: The 'newCall' event handler calls the async method
answerCall(existingCallUUID) without awaiting or handling rejections; update the
handler so answerCall is awaited or its returned promise has a .catch to
log/handle errors (so failures from mainCall.accept() inside answerCall are not
unhandled). Locate the 'newCall' event block where existingCallUUID is checked
and modify it to either make the event handler async and await
this.answerCall(existingCallUUID) or append .catch(...) to
this.answerCall(existingCallUUID) to surface errors, ensuring any exceptions are
logged and the call state is cleaned up.
In `@app/views/CallView/index.tsx`:
- Around line 1-28: CallView currently no longer prevents screen sleep during an
active call; restore a keep-awake lifecycle by importing and calling
activateKeepAwakeAsync when the CallView mounts and calling deactivateKeepAwake
on unmount (use a React useEffect inside CallView to run these), ensuring the
calls only run while call exists (use the existing call from useCallStore). Add
the necessary import for the keep-awake API (e.g., activateKeepAwakeAsync /
deactivateKeepAwake) and ensure cleanup on unmount to avoid leaks.
---
Nitpick comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 100-103: Replace the non-idiomatic global alert call with React
Native's Alert API: change the alert('Call not found') call inside the
MediaSessionInstance method where RNCallKeep.endCall(callUUID) is invoked to use
Alert.alert(...) instead and add/ensure import { Alert } from 'react-native' at
the top of MediaSessionInstance.ts; this removes reliance on global.alert and
uses the idiomatic Alert API for consistent RN behavior.
In `@app/lib/services/voip/useCallStore.ts`:
- Line 166: The current setDialpadValue function actually appends to
get().dialpadValue instead of replacing it, so rename it to reflect intent
(e.g., appendDialpadDigit or sendDTMFTone) and update its signature/usage
accordingly: change the store key from setDialpadValue to appendDialpadDigit,
keep the implementation using get().dialpadValue + value, and update all call
sites to use the new name; also update any exported types/interfaces that
reference setDialpadValue (compare with setCallUUID which truly replaces to
ensure consistency).
In `@app/views/CallView/components/Dialpad/Dialpad.stories.tsx`:
- Around line 40-57: The stories mutate the store during render (via
setStoreState/useCallStore.setState in the decorator and the WithValue story),
which is an antipattern and causes double-run issues in Strict Mode; move those
side effects into Storybook loaders (add a loaders array on the default export
for the decorator initialization and add a loader on the WithValue story or
story.parameters.loaders) so that setStoreState is invoked once before
rendering, and remove all calls to setStoreState from the decorator/WithValue
render functions (use the existing Wrapper/Story and Dialpad components
unchanged).
In `@app/views/CallView/components/Dialpad/Dialpad.test.tsx`:
- Around line 47-65: The two tests 'should render correctly' and 'should display
the input field' are duplicates; remove one of them or change one to assert a
distinct behavior: keep the test that checks getByTestId('dialpad-input') in
Dialpad and update the other (e.g., rename to 'should render 12 dialpad
buttons') to render <Dialpad testID='dialpad' /> and assert the dialpad buttons
are present by querying the button elements (use the existing test helpers such
as getAllByTestId('dialpad-button') or similar) and expect the length to equal
the expected count (12); update the test name accordingly.
- Around line 81-96: Add an assertion to the "should call sendDTMF and update
value when digit is pressed" test to verify the accumulated dialpad value is
rendered: after firing presses on '5' and '2' in the Dialpad test (the test that
uses Wrapper and Dialpad and mocks sendDTMFMock), query the input via
getByTestId('dialpad-input') and assert its props.value (or text content
depending on your component) equals '52' to cover the store → UI binding. Ensure
this assertion is placed after the two fireEvent.press calls and existing
sendDTMFMock expectations.
In `@app/views/CallView/components/Dialpad/Dialpad.tsx`:
- Line 37: The Dialpad component is missing the 'use memo' directive at the top
of its function body; update the Dialpad function (const Dialpad = ({ testID }:
IDialpad): React.ReactElement => { ... }) to include the 'use memo' directive as
the first statement inside the function so it matches sibling components
(CallView, CallButtons, DialpadButton) and preserves consistent memoization
behavior across components.
In `@app/views/CallView/components/Dialpad/DialpadButton.tsx`:
- Line 32: In DialpadButton.tsx inside the DialpadButton component, remove the
unnecessary fallback on the Text render—replace the expression {letters || ''}
with simply {letters} (since letters is typed as string), so update the JSX in
the Text element that currently uses styles.letters and colors.fontSecondaryInfo
to render letters directly.
In `@app/views/CallView/components/Dialpad/styles.ts`:
- Around line 6-9: The container style object in styles.ts redundantly sets
paddingBottom after a blanket padding:32; remove the duplicate paddingBottom
property from the container object so only padding: 32 remains (or if a
different bottom padding was intended, replace padding:32 with explicit
paddingTop/Left/Right and the intended paddingBottom in the container
definition).
In `@app/views/CallView/index.test.tsx`:
- Around line 265-269: The test currently asserts mockShowActionSheetRef was
called with children: expect.anything(), which allows null; tighten it by
asserting children is a render function or more specific shape—update the
expectation in the CallView test to use children: expect.any(Function) (or
assert the provided function returns a Dialpad element) so the Dialpad
render-prop wiring is actually validated; target the mockShowActionSheetRef call
site and change the expect.objectContaining matcher accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/views/CallView/components/Dialpad/Dialpad.tsx (1)
43-53:keyboardType='phone-pad'is now a no-op witheditable={false}— consider removing it.Since
editable={false}was added (addressing the earlier feedback), the keyboard will never appear, makingkeyboardTypedead configuration. Removing it avoids confusion for future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/Dialpad/Dialpad.tsx` around lines 43 - 53, The FormTextInput in Dialpad.tsx currently passes keyboardType='phone-pad' while also setting editable={false}, making keyboardType a no-op; remove the keyboardType prop from the FormTextInput invocation (inside the Dialpad component) to avoid misleading/unused configuration and keep only props that affect behavior (i.e., retain editable={false}, value={dialpadValue}, containerStyle, testID, multiline, etc.).
🤖 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/views/CallView/components/CallButtons.tsx`:
- Around line 35-37: handleDialpad currently opens the Dialpad via
showActionSheetRef but never clears previous input; update handleDialpad to pass
a dismissal/close handler to showActionSheetRef that calls setDialpadValue('')
when the action sheet is dismissed (or alternatively add a useEffect in the
Dialpad component to call setDialpadValue('') on mount), referencing the
existing handleDialpad, showActionSheetRef, setDialpadValue and Dialpad symbols
so stale DTMF input is cleared whenever the dialpad is closed or reopened.
---
Duplicate comments:
In `@app/views/CallView/components/Dialpad/Dialpad.tsx`:
- Around line 43-53: The FormTextInput in Dialpad.tsx currently passes
keyboardType='phone-pad' while also setting editable={false}, making
keyboardType a no-op; remove the keyboardType prop from the FormTextInput
invocation (inside the Dialpad component) to avoid misleading/unused
configuration and keep only props that affect behavior (i.e., retain
editable={false}, value={dialpadValue}, containerStyle, testID, multiline,
etc.).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/views/CallView/components/CallActionButton.tsxapp/views/CallView/components/CallButtons.tsxapp/views/CallView/components/Dialpad/Dialpad.tsxapp/views/CallView/components/Dialpad/DialpadButton.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/CallView/components/Dialpad/DialpadButton.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/CallView/components/Dialpad/Dialpad.tsx
🧬 Code graph analysis (2)
app/views/CallView/components/Dialpad/Dialpad.tsx (3)
app/theme.tsx (1)
useTheme(29-29)app/lib/services/voip/useCallStore.ts (1)
useDialpadValue(211-211)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
app/views/CallView/components/CallButtons.tsx (1)
app/containers/ActionSheet/Provider.tsx (1)
showActionSheetRef(80-82)
🔇 Additional comments (4)
app/views/CallView/components/CallActionButton.tsx (1)
55-58: LGTM — haptic feedback integration is clean.Fire-and-forget
impactAsyncis the correct pattern here; awaiting it would unnecessarily delay theonPresscallback. ThePressable'sdisabledprop prevents haptic triggers on disabled buttons.app/views/CallView/components/Dialpad/Dialpad.tsx (1)
54-62: Grid rendering looks correct.Keys are static, so
rowIndexas a key for rows anddigitas a key for buttons are both stable and unique — no issues here.app/views/CallView/components/CallButtons.tsx (2)
35-37: Dialpad integration anddisabled={isConnecting}look good.The action sheet approach is appropriate for an overlay dialpad, and disabling during connecting is now consistent with Hold, Mute, and Speaker.
Also applies to: 81-87
46-53: LGTM — Speaker now correctly disabled while connecting.Consistent with Hold and Mute behavior.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
10 tasks
10 tasks
3 tasks
diegolmello
added a commit
that referenced
this pull request
Apr 22, 2026
…/Decline (#7215) * merge feat.voip-lib * feat(voip): enhance call handling with UUID mapping and event listeners * Base call UI * feat(voip): integrate Zustand for call state management and enhance CallView UI * feat(voip): add simulateCall function for mock call handling in UI development * refactor(CallView): update button handlers and improve UI responsiveness * Add pause-shape-unfilled icon * Base CallHeader * toggleFocus * collapse buttons * Header components * Hide header when no call * Timer * Add use memo * Add voice call item on sidebar * cleanup * Temp use @rocket.chat/media-signaling from .tgz * cleanup * Check module and permissions to enable voip * Refactor stop method to use optional chaining for media signal listeners * voip push first test * Add VoIP call handling with pending call management - Implemented VoIP push notification handling in index.js, including storing call info for later processing. - Added CallKeep event handlers for answering and ending calls from a cold start. - Introduced a new CallIdUUID module to convert call IDs to deterministic UUIDs for compatibility with CallKit. - Created a pending call store to manage incoming calls when the app is not fully initialized. - Updated deep linking actions to include VoIP call handling. - Enhanced MediaSessionInstance to process pending calls and manage call states effectively. * Remove pending store and create getInitialEvents on app/index * Attempt to make iOS calls work from cold state * lint and format * Patch callkeep ios * Temp send iOS voip push token on gcm * Temp fix require cycle * chore: format code and fix lint issues [skip ci] * CallIDUUID module on android and voip push * Add setCallUUID on useCallStore to persist calls accepted on native Android * remove callkeep from notification * Android Incoming Call UI POC * Refactor VoIP handling: Migrate VoIP-related classes to a new package structure, removing deprecated modules and consolidating functionality. Update imports in MainApplication and NotificationIntentHandler to reflect changes. This cleanup enhances code organization and prepares for future VoIP feature enhancements. * Remove VoipForegroundService * cleanup and use caller instead of callerName * Cleanup and make iOS build again * Refactor VoIP handling: Remove unused event emissions for call answered and declined, switch from SharedPreferences to in-memory storage for pending VoIP call data, and update method signatures for better clarity. This cleanup enhances performance and prepares for future VoIP feature improvements. * Refactor VoIP handling: Introduce a new VoipPayload class to encapsulate call data, streamline notification processing, and enhance method signatures across the VoIP module. This update improves code clarity and prepares for future feature enhancements. * Migrate react-native-voip-push-notifications to VoipModule * Refactor VoIP module: Update package structure by moving VoipTurboPackage to the main package and removing the obsolete NativeVoipSpec class. Adjust imports in MainApplication and VoipModule to reflect these changes, enhancing code organization and maintainability. * Unify emitters * Move CallKeep listeners from MediaSessionInstance to getInitialEvents * Clear callkeep on endcall * Unify getInitialEvents logic * getInitialEvents -> MediaCallEvents * chore: format code and fix lint issues [skip ci] * feat(Android): Add full screen incoming call (#6977) * feat: Update call UI (#6990) * feat: Handle audio routing, e.g., Bluetooth headset vs. internal speaker switching (#6992) * fix: empty space when not on call (#6993) * feat: Dialpad (#7000) * action: organized translations * feat: start call (#7024) * chore: format code and fix lint issues * feat: Pre flight (#7038) * action: organized translations * feat: Receive voip push notifications from backend (#7045) * feat: Refactor media session handling and improve disconnect logic (#7065) * feat: Control incoming call from native (#7066) * feat: Voice message blocks (#7057) * feat: native accept success event (#7068) * feat(voip): call waiting, busy detection, and videoconf blocking (#7077) * action: organized translations * feat(voip): tap-to-hide call controls with animations (#7078) * feat(voip): navigate to call DM from message button and header (#7082) * feat(voip): tablet and landscape layout (#7110) * chore: develop into feat.voip-lib-new (RN 81 + Expo 54 + reanimated 4 + true-sheet + iOS 26) (#7114) * chore: format code and fix lint issues * feat(voip): android landscape layout for IncomingCallActivity (#7116) * Update agents files * feat(voip): Support a11y (#7106) * Fix content cutting on iOS on some edge cases * pods * Ignore .worktrees on jest * chore: Merge develop into feat.voip-lib-new (#7129) * fix(voip): show CallKit UI when call is active in background (#7128) * chore: Update media-signaling to 0.2.0 (#7153) * feat(voip): migrate iOS accept/reject from DDP to REST (#7124) * Fix icons * feat(voip): migrate Android accept/reject from DDP to REST (#7127) * test(voip): integration tests for CallView pipeline (#7161) * feat(voip): display video conf provider as subtitle (#7160) * fix(voip): CallView button grid and correct landscape/dialpad layouts (#7164) * fix(voip): prevent stale MMKV cache on Android first-install accept MMKVKeyManager.initialize ran in MainApplication.onCreate before the JS engine started and opened the default MMKV file via the Tencent 1.2 JAR when it was still empty. Tencent caches instances per-ID in a singleton registry, so that empty-state view was held for the rest of the process. JS later wrote credentials through react-native-mmkv (MMKV Core 2.0), which has its own separate registry. When a VoIP push arrived, Ejson.getMMKV() got the cached empty Tencent instance and reported "No userId found in MMKV for server". Closing and reopening the app cleared the cache, which is why only the very first call after install failed. Drop the open/verify block — the encryption key is already cached from SecureKeystore, so no MMKV handle is needed here. The first Tencent instance is now created inside Ejson.getMMKV() after JS has written, so it scans the file fresh. * fix(voip): prevent duplicate ringtone on Android incoming call (#7158) * fix(voip): set explicit snaps for NewMediaCall bottom sheet (#7165) * Update app/lib/services/voip/MediaSessionStore.ts Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com> * fix: make startVoipFork reactive to permissions-changed (#7151) * fix(android): remove MediaProjectionService from merged manifest (#7190) * fix(voip): Phone account creation (#7170) * feat: add Enable Mobile Ringing toggle in user preferences (#7155) * fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens (#7167) * fix(android): Play Store mic discoverability, safer FCM logs, avatar auth via headers (#7171) * fix(ios): serialize VoipService bridge statics (#7169) * fix(voip): Android DDP thread safety and VoipPayload bundle parity (#7168) * chore(voip): dead-code and hygiene sweep (#7174) * refactor(voip): decouple navigateToCallRoom from Redux and backfill REST/connect tests (#7176) * test(voip): tighten ringing endCall assertion and add VideoConf VoIP-lock saga coverage (#7177) * fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS (#7173) * refactor(voip): MediaCallEvents Redux adapters and resetVoipState (#7178) * refactor(voip): decouple peer autocomplete from Redux; simplify NewMediaCall (#7175) * fix(ios): add NS_SWIFT_NAME to Challenge.runChallenge for Swift 6.2 compatibility Swift 6.2 (Xcode 26.x / macos-26 runner) auto-renames the Objective-C method runChallenge:didReceiveChallenge:completionHandler: to run(_:didReceive:completionHandler:) when imported into Swift. Add NS_SWIFT_NAME to explicitly pin the Swift import name, preventing the compiler from applying its heuristics. This keeps the existing Swift call site in DDPClient.swift working without changes. * fix(ios): cancel old URLSession/webSocketTask before reconnecting in DDPClient.connect (#7197) * fix(ios): add NSLock to nativeAcceptHandledCallIds and 10s REST timeout to handleNativeAccept (#7198) * feat(android): create VoipCallService with FOREGROUND_SERVICE_MICROPHONE (#7199) * fix(android): start VoipCallService on accept, stop on hangup/timeout, install end-call listener (#7200) * fix(voip): enable DM nav for users with SIP extension (#7203) * fix(android): handle null VoiceConnection in answerIncomingCall, notify JS (#7201) * fix(voip): resolve closure capture ordering in handleNativeAccept (#7209) * fix(android): integrate VoIP modules with SSL-pinned OkHttpClient (#7208) * fix(push): gate id and voipToken behind server version checks, fix VideoConf caller extra (#7210) * fix(voip): remove sensitive data from production logs (#7207) * fix(android): remove isRunning guard + add double-tap guard on Accept/Decline - VoipCallService: remove if (!isRunning) guard, call startForeground unconditionally (idempotent on Android, fixes Android 14+ foreground service requirement) - IncomingCallActivity: add AtomicBoolean guard on handleAccept/handleDecline to prevent double-tap from triggering multiple service starts --------- Co-authored-by: diegolmello <diegolmello@users.noreply.github.com> Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
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
1 participant
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-16
How to test or reproduce
video callon sidebardialpadon the callScreenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Quality of Life
Tests / Storybook