Add useLogicalGroupKey() so rows re-render when their logical group changes

.changelog/5966-fix-logical-group-key-hook.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- Fixed grouped messages showing avatars and breaking keyboard navigation in React Strict Mode, in PR [#5966](https://github.com/microsoft/BotFramework-WebChat/pull/5966), by [@OEvgeny](https://github.com/OEvgeny)

__tests__/html2/part-grouping/strictMode.html

@@ -0,0 +1,116 @@
+<!doctype html>
+<html lang="en-US">
+  <head>
+    <title>Part grouping: keeps the group after Strict Mode re-runs effects</title>
+    <link href="/assets/index.css" rel="stylesheet" type="text/css" />
+    <script type="importmap">
+      {
+        "imports": {
+          "@testduet/wait-for": "https://esm.sh/@testduet/wait-for",
+          "react": "https://esm.sh/react@18.3.1?dev",
+          "react-dom": "https://esm.sh/react-dom@18.3.1?dev",
+          "react-dom/client": "https://esm.sh/react-dom@18.3.1/client?dev"
+        }
+      }
+    </script>
+    <script crossorigin="anonymous" src="/test-harness.js"></script>
+    <script crossorigin="anonymous" src="/test-page-object.js"></script>
+    <script type="module">
+      // Strict Mode only re-runs effects with the development build of React 18 or later.
+      import React from 'react';
+      import ReactDOM from 'react-dom';
+
+      window.React = React;
+      window.ReactDOM = ReactDOM;
+    </script>
+    <script crossorigin="anonymous" defer src="/__dist__/webchat-es5.js"></script>
+  </head>
+  <body>
+    <main id="webchat"></main>
+    <script type="module">
+      import { waitFor } from '@testduet/wait-for';
+      import { createElement, StrictMode } from 'react';
+      import { createRoot } from 'react-dom/client';
+
+      run(async function () {
+        const {
+          WebChat: { ReactWebChat }
+        } = window; // Imports in UMD fashion.
+
+        const { directLine, store } = testHelpers.createDirectLineEmulator();
+        const root = createRoot(document.getElementById('webchat'));
+
+        const render = styleOptions =>
+          root.render(createElement(StrictMode, {}, createElement(ReactWebChat, { directLine, store, styleOptions })));
+
+        // GIVEN: Web Chat rendered in Strict Mode, which mounts, cleans up, and mounts every effect again.
+        render({ botAvatarInitials: 'B', hideSendBox: true });
+
+        await pageConditions.uiConnected();
+
+        const createActivity = (id, text) => ({
+          entities: [
+            {
+              '@context': 'https://schema.org',
+              '@id': '',
+              '@type': 'Message',
+              isPartOf: { '@id': '_:group-1', '@type': 'HowTo' },
+              type: 'https://schema.org/Message'
+            }
+          ],
+          from: { role: 'bot' },
+          id,
+          text,
+          type: 'message'
+        });
+
+        const activeDescendant = () =>
+          document.getElementById(pageElements.transcript().getAttribute('aria-activedescendant'));
+        const avatarInitials = () =>
+          Array.from(document.querySelectorAll('.webchat__initialsAvatar__initials'), ({ textContent }) => textContent);
+
+        // WHEN: 2 messages of the same group arrive.
+        directLine.emulateIncomingActivity(createActivity('a-00001', 'Part 1 of the group.'));
+        directLine.emulateIncomingActivity(createActivity('a-00002', 'Part 2 of the group.'));
+
+        await pageConditions.numActivitiesShown(2);
+
+        // WHEN: Web Chat re-renders the messages without changing the group.
+        render({ botAvatarInitials: 'R', hideSendBox: true });
+
+        // THEN: Only the group shows an avatar, the messages inside it do not.
+        await waitFor(() => expect(avatarInitials()).toEqual(['R']));
+        await pageConditions.scrollStabilized();
+
+        await host.snapshot('local');
+
+        // WHEN: The transcript is focused.
+        pageObjects.focusTranscript();
+
+        // THEN: The last message of the group is focused.
+        await waitFor(() => expect(pageElements.focusedActivity()?.textContent).toContain('Part 2 of the group.'));
+
+        await host.snapshot('local');
+
+        // WHEN: Pressing ARROW_UP.
+        await host.sendKeys('ARROW_UP');
+
+        // THEN: The first message of the group is focused.
+        await waitFor(() => expect(pageElements.focusedActivity()?.textContent).toContain('Part 1 of the group.'));
+
+        await host.snapshot('local');
+
+        // WHEN: Pressing ARROW_UP again.
+        await host.sendKeys('ARROW_UP');
+
+        // THEN: The group is focused, not a message in it.
+        await waitFor(() => {
+          expect(activeDescendant()?.closest('.part-grouping-activity')).toBeTruthy();
+          expect(pageElements.focusedActivity()).toBeUndefined();
+        });
+
+        await host.snapshot('local');
+      });
+    </script>
+  </body>
+</html>

packages/component/src/Activity/StackedLayout.tsx

@@ -21,7 +21,7 @@ import getFirstBaseOfSoftwareSourceCode from '../Utils/orgSchema/getFirstBaseOfS
 import textFormatToContentType from '../Utils/textFormatToContentType';
 import useLocalizeActivityGreetingAlt from '../hooks/internal/useLocalizeActivityGreetingAlt.js';
 import useLocalizeProgrammingLanguage from '../hooks/internal/useLocalizeProgrammingLanguage';
-import { useGetLogicalGroupKey } from '../providers/ActivityLogicalGrouping';
+import { useLogicalGroupKey } from '../providers/ActivityLogicalGrouping';
 import AttachmentRow from './AttachmentRow';
 import Bubble from './Bubble';
 import CodeBlockContent from './CodeBlockContent';
@@ -144,8 +144,8 @@ const StackedLayout = ({
   const nubSize = fromUser ? bubbleFromUserNubSize : bubbleNubSize;
   const otherInitials = fromUser ? botInitials : userInitials;
   const otherNubSize = fromUser ? bubbleNubSize : bubbleFromUserNubSize;
-  const activityKey = useGetKeyByActivity()(activity);
-  const isInGroup = !!useGetLogicalGroupKey()(activityKey);
+  const [logicalGroupKey] = useLogicalGroupKey(useGetKeyByActivity()(activity));
+  const isInGroup = !!logicalGroupKey;
 
   const hasAvatar = (initials || typeof initials === 'string') && !isInGroup;
   const hasOtherAvatar = otherInitials || typeof otherInitials === 'string';

packages/component/src/Middleware/ActivityGrouping/ui/PartGrouping/private/PartGroupingActivity.tsx

@@ -13,7 +13,7 @@ import StackedLayoutStatus from '../../../../../Activity/StackedLayoutStatus';
 import useActivityElementMapRef from '../../../../../providers/ChatHistoryDOM/useActivityElementRef';
 import useActiveDescendantId from '../../../../../providers/TranscriptFocus/useActiveDescendantId';
 import useFocusByGroupKey from '../../../../../providers/TranscriptFocus/useFocusByGroupKey';
-import useGetGroupDescendantIdByActivityKey from '../../../../../providers/TranscriptFocus/useGetGroupDescendantIdByActivityKey';
+import useGroupDescendantIdByGroupKey from '../../../../../providers/TranscriptFocus/useGroupDescendantIdByGroupKey';
 import FocusTrap from '../../../../../Transcript/FocusTrap';
 import useRenderActivityProps from '../../../../../Transcript/hooks/useRenderActivityProps';
 import {
@@ -50,7 +50,6 @@ type PartGroupingActivityProps = InferOutput<typeof partGroupingActivityPropsSch
 
 const partGroupingFocusableActivityPropsSchema = pipe(
   object({
-    activity: custom<WebChatActivity>(value => is(object({}), value)),
     children: optional(reactNode()),
     className: optional(string()),
     groupKey: string()
@@ -64,15 +63,12 @@ type FocusablePartGroupingActivityProps = InferOutput<typeof partGroupingFocusab
 const FocusablePartGroupingActivity = memo(function FocusablePartGroupingActivity(
   props: FocusablePartGroupingActivityProps
 ) {
-  const { activity, children, className, groupKey } = validateProps(partGroupingFocusableActivityPropsSchema, props);
+  const { children, className, groupKey } = validateProps(partGroupingFocusableActivityPropsSchema, props);
 
   const [activeDescendantId] = useActiveDescendantId();
 
-  const getGroupDescendantIdByActivityKey = useGetGroupDescendantIdByActivityKey();
   const focusByGroupKey = useFocusByGroupKey();
-
-  const getKeyByActivity = useGetKeyByActivity();
-  const groupingActivityDescendantId = getGroupDescendantIdByActivityKey(getKeyByActivity(activity));
+  const [groupingActivityDescendantId] = useGroupDescendantIdByGroupKey(groupKey);
 
   const focusSelf = useCallback<(withFocus?: boolean) => void>(
     (withFocus?: boolean) => focusByGroupKey(groupKey, withFocus),
@@ -220,11 +216,7 @@ function PartGroupingActivity(props: PartGroupingActivityProps) {
   );
 
   return (
-    <FocusablePartGroupingActivity
-      activity={firstActivity}
-      className={classNames['part-grouping-activity']}
-      groupKey={groupKey}
-    >
+    <FocusablePartGroupingActivity className={classNames['part-grouping-activity']} groupKey={groupKey}>
       <StackedLayoutRoot
         hideAvatar={hasAvatar && !showAvatar}
         isGroup={true}

packages/component/src/Middleware/ActivityGrouping/ui/PartGrouping/private/usePartGroupingLogicalGroup.ts

@@ -1,9 +1,7 @@
 import random from 'math-random';
-import { useCallback, useEffect, useMemo } from 'react';
-import { useRefFrom } from 'use-ref-from';
-import { useStateWithRef } from 'use-state-with-ref';
+import { useLayoutEffect, useMemo } from 'react';
 
-import { useAddLogicalGrouping, useRemoveLogicalGrouping } from '../../../../../providers/ActivityLogicalGrouping';
+import { useRemoveLogicalGrouping, useSetLogicalGrouping } from '../../../../../providers/ActivityLogicalGrouping';
 
 type UsePartGroupingLogicalGroupOptions = {
   activityKeys: readonly string[];
@@ -12,27 +10,18 @@ type UsePartGroupingLogicalGroupOptions = {
 
 function usePartGroupingLogicalGroup({ activityKeys, isCollapsed }: UsePartGroupingLogicalGroupOptions): string {
   // eslint-disable-next-line no-magic-numbers
-  const [partGroupKey, _setPartKey, partKeyRef] = useStateWithRef(() => `part-${random().toString(36).slice(2, 11)}`);
-  const addLogicalGrouping = useAddLogicalGrouping();
+  const partGroupKey = useMemo(() => `part-${random().toString(36).slice(2, 11)}`, []);
   const removeLogicalGrouping = useRemoveLogicalGrouping();
+  const setLogicalGrouping = useSetLogicalGrouping();
 
-  const isCollapsedRef = useRefFrom(isCollapsed);
+  // In layout effects, not while rendering: setting re-renders the activities before paint.
+  // Strict Mode runs the cleanup below, then this effect again.
+  // Removed in its own effect, so a change of `activityKeys` updates the group instead of removing and adding it.
+  useLayoutEffect(() => {
+    setLogicalGrouping(partGroupKey, { activityKeys: Object.freeze(Array.from(activityKeys)), isCollapsed });
+  }, [activityKeys, isCollapsed, partGroupKey, setLogicalGrouping]);
 
-  const getGroupState = useCallback(() => ({ isCollapsed: isCollapsedRef.current }), [isCollapsedRef]);
-
-  useMemo(
-    () =>
-      addLogicalGrouping({
-        key: partKeyRef.current,
-        activityKeys: Array.from(activityKeys),
-        getGroupState
-      }),
-    [activityKeys, addLogicalGrouping, getGroupState, partKeyRef]
-  );
-
-  const removeLogicalGroupingRef = useRefFrom(removeLogicalGrouping);
-
-  useEffect(() => () => removeLogicalGroupingRef.current(partKeyRef.current), [partKeyRef, removeLogicalGroupingRef]);
+  useLayoutEffect(() => () => removeLogicalGrouping(partGroupKey), [partGroupKey, removeLogicalGrouping]);
 
   return partGroupKey;
 }

packages/component/src/hooks/internal/createKeyedStore.ts

@@ -0,0 +1,38 @@
+type ReadonlyKeyedStore<T> = {
+  readonly get: (key: string) => T | undefined;
+  /** Calls the listener after the value of the key changed, until unsubscribed. */
+  readonly subscribe: (key: string, listener: () => void) => () => void;
+};
+
+type KeyedStore<T> = ReadonlyKeyedStore<T> & {
+  readonly delete: (key: string) => void;
+  /** Notifies the listeners of the key if the value changed. Listeners may re-render: call it after commit, e.g. from an effect. */
+  readonly set: (key: string, value: T | undefined) => void;
+};
+
+/** Creates a store of values by key, which notifies only the listeners of keys whose value changed. */
+export default function createKeyedStore<T>(): KeyedStore<T> {
+  const eventTarget = new EventTarget();
+  const values = new Map<string, T>();
+
+  const set = (key: string, value: T | undefined): void => {
+    if (!Object.is(values.get(key), value)) {
+      typeof value === 'undefined' ? values.delete(key) : values.set(key, value);
+
+      eventTarget.dispatchEvent(new Event(key));
+    }
+  };
+
+  return Object.freeze({
+    delete: (key: string) => set(key, undefined),
+    get: (key: string) => values.get(key),
+    set,
+    subscribe: (key: string, listener: () => void) => {
+      eventTarget.addEventListener(key, listener);
+
+      return () => eventTarget.removeEventListener(key, listener);
+    }
+  });
+}
+
+export { type KeyedStore, type ReadonlyKeyedStore };