Stop updating other components while rendering

.changelog/5958-fix-render-phase-updates.md

@@ -0,0 +1,3 @@
+### Fixed
+
+- Fixed state updates while rendering that React development builds logged as "Cannot update a component while rendering a different component", from the toaster, the typing indicator, the "New messages" button, and part grouping, in PR [#5958](https://github.com/microsoft/BotFramework-WebChat/pull/5958), by [@OEvgeny](https://github.com/OEvgeny)

__tests__/html2/activityOrdering/partGroupingShouldUseMaxTimestamp.html

@@ -24,10 +24,8 @@
       import { version } from 'react';
 
       const IGNORING_KEYWORDS = [
-        'Cannot update a component',
         'Support for defaultProps',
         'ReactDOM.render is no longer supported',
-        'To locate the bad setState()',
         'has been deprecated, use `import {'
       ];
 

__tests__/html2/scrollToEndButton/scrollToEndButton.noUpdateWhileRendering.html

@@ -0,0 +1,95 @@
+<!doctype html>
+<html lang="en-US">
+  <head>
+    <link href="/assets/index.css" rel="stylesheet" type="text/css" />
+  </head>
+  <body>
+    <main id="webchat"></main>
+    <script type="importmap">
+      {
+        "imports": {
+          "@testduet/wait-for": "https://esm.sh/@testduet/wait-for",
+          "botframework-webchat": "/__dist__/packages/bundle/static/botframework-webchat.js",
+          "botframework-webchat/": "/__dist__/packages/bundle/static/botframework-webchat/",
+          "jest-mock": "https://esm.sh/jest-mock",
+          "react": "/__dist__/packages/bundle/static/react.js",
+          "react-dom": "/__dist__/packages/bundle/static/react-dom.js",
+          "react-dom/client": "/__dist__/packages/bundle/static/react-dom/client.js"
+        }
+      }
+    </script>
+    <script type="module">
+      import '/test-harness.mjs';
+      import '/test-page-object.mjs';
+
+      import { waitFor } from '@testduet/wait-for';
+      import { createStoreWithOptions, ReactWebChat } from 'botframework-webchat';
+      import { spyOn } from 'jest-mock';
+      import { createElement } from 'react';
+      import { createRoot } from 'react-dom/client';
+
+      // TODO: This is for `createDirectLineEmulator` only, should find ways to eliminate this line.
+      window.WebChat = { createStoreWithOptions };
+
+      run(async function () {
+        // GIVEN: The development build of React, which logs "Cannot update a component while rendering a different component" to console.error.
+        const consoleErrorSpy = spyOn(console, 'error');
+        const { directLine, store } = testHelpers.createDirectLineEmulator();
+
+        // GIVEN: Web Chat keeps the live region copy, so it can be checked.
+        createRoot(document.getElementById('webchat')).render(
+          createElement(ReactWebChat, {
+            directLine,
+            store,
+            styleOptions: { hideSendBox: true, internalLiveRegionFadeAfter: 60000 }
+          })
+        );
+
+        await pageConditions.uiConnected();
+
+        // WHEN: 5 messages arrive while the transcript is at the end.
+        for (let index = 1; index <= 5; index++) {
+          // eslint-disable-next-line no-await-in-loop
+          await directLine.emulateIncomingActivity(`Message ${index}.\n\n${'Lorem ipsum dolor sit amet. '.repeat(10)}`);
+        }
+
+        await pageConditions.numActivitiesShown(5);
+        await pageConditions.scrollToBottomCompleted();
+
+        // WHEN: Scrolled to the top and a message arrives.
+        await pageObjects.scrollToTop();
+        await directLine.emulateIncomingActivity('Message 6.');
+
+        // THEN: The "New messages" button should be shown and announced in the live region.
+        await pageConditions.scrollToEndButtonShown();
+        await waitFor(() =>
+          expect(
+            Array.from(
+              pageElements.liveRegion().querySelectorAll('.webchat__scroll-to-end-button__status'),
+              ({ textContent }) => textContent
+            )
+          ).toEqual(['New messages available. Press Alt Shift U to focus the "New messages" button.'])
+        );
+
+        // WHEN: The "New messages" button is clicked.
+        await host.click(pageElements.scrollToEndButton());
+
+        // THEN: It should scroll to the end, hide the button, and focus on the unread message.
+        await pageConditions.scrollToBottomCompleted();
+        await pageConditions.scrollToEndButtonHidden();
+        await waitFor(() => expect(pageElements.focusedActivity()).toBe(pageElements.activities()[5]));
+
+        // THEN: Marking messages as read and acknowledged should not update the state while rendering.
+        expect(
+          consoleErrorSpy.mock.calls
+            .filter(([message]) => `${message}`.includes('Cannot update a component'))
+            // [component being updated, component being rendered]
+            .map(([, updatedComponent, renderingComponent]) => [updatedComponent, renderingComponent])
+        ).toEqual([]);
+
+        // THEN: The transcript is at the end and "Message 6." is focused.
+        await host.snapshot('local');
+      });
+    </script>
+  </body>
+</html>

__tests__/html2/themeProvider/override.html

@@ -54,10 +54,7 @@
           !(
             message.includes(
               'Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead.'
-            ) ||
-            message.includes('ReactDOM.render is no longer supported in React 18. Use createRoot instead.') ||
-            // TODO: [P0] We should fix the "Cannot update a component while rendering a different component" error.
-            (message.includes('Cannot update a component') && message.includes('while rendering a different component'))
+            ) || message.includes('ReactDOM.render is no longer supported in React 18. Use createRoot instead.')
           )
         ) {
           consoleError(...args);
@@ -108,14 +105,11 @@
                       stylesRoot,
                       variant: 'fluent'
                     },
-                    createElement(
-                      ReactWebChat,
-                      {
-                        className: 'webchat-with-overrides',
-                        directLine,
-                        store
-                      }
-                    )
+                    createElement(ReactWebChat, {
+                      className: 'webchat-with-overrides',
+                      directLine,
+                      store
+                    })
                   )
                 )
               )

__tests__/html2/toast/noUpdateWhileRendering.html

@@ -0,0 +1,89 @@
+<!doctype html>
+<html lang="en-US">
+  <head>
+    <link href="/assets/index.css" rel="stylesheet" type="text/css" />
+    <!-- Shows the live region, so the snapshot shows the copy sent to screen readers. -->
+    <link href="/assets/accessibility.liveRegionAttachment.css" rel="stylesheet" type="text/css" />
+    <style>
+      /* Shows the live region below the toast. */
+      .webchat__live-region {
+        bottom: 0;
+      }
+    </style>
+  </head>
+  <body>
+    <main id="webchat"></main>
+    <script type="importmap">
+      {
+        "imports": {
+          "@testduet/wait-for": "https://esm.sh/@testduet/wait-for",
+          "botframework-webchat": "/__dist__/packages/bundle/static/botframework-webchat.js",
+          "botframework-webchat/": "/__dist__/packages/bundle/static/botframework-webchat/",
+          "jest-mock": "https://esm.sh/jest-mock",
+          "react": "/__dist__/packages/bundle/static/react.js",
+          "react-dom": "/__dist__/packages/bundle/static/react-dom.js",
+          "react-dom/client": "/__dist__/packages/bundle/static/react-dom/client.js"
+        }
+      }
+    </script>
+    <script type="module">
+      import '/test-harness.mjs';
+      import '/test-page-object.mjs';
+
+      import { waitFor } from '@testduet/wait-for';
+      import { createStoreWithOptions, ReactWebChat } from 'botframework-webchat';
+      import { spyOn } from 'jest-mock';
+      import { createElement } from 'react';
+      import { createRoot } from 'react-dom/client';
+
+      // TODO: This is for `createDirectLineEmulator` only, should find ways to eliminate this line.
+      window.WebChat = { createStoreWithOptions };
+
+      run(async function () {
+        // GIVEN: The development build of React, which logs "Cannot update a component while rendering a different component" to console.error.
+        const consoleErrorSpy = spyOn(console, 'error');
+        const { directLine, store } = testHelpers.createDirectLineEmulator();
+
+        // GIVEN: Web Chat keeps the live region copy, so it can be checked and seen in the snapshot.
+        createRoot(document.getElementById('webchat')).render(
+          createElement(ReactWebChat, {
+            directLine,
+            store,
+            styleOptions: { hideSendBox: true, internalLiveRegionFadeAfter: 60000 }
+          })
+        );
+
+        await pageConditions.uiConnected();
+
+        // WHEN: A notification is shown.
+        store.dispatch({
+          payload: { id: '1', level: 'info', message: 'Notification 1.' },
+          type: 'WEB_CHAT/SET_NOTIFICATION'
+        });
+
+        await pageConditions.toastShown(1);
+
+        // THEN: The notification should be announced in the live region.
+        await waitFor(() =>
+          expect(
+            Array.from(
+              pageElements.liveRegion().querySelectorAll('.webchat__toaster__notifications'),
+              ({ textContent }) => textContent
+            )
+          ).toContain('Notification 1.')
+        );
+
+        // THEN: The toaster should not update the live region while rendering.
+        expect(
+          consoleErrorSpy.mock.calls
+            .filter(([message]) => `${message}`.includes('Cannot update a component'))
+            // [component being updated, component being rendered]
+            .map(([, updatedComponent, renderingComponent]) => [updatedComponent, renderingComponent])
+        ).toEqual([]);
+
+        // THEN: The toast and its copy in the live region are shown.
+        await host.snapshot('local');
+      });
+    </script>
+  </body>
+</html>

__tests__/html2/typing/typingIndicator.liveRegion.noUpdateWhileRendering.html

@@ -0,0 +1,79 @@
+<!doctype html>
+<html lang="en-US">
+  <head>
+    <link href="/assets/index.css" rel="stylesheet" type="text/css" />
+    <!-- Shows the live region, so the snapshot shows the copy sent to screen readers. -->
+    <link href="/assets/accessibility.liveRegionAttachment.css" rel="stylesheet" type="text/css" />
+    <style>
+      /* The animated typing indicator cannot be snapshotted, show it as a black box. */
+      #webchat .webchat__typing-indicator {
+        background-color: black;
+        background-image: none;
+      }
+    </style>
+  </head>
+  <body>
+    <main id="webchat"></main>
+    <script type="importmap">
+      {
+        "imports": {
+          "@testduet/wait-for": "https://esm.sh/@testduet/wait-for",
+          "botframework-webchat": "/__dist__/packages/bundle/static/botframework-webchat.js",
+          "botframework-webchat/": "/__dist__/packages/bundle/static/botframework-webchat/",
+          "jest-mock": "https://esm.sh/jest-mock",
+          "react": "/__dist__/packages/bundle/static/react.js",
+          "react-dom": "/__dist__/packages/bundle/static/react-dom.js",
+          "react-dom/client": "/__dist__/packages/bundle/static/react-dom/client.js"
+        }
+      }
+    </script>
+    <script type="module">
+      import '/test-harness.mjs';
+      import '/test-page-object.mjs';
+
+      import { waitFor } from '@testduet/wait-for';
+      import { createStoreWithOptions, ReactWebChat, testIds } from 'botframework-webchat';
+      import { spyOn } from 'jest-mock';
+      import { createElement } from 'react';
+      import { createRoot } from 'react-dom/client';
+
+      // TODO: This is for `createDirectLineEmulator` and `pageElements.typingIndicator` only, should find ways to eliminate this line.
+      window.WebChat = { createStoreWithOptions, testIds };
+
+      run(async function () {
+        // GIVEN: The development build of React, which logs "Cannot update a component while rendering a different component" to console.error.
+        const consoleErrorSpy = spyOn(console, 'error');
+        const { directLine, store } = testHelpers.createDirectLineEmulator();
+
+        // GIVEN: Web Chat keeps the live region copy, so it can be checked and seen in the snapshot.
+        createRoot(document.getElementById('webchat')).render(
+          createElement(ReactWebChat, {
+            directLine,
+            store,
+            styleOptions: { hideSendBox: true, internalLiveRegionFadeAfter: 60000 }
+          })
+        );
+
+        await pageConditions.uiConnected();
+
+        // WHEN: The bot is typing.
+        await directLine.emulateIncomingActivity({ from: { id: 'bot', name: 'Bot', role: 'bot' }, type: 'typing' });
+
+        // THEN: The typing indicator should be shown and announced in the live region.
+        await pageConditions.typingIndicatorShown();
+        await waitFor(() => expect(pageElements.liveRegion().textContent).toContain('Bot is typing.'));
+
+        // THEN: The typing indicator should not update the live region while rendering.
+        expect(
+          consoleErrorSpy.mock.calls
+            .filter(([message]) => `${message}`.includes('Cannot update a component'))
+            // [component being updated, component being rendered]
+            .map(([, updatedComponent, renderingComponent]) => [updatedComponent, renderingComponent])
+        ).toEqual([]);
+
+        // THEN: The typing indicator (black box) and its copy in the live region are shown.
+        await host.snapshot('local');
+      });
+    </script>
+  </body>
+</html>

packages/api-graph/src/private/GraphProvider.tsx

@@ -7,7 +7,7 @@ import {
   type GraphSubscriber,
   type Identifier
 } from 'botframework-webchat-core/graph.js';
-import React, { memo, useEffect, useMemo, useState } from 'react';
+import React, { memo, useLayoutEffect, useMemo, useState } from 'react';
 import { custom, function_, is, object, optional, parse, pipe, readonly, type InferInput } from 'valibot';
 
 import GraphContext, { graphContextSchema, GraphContextType } from './GraphContext';
@@ -33,7 +33,9 @@ function GraphProvider(props: GraphProviderProps) {
 
   const [orderedActivityNodes, setOrderedActivityNodes] = useState<readonly DirectLineActivityNode[]>(EMPTY_ARRAY);
 
-  useEffect(() => {
+  // Layout effect: activities already in the store (e.g. rehydrated) must be in this state before the re-render of any other layout effect.
+  // Otherwise, that re-render would see activities in Redux which are not in the graph yet.
+  useLayoutEffect(() => {
     // Sync between graph and `orderedActivities`.
     const handleChange: GraphSubscriber = record => {
       const state = graph.getState();

packages/component/src/BasicTranscript.tsx

@@ -8,6 +8,7 @@ import React, {
   Fragment,
   memo,
   useCallback,
+  useLayoutEffect,
   useMemo,
   useRef,
   type KeyboardEventHandler,
@@ -493,15 +494,10 @@ const InternalTranscriptScrollable = ({ children, onFocusFiller }: InternalTrans
 
   const stickyChangedToTrue = prevSticky !== sticky && sticky;
 
-  useMemo(
-    () =>
-      stickyChangedToTrue &&
-      // TODO: [P2] Both `markActivityKeyAsRead` and `markAllAsAcknowledged` hook are setters of useState.
-      //       This means, in a render loop, we will be calling setter and will cause another re-render.
-      //       This is not trivial but we should think if there is a way to avoid this.
-      markAllAsAcknowledged(),
-    [markAllAsAcknowledged, stickyChangedToTrue]
-  );
+  // `markAllAsAcknowledged` is a setter of <ActivityAcknowledgementComposer>, it cannot be called while rendering this component.
+  useLayoutEffect(() => {
+    stickyChangedToTrue && markAllAsAcknowledged();
+  }, [markAllAsAcknowledged, stickyChangedToTrue]);
 
   // We need to check if `children` is `false` or not.
   // If `children` is `false`, React.Children.count(children) will still return 1 (truthy).

packages/component/src/ChatHistory/private/ScrollToEndButton.tsx

@@ -1,5 +1,5 @@
 import { hooks } from 'botframework-webchat-api';
-import React, { Fragment, memo, MutableRefObject, useCallback, useMemo } from 'react';
+import React, { Fragment, memo, MutableRefObject, useCallback, useLayoutEffect, useMemo } from 'react';
 import { useAnimatingToEnd, useAtEnd, useScrollToEnd, useSticky } from 'react-scroll-to-bottom';
 import { useRefFrom } from 'use-ref-from';
 
@@ -46,17 +46,17 @@ const useScrollToEndRenderResult = (terminatorRef: MutableRefObject<HTMLDivEleme
 
   // To prevent flashy button, we are not waiting for another render loop to update the `[readActivityKeys, unreadActivityKeys]` state.
   // Instead, we are building the next one in this `useMemo` call.
-  const nextUnreadActivityKeys = useMemo(() => {
-    // This code need to be careful reviewed as it will cause another render. The code should be converging.
-    // After we call `markActivityKeyAsRead`, everything will be read and nothing will be unread.
-    // That means, in next render, `unreadActivityKeys` will be emptied and the `markActivityKeyAsRead` will not get called again.
-    if (sticky && unreadActivityKeys.length) {
-      markActivityKeyAsRead(unreadActivityKeys[unreadActivityKeys.length - 1]);
-
-      return [];
-    }
+  const nextUnreadActivityKeys = useMemo(
+    () => (sticky && unreadActivityKeys.length ? [] : unreadActivityKeys),
+    [sticky, unreadActivityKeys]
+  );
 
-    return unreadActivityKeys;
+  // This code need to be careful reviewed as it will cause another render. The code should be converging.
+  // After we call `markActivityKeyAsRead`, everything will be read and nothing will be unread.
+  // That means, in next render, `unreadActivityKeys` will be emptied and the `markActivityKeyAsRead` will not get called again.
+  // Layout effect: updating <ActivityAcknowledgementComposer> while rendering this component is not allowed.
+  useLayoutEffect(() => {
+    sticky && unreadActivityKeys.length && markActivityKeyAsRead(unreadActivityKeys[unreadActivityKeys.length - 1]);
   }, [markActivityKeyAsRead, sticky, unreadActivityKeys]);
 
   const nextUnreadActivityKeysRef = useRefFrom(nextUnreadActivityKeys);