Skip to content

Latency improved for activity keyer and tree composer #5465

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ Notes: web developers are advised to use [`~` (tilde range)](https://github.com/
- [`[email protected]`](https://npmjs.com/package/webpack-cli/)
- [`[email protected]`](https://npmjs.com/package/webpack/)
- Fixed [#5446](https://github.com/microsoft/BotFramework-WebChat/issues/5446). Embedded `uuid` so `microsoft-cognitiveservices-speech-sdk` do not need to use dynamic loading, as this could fail in Webpack 4 environment, in PR [#5445](https://github.com/microsoft/BotFramework-WebChat/pull/5445), by [@compulim](https://github.com/compulim)
- Improved latency of `ActivityKeyerComposer` and `ActivityTreeComposer` in PR [#5465](https://github.com/microsoft/BotFramework-WebChat/pull/5465) by [@pranavjoshi001](https://github.com/pranavjoshi001)

### Fixed

Expand Down Expand Up @@ -216,6 +217,7 @@ Notes: web developers are advised to use [`~` (tilde range)](https://github.com/
# Removed

- Deprecating `disabled` props and `useDisabled` hook in favor of new `uiState` props and `useUIState` hook, in PR [#5276](https://github.com/microsoft/BotFramework-WebChat/pull/5276), by [@compulim](https://github.com/compulim)
- Deleted `ActivitListenerComposer` and `useUpsertedActivities` hook in PR [#5465](https://github.com/microsoft/BotFramework-WebChat/pull/5465) by [@pranavjoshi001](https://github.com/pranavjoshi001)

## [4.18.0] - 2024-07-10

Expand Down
23 changes: 10 additions & 13 deletions packages/api/src/hooks/Composer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import normalizeStyleOptions from '../normalizeStyleOptions';
import patchStyleOptionsFromDeprecatedProps from '../patchStyleOptionsFromDeprecatedProps';
import ActivityAcknowledgementComposer from '../providers/ActivityAcknowledgement/ActivityAcknowledgementComposer';
import ActivityKeyerComposer from '../providers/ActivityKeyer/ActivityKeyerComposer';
import ActivityListenerComposer from '../providers/ActivityListener/ActivityListenerComposer';
import ActivitySendStatusComposer from '../providers/ActivitySendStatus/ActivitySendStatusComposer';
import ActivitySendStatusTelemetryComposer from '../providers/ActivitySendStatusTelemetry/ActivitySendStatusTelemetryComposer';
import ActivityTypingComposer from '../providers/ActivityTyping/ActivityTypingComposer';
Expand Down Expand Up @@ -625,18 +624,16 @@ const ComposerCore = ({

return (
<WebChatAPIContext.Provider value={context}>
<ActivityListenerComposer>
<ActivitySendStatusComposer>
<ActivityTypingComposer>
<SendBoxMiddlewareProvider middleware={sendBoxMiddleware || EMPTY_ARRAY}>
<SendBoxToolbarMiddlewareProvider middleware={sendBoxToolbarMiddleware || EMPTY_ARRAY}>
{typeof children === 'function' ? children(context) : children}
<ActivitySendStatusTelemetryComposer />
</SendBoxToolbarMiddlewareProvider>
</SendBoxMiddlewareProvider>
</ActivityTypingComposer>
</ActivitySendStatusComposer>
</ActivityListenerComposer>
<ActivitySendStatusComposer>
<ActivityTypingComposer>
<SendBoxMiddlewareProvider middleware={sendBoxMiddleware || EMPTY_ARRAY}>
<SendBoxToolbarMiddlewareProvider middleware={sendBoxToolbarMiddleware || EMPTY_ARRAY}>
{typeof children === 'function' ? children(context) : children}
<ActivitySendStatusTelemetryComposer />
</SendBoxToolbarMiddlewareProvider>
</SendBoxMiddlewareProvider>
</ActivityTypingComposer>
</ActivitySendStatusComposer>
{onTelemetry && <Tracker />}
</WebChatAPIContext.Provider>
);
Expand Down
4 changes: 3 additions & 1 deletion packages/api/src/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import useUIState from './useUIState';
import useUserID from './useUserID';
import useUsername from './useUsername';
import useVoiceSelector from './useVoiceSelector';
import useReduceActivities from './useReduceActivities';

export {
useActiveTyping,
Expand Down Expand Up @@ -143,5 +144,6 @@ export {
useUIState,
useUserID,
useUsername,
useVoiceSelector
useVoiceSelector,
useReduceActivities
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort.

};
3 changes: 3 additions & 0 deletions packages/api/src/hooks/useReduceActivities.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import useReduceActivities from '../providers/ActivityTyping/private/useReduceActivities';

export default useReduceActivities;
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type KeyToActivitiesMap = Map<string, readonly WebChatActivity[]>;
*
* Local key are only persisted in memory. On refresh, they will be a new random key.
*/
// eslint-disable-next-line complexity
const ActivityKeyerComposer = ({ children }: Readonly<{ children?: ReactNode | undefined }>) => {
const existingContext = useActivityKeyerContext(false);

Expand All @@ -43,53 +44,36 @@ const ActivityKeyerComposer = ({ children }: Readonly<{ children?: ReactNode | u
const activityToKeyMapRef = useRef<Readonly<ActivityToKeyMap>>(Object.freeze(new Map()));
const clientActivityIdToKeyMapRef = useRef<Readonly<ClientActivityIdToKeyMap>>(Object.freeze(new Map()));
const keyToActivitiesMapRef = useRef<Readonly<KeyToActivitiesMap>>(Object.freeze(new Map()));
const lastProcessedIndexRef = useRef(0);

// TODO: [P1] `useMemoWithPrevious` to check and cache the resulting array if it hasn't changed.
const activityKeysState = useMemo<readonly [readonly string[]]>(() => {
const { current: activityIdToKeyMap } = activityIdToKeyMapRef;
const { current: activityToKeyMap } = activityToKeyMapRef;
const { current: clientActivityIdToKeyMap } = clientActivityIdToKeyMapRef;
const nextActivityIdToKeyMap: ActivityIdToKeyMap = new Map();
const nextActivityKeys: Set<string> = new Set();
const nextActivityToKeyMap: ActivityToKeyMap = new Map();
const nextClientActivityIdToKeyMap: ClientActivityIdToKeyMap = new Map();
const nextKeyToActivitiesMap: KeyToActivitiesMap = new Map();

activities.forEach(activity => {
// Process new activities (if any)
if (activities.length > lastProcessedIndexRef.current) {
Copy link
Contributor

@compulim compulim Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The activities array can change at any given position. Says, we have 3 activities [a1, b1, c1].

It could be updated and become [a2, b1, c1] and the lastProcessedIndexRef would missed that change.

Can we use useReduceActivities here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried with useReduceActivities but then I was not aware that activities can change at any given position in that case updated activity will come in prevActivities in hook's argument and we need to process it again to fill our ref maps, that would nothing but iteration through all activities like before - is my understanding correct ?

for (let i = lastProcessedIndexRef.current; i < activities.length; i++) {
// eslint-disable-next-line security/detect-object-injection
const activity = activities[i];
const activityId = getActivityId(activity);
if (!activityId) {
break;
}
const clientActivityId = getClientActivityId(activity);
const typingActivityId = getActivityLivestreamingMetadata(activity)?.sessionId;

const key =
(clientActivityId &&
(clientActivityIdToKeyMap.get(clientActivityId) || nextClientActivityIdToKeyMap.get(clientActivityId))) ||
(typingActivityId &&
(activityIdToKeyMap.get(typingActivityId) || nextActivityIdToKeyMap.get(typingActivityId))) ||
(activityId && (activityIdToKeyMap.get(activityId) || nextActivityIdToKeyMap.get(activityId))) ||
activityToKeyMap.get(activity) ||
nextActivityToKeyMap.get(activity) ||
(clientActivityId && clientActivityIdToKeyMapRef.current.get(clientActivityId)) ||
(typingActivityId && activityIdToKeyMapRef.current.get(typingActivityId)) ||
(activityId && activityIdToKeyMapRef.current.get(activityId)) ||
activityToKeyMapRef.current.get(activity) ||
uniqueId();

activityId && nextActivityIdToKeyMap.set(activityId, key);
clientActivityId && nextClientActivityIdToKeyMap.set(clientActivityId, key);
nextActivityToKeyMap.set(activity, key);
nextActivityKeys.add(key);

const activities = nextKeyToActivitiesMap.has(key) ? [...nextKeyToActivitiesMap.get(key)] : [];

activities.push(activity);
nextKeyToActivitiesMap.set(key, Object.freeze(activities));
});

activityIdToKeyMapRef.current = Object.freeze(nextActivityIdToKeyMap);
activityToKeyMapRef.current = Object.freeze(nextActivityToKeyMap);
clientActivityIdToKeyMapRef.current = Object.freeze(nextClientActivityIdToKeyMap);
keyToActivitiesMapRef.current = Object.freeze(nextKeyToActivitiesMap);

// `nextActivityKeys` could potentially same as `prevActivityKeys` despite reference differences, we should memoize it.
return Object.freeze([Object.freeze([...nextActivityKeys.values()])]) as readonly [readonly string[]];
}, [activities, activityIdToKeyMapRef, activityToKeyMapRef, clientActivityIdToKeyMapRef, keyToActivitiesMapRef]);
activityId && activityIdToKeyMapRef.current.set(activityId, key);
clientActivityId && clientActivityIdToKeyMapRef.current.set(clientActivityId, key);

activityToKeyMapRef.current.set(activity, key);
Copy link
Contributor

@compulim compulim Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why this would work.

In the new code, activityToKeyMapRef is always frozen and setting to it should throw (in strict mode) or no-op (in non-strict mode).

const existingList = keyToActivitiesMapRef.current.get(key) ?? [];
keyToActivitiesMapRef.current.set(key, Object.freeze([...existingList, activity]));
lastProcessedIndexRef.current = activities.length;
}
}
const getActivitiesByKey: (key?: string | undefined) => readonly WebChatActivity[] | undefined = useCallback(
(key?: string | undefined): readonly WebChatActivity[] | undefined => key && keyToActivitiesMapRef.current.get(key),
[keyToActivitiesMapRef]
Expand All @@ -110,9 +94,18 @@ const ActivityKeyerComposer = ({ children }: Readonly<{ children?: ReactNode | u
[activityIdToKeyMapRef]
);

// eslint-disable-next-line react-hooks/exhaustive-deps
const activityKeys = useMemo(() => Object.freeze([...keyToActivitiesMapRef.current.keys()]), [activities.length]); // we want to update the keys when activities change

const contextValue = useMemo<ActivityKeyerContextType>(
() => ({ activityKeysState, getActivityByKey, getActivitiesByKey, getKeyByActivity, getKeyByActivityId }),
[activityKeysState, getActivitiesByKey, getActivityByKey, getKeyByActivity, getKeyByActivityId]
() => ({
activityKeysState: [activityKeys],
getActivityByKey,
getActivitiesByKey,
getKeyByActivity,
getKeyByActivityId
}),
[activityKeys, getActivitiesByKey, getActivityByKey, getKeyByActivity, getKeyByActivityId]
);

const { length: numActivities } = activities;
Expand Down Expand Up @@ -149,7 +142,7 @@ const ActivityKeyerComposer = ({ children }: Readonly<{ children?: ReactNode | u
);
}

if (activityKeysState[0].length !== keyToActivitiesMapRef.current.size) {
if (activityKeys.length !== keyToActivitiesMapRef.current.size) {
console.warn(
'botframework-webchat internal assertion: "activityKeys.length" should be same as "keyToActivitiesMap.size".'
);
Expand Down

This file was deleted.

16 changes: 0 additions & 16 deletions packages/api/src/providers/ActivityListener/private/Context.tsx

This file was deleted.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { hooks, type ActivityComponentFactory } from 'botframework-webchat-api';
import type { WebChatActivity } from 'botframework-webchat-core';
import React, { useMemo, type ReactNode } from 'react';
import React, { useMemo, useRef, type ReactNode } from 'react';

import useMemoWithPrevious from '../../hooks/internal/useMemoWithPrevious';
import ActivityTreeContext from './private/Context';
Expand All @@ -13,7 +13,14 @@ import type { ActivityTreeContextType } from './private/Context';

type ActivityTreeComposerProps = Readonly<{ children?: ReactNode | undefined }>;

const { useActivities, useActivityKeys, useCreateActivityRenderer, useGetActivitiesByKey, useGetKeyByActivity } = hooks;
const {
useActivities,
useActivityKeys,
useCreateActivityRenderer,
useGetActivitiesByKey,
useGetKeyByActivity,
useReduceActivities
} = hooks;

const ActivityTreeComposer = ({ children }: ActivityTreeComposerProps) => {
const existingContext = useActivityTreeContext(false);
Expand All @@ -27,26 +34,35 @@ const ActivityTreeComposer = ({ children }: ActivityTreeComposerProps) => {
const getKeyByActivity = useGetKeyByActivity();
const activityKeys = useActivityKeys();

const activities = useMemo<readonly WebChatActivity[]>(() => {
const activities: WebChatActivity[] = [];
// Persistent Map to store activities by their keys
const activityMapRef = useRef<Readonly<Map<string, WebChatActivity>>>(
Object.freeze(new Map<string, WebChatActivity>())
);

if (!activityKeys) {
return rawActivities;
}
const activities =
useReduceActivities<readonly WebChatActivity[]>((prevActivities = [], activity) => {
if (!activityKeys) {
return rawActivities;
}

for (const activity of rawActivities) {
// If an activity has multiple revisions, display the latest revision only at the position of the first revision.
const activityKey = getKeyByActivity(activity);

// "Activities with same key" means "multiple revisions of same activity."
const activitiesWithSameKey = getActivitiesByKey(getKeyByActivity(activity));
const activitiesWithSameKey = getActivitiesByKey(activityKey);

// TODO: We may want to send all revisions of activity to the middleware so they can render UI to see previous revisions.
activitiesWithSameKey?.[0] === activity &&
activities.push(activitiesWithSameKey[activitiesWithSameKey.length - 1]);
}
if (activitiesWithSameKey?.[activitiesWithSameKey.length - 1] === activity) {
const activityMap = activityMapRef.current;

// Update or add the activity in the persistent Map
activityMap.set(activityKey, activity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the activity is removed from the rawActivities, it will still keep in the activityMapRef forever and this will cause memory leak.


// Return the updated activities as an array
return Array.from(activityMap.values());
}

return Object.freeze(activities);
}, [activityKeys, getActivitiesByKey, getKeyByActivity, rawActivities]);
return prevActivities;
}) || [];

const createActivityRenderer: ActivityComponentFactory = useCreateActivityRenderer();

Expand Down
Loading