-
Notifications
You must be signed in to change notification settings - Fork 13k
feat: Implement Call History contextual bar inside rooms #37773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughAdds a media call history contextual bar feature: new contextual-bar component with API fetch and error handling, internal and external renderers, internal actions hook, room-toolbox core route/provider integration, message-block action to open the history tab, query keys, and i18n keys. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageBlock as Message Block Hook
participant Toolbox as Room Toolbox
participant CoreRoutes as Core Room Routes
participant Contextualbar as MediaCallHistoryContextualbar
participant API as Server API
participant Internal as MediaCallHistoryInternal
participant External as MediaCallHistoryExternal
User->>MessageBlock: trigger 'open-history' (blockId)
MessageBlock->>Toolbox: openTab('media-call-history', blockId)
Toolbox->>CoreRoutes: resolve tab by id
CoreRoutes-->>Toolbox: return media-call-history route
Toolbox->>Contextualbar: render contextual bar (route context)
Contextualbar->>API: fetch call-history.info (async)
API-->>Contextualbar: return call data
alt internal call
Contextualbar->>Internal: render internal component
Internal->>Internal: compute contact & actions via hook
else external call
Contextualbar->>External: render external component
External->>External: compute contact & actions
end
Internal-->>User: show actions (voice, DM, jump, user info)
External-->>User: show actions (voice)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37773 +/- ##
===========================================
+ Coverage 67.72% 67.78% +0.05%
===========================================
Files 3464 3469 +5
Lines 113728 113811 +83
Branches 20908 20926 +18
===========================================
+ Hits 77024 77143 +119
+ Misses 34527 34483 -44
- Partials 2177 2185 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
baa16bd to
3479327
Compare
27bc69d to
b2c3873
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (8)
apps/meteor/client/uikit/hooks/useMessageBlockContextValue.ts (1)
58-62: Consider addingevent.preventDefault()for consistency.The
videoconf-coreblock above (line 47) callsevent.preventDefault()to prevent default browser behavior. For consistency and to avoid unintended side effects (such as form submission or link navigation), consider adding the same for themedia-call-coreblock.🔎 Apply this diff to add event.preventDefault():
if (appId === 'media-call-core') { + event.preventDefault(); if (actionId === 'open-history') { return openTab('media-call-history', blockId); } }packages/i18n/src/locales/en.i18n.json (1)
4170-4177: Optional: confirm reuse and punctuation for genericPlease_try_againThe generic
Please_try_againkey is fine, but two small things you might want to double‑check:
- Whether you really want a new generic key vs. reusing an existing “try again”/error string to reduce duplication.
- Whether this should end with a period or match nearby short prompts (many omit trailing punctuation) for consistency.
If usage looks good in the UI, you can leave as is.
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsx (3)
24-33: Consider usingenabledoption instead of throwing inside queryFn.Currently, the query throws an error when
contextis undefined, which triggers the error state. Using theenabledoption is more idiomatic for react-query and prevents unnecessary query attempts.🔎 Suggested improvement:
const { data, isError, isPending } = useQuery({ queryKey: ['call-history', context], queryFn: async () => { - if (!context) { - throw new Error('Call ID is required'); - } return getCallHistory({ callId: context } as any); // TODO fix this type }, staleTime: Infinity, // Call history should never change... + enabled: !!context, });
56-60: Consider graceful error handling instead of throwing.If
datadoesn't match internal or external call history types, the component throws an error that could crash the application. Consider falling back to the error UI instead.🔎 Suggested improvement:
if (isExternalCallHistoryItem(data)) { return <MediaCallHistoryExternal onClose={closeTab} data={data} />; } -throw new Error('Invalid call history item'); +// Fallback to error state for unknown data types +return ( + <ContextualbarDialog onClose={closeTab}> + <ContextualbarHeader> + <ContextualbarIcon name='info-circled' /> + <ContextualbarTitle>{t('Call_info')}</ContextualbarTitle> + <ContextualbarClose onClick={closeTab} /> + </ContextualbarHeader> + <ContextualbarEmptyContent icon='warning' title={t('Call_info_could_not_be_loaded')} subtitle={t('Please_try_again')} /> + </ContextualbarDialog> +);
30-30: Replaceas anywith proper discriminated union type for endpoint parameters.The
/v1/call-history.infoendpoint accepts a discriminated union:{ historyId: string; callId: never } | { callId: string; historyId: never }. Sincecontextis guaranteed to be a string at this point (line 27-29 guard), replace the cast withas { callId: string }to maintain type safety:return getCallHistory({ callId: context } as { callId: string });Alternatively, allow TypeScript to infer the type if the
useEndpointgeneric typing properly resolves the discriminated union through theEndpointsinterface.apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts (1)
1-1: Remove commented-out code.Commented-out type definition should be removed per coding guidelines.
-// type HistoryActions = 'voiceCall' | 'videoCall' | 'jumpToMessage' | 'directMessage' | 'userInfo'; -apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx (1)
36-46: Consider more granular memoization dependencies.Using
[data]as dependency means any change to the data object triggers recalculation. Consider using specific fields if the data object reference changes frequently but the relevant fields remain the same.🔎 Suggested improvement:
-const contact = useMemo(() => getContact(data.item, data.call), [data]); +const contact = useMemo(() => getContact(data.item, data.call), [data.item, data.call]); const historyData = useMemo(() => { return { callId: data.call._id, direction: data.item.direction, duration: data.item.duration, startedAt: new Date(data.item.ts), state: data.item.state, }; -}, [data]); +}, [data.call._id, data.item.direction, data.item.duration, data.item.ts, data.item.state]);apps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx (1)
29-29: Consider optimizing thecontactdependency array.The
contactmemo depends on[data]but only accessesdata.item. Using[data.item]would prevent unnecessary re-computation when other parts ofdatachange.🔎 View suggested change:
- const contact = useMemo(() => getContact(data.item), [data]); + const contact = useMemo(() => getContact(data.item), [data.item]);
📜 Review details
Configuration used: Organization 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 (8)
apps/meteor/client/uikit/hooks/useMessageBlockContextValue.ts(3 hunks)apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsx(1 hunks)apps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx(1 hunks)apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx(1 hunks)apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts(1 hunks)apps/meteor/client/views/room/providers/RoomToolboxProvider.tsx(3 hunks)apps/meteor/client/views/room/providers/hooks/useCoreRoomRoutes.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/uikit/hooks/useMessageBlockContextValue.tsapps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsxapps/meteor/client/views/room/providers/RoomToolboxProvider.tsxapps/meteor/client/views/room/providers/hooks/useCoreRoomRoutes.tsapps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tsapps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/client/uikit/hooks/useMessageBlockContextValue.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsxapps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsxapps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.tsapps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🧬 Code graph analysis (6)
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsx (2)
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx (1)
isInternalCallHistoryItem(17-22)apps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx (1)
isExternalCallHistoryItem(21-26)
apps/meteor/client/views/room/providers/RoomToolboxProvider.tsx (1)
apps/meteor/client/views/room/providers/hooks/useCoreRoomRoutes.ts (1)
useCoreRoomRoutes(17-19)
apps/meteor/client/views/room/providers/hooks/useCoreRoomRoutes.ts (1)
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsx (1)
MediaCallHistoryContextualbar(17-61)
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx (4)
packages/core-typings/src/ICallHistoryItem.ts (2)
IInternalMediaCallHistoryItem(37-43)CallHistoryItem(51-51)packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-68)apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts (1)
useMediaCallInternalHistoryActions(21-75)packages/ui-voip/src/index.ts (1)
CallHistoryContextualBar(7-7)
apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts (2)
packages/ui-contexts/src/index.ts (3)
useRouter(63-63)useCurrentRoutePath(32-32)useUserAvatarPath(84-84)packages/ui-contexts/src/RouterContext.ts (1)
LocationPathname(16-16)
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx (2)
packages/core-typings/src/ICallHistoryItem.ts (2)
IExternalMediaCallHistoryItem(45-49)CallHistoryItem(51-51)packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
IMediaCall(35-68)
⏰ 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: 🚢 Build Docker (arm64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
🔇 Additional comments (7)
apps/meteor/client/uikit/hooks/useMessageBlockContextValue.ts (1)
4-4: LGTM: Clean integration with room toolbox.The import and hook usage are correctly implemented to enable opening the media call history tab.
Also applies to: 42-42
packages/i18n/src/locales/en.i18n.json (1)
984-1000: Call info error string is well‑named and placed
Call_info_could_not_be_loadedfits the existing naming pattern in the Call_* block, the wording is clear, and the change does not touch the specialCall_*_boldformatting keys mentioned in prior i18n guidance. No changes needed here.apps/meteor/client/views/room/providers/hooks/useCoreRoomRoutes.ts (1)
5-11: LGTM!The route configuration is well-structured with appropriate id, title, component reference, icon, and group restrictions.
apps/meteor/client/views/room/providers/RoomToolboxProvider.tsx (2)
74-75: LGTM!Good clarifying comment explaining the distinction between core routes (no header button) and core actions.
91-102: LGTM!The tab resolution logic correctly prioritizes core routes before falling back to actions, and the memoization dependencies are properly updated.
apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts (1)
21-74: LGTM!The hook is well-structured with proper use of
useEffectEventfor stable callbacks, appropriate guards for optional values, and correct memoization of the returned actions object.apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx (1)
17-22: LGTM!The type guard correctly narrows the data type by checking for the
externalproperty and its boolean value.
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryExternal.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/mediaCallHistory/MediaCallHistoryContextualbar.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/mediaCallHistory/useMediaCallInternalHistoryActions.ts
Outdated
Show resolved
Hide resolved
…HistoryActions.ts Co-authored-by: Douglas Fabris <devfabris@gmail.com>
…37773) Co-authored-by: Douglas Fabris <devfabris@gmail.com>
Proposed changes (including videos or screenshots)
Issue(s)
VGA-75
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.