regression: Missing call button with "Enhanced Navigation" enabled on small screens #37155
Conversation
|
|
Looks like this PR is ready to merge! 🎉 |
WalkthroughReplaces prior VoIP multi-item handling with a single optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Data as NavBarControlsWithData
participant Hook as useMediaCallAction()
participant WithCall as NavBarControlsWithCall
participant Menu as NavBarControlsMenu
participant Generic as GenericMenu
User->>Data: open nav controls
Data->>Hook: request callAction
Hook-->>Data: callAction (or null)
alt callAction present
Data->>Data: build callItem
else
Data->>Data: callItem = undefined
end
Data->>WithCall: render (props..., callItem?)
Data->>Menu: render (omnichannelItems, isPressed, callItem?)
Menu->>Menu: sections = [Voice_Call(callItem?), Omnichannel(...)]
Menu->>Generic: render filtered sections
Generic-->>User: menu (with or without Voice_Call)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithCall.tsx (1)
11-11: Consider explicit forwarding ofcallItemfor clarity.While the
callItemprop is correctly passed through the...propsspread at line 47, explicitly destructuring and forwarding it would improve code readability and make the data flow more obvious.Apply this diff to make the forwarding explicit:
-const NavBarControlsWithCall = ({ omnichannelItems, isPressed, ...props }: NavBarControlsMenuProps) => { +const NavBarControlsWithCall = ({ omnichannelItems, isPressed, callItem, ...props }: NavBarControlsMenuProps) => {- return <NavBarControlsMenu omnichannelItems={omnichannelItemsWithCall} isPressed={isPressed} {...props} />; + return <NavBarControlsMenu omnichannelItems={omnichannelItemsWithCall} isPressed={isPressed} callItem={callItem} {...props} />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsMenu.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithCall.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithData.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithData.tsx (1)
packages/ui-voip/src/index.ts (1)
useMediaCallAction(5-5)
⏰ 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). (6)
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsMenu.tsx (1)
12-12: LGTM! Clean integration of the Voice_Call section.The
callItemprop is properly integrated:
- Added to the type definition
- Conditionally rendered in a Voice_Call section
- Filtered out when empty alongside the Omnichannel section
The implementation follows the existing pattern and maintains consistency with the Omnichannel section handling.
Also applies to: 15-15, 20-23
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithData.tsx (3)
73-73: LGTM! Correct propagation ofcallItemto both render paths.The
callItemis properly passed to bothNavbarControlsWithCall(when call is enabled) andNavBarControlsMenu(otherwise), ensuring the call button appears in both navigation modes.Also applies to: 76-76
2-2: VerifyuseMediaCallActioncovers small-screen logic.
Confirm that the hook from@rocket.chat/ui-voipinternally handles screen-size and “Enhanced Navigation” mode, hiding the call button when appropriate.
40-47: Verify null safety ofcallActionproperties
Confirm that whenuseMediaCallActionreturns a non-nullcallAction, itsicon,title, andactionare always defined; otherwise add defensive checks (optional chaining or default values).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37155 +/- ##
=================================================
Coverage ? 66.41%
=================================================
Files ? 3384
Lines ? 115592
Branches ? 21348
=================================================
Hits ? 76767
Misses ? 36219
Partials ? 2606
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
If this menu item hasn't been displayed in 7.11.0, we should treat as a regression IMO |
dougfabris
force-pushed
the
fix/missingCallItem
branch
from
14a8968 to
86df46a
Compare
dougfabris
changed the title
dougfabris
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithCall.tsx (2)
8-12: Unify prop type source to avoid drift.You re-declare NavBarControlsMenuProps here (now with callItem). Prefer importing the actual prop type from NavBarControlsMenu (or exporting a shared type) to prevent divergence across files.
14-14: Call item forwarding via spread is correct; minor clarity tweak optional.callItem is forwarded via
{...props}to NavBarControlsMenu. Works. For readability, you could destructurecallItemexplicitly and pass it as a named prop.Also applies to: 47-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsMenu.tsx(1 hunks)apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithCall.tsx(2 hunks)apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithData.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsMenu.tsx
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/NavBarV2/NavBarControls/NavBarControlsWithData.tsx (4)
70-70: Confirm pressed-state scope is intentional.isPressed now ignores VOIP-specific pressed states. Confirm that only queue/contact need to drive the popover state in this view.
73-73: All verification checks confirm the code changes are correct.The
callItemis properly passed to bothNavbarControlsWithCall(line 73) andNavBarControlsMenu(line 76), andNavBarControlsMenucorrectly consumes it to render the Voice_Call section at line 22 (items: callItem ? [callItem] : []). The fix ensures the call entry is available on both desktop and mobile views.
2-2: @rocket.chat/ui-voip hook integration validated
useMediaCallAction is properly exported and used across the monorepo; no voipItems references remain.
40-47: No action required;useMediaCallActiondoes not expose a disabled state.The hook returns only
{ title: string; icon: IconNames; action: (callee?: PeerInfo) => void }across all code paths. There is nodisabledorisDisabledflag to propagate.
Proposed changes (including videos or screenshots)
Issue(s)
VGA-20
Steps to test or reproduce
Further comments
Summary by CodeRabbit