Add useLogicalGroupKey() so rows re-render when their logical group changes by OEvgeny · Pull Request #5966 · microsoft/BotFramework-WebChat

Eugene (OEvgeny) · 2026-10-22T23:08:39Z

Follow-up to #5958.

Changelog Entry

Fixed

Fixed grouped messages showing avatars and breaking keyboard navigation in React Strict Mode, in PR #5966, by @OEvgeny

Description

In React Strict Mode, the effect cleanup unregistered part groups, so grouped messages showed their own avatar and keyboard navigation skipped the group. Groups now register from layout effects into a keyed store; each row subscribes to its group key.

Design

createKeyedStore() notifies a key (one EventTarget event type) only when its value changes; not useSyncExternalStore(), which React 16.8/17 lack.
Keyboard navigation reads the stores at key-press time.

Specific Changes

createKeyedStore, useKeyedStoreValue: new
ActivityLogicalGroupingComposer: two stores; setLogicalGrouping() replaces addLogicalGrouping()
useLogicalGroupKey(), useGroupDescendantIdByGroupKey(): new hooks, replacing the useGet*() getters
TranscriptFocusComposer: reads the stores at key-press time; one descendant ID helper
StackedLayout, PartGroupingActivity, usePartGroupingLogicalGroup: use the hooks
part-grouping/strictMode.html: snapshot 1, avatar on the group only; 2–4, focus Part 2 → Part 1 → group
Unit specs: only changed keys re-render; leaving and moving groups; collapsed focus

I have added tests and executed them locally
I have added .changelog/<PR#>-<slug>.md
I have updated documentation

Review Checklist

This section is for contributors to review your work.

Accessibility reviewed (tab order, content readability, alt text, color contrast)
Browser and platform compatibilities reviewed
CSS styles reviewed (minimal rules, no z-index)
Documents reviewed (docs, samples, live demo)
Internationalization reviewed (strings, unit formatting)
package.json and package-lock.json reviewed
Security reviewed (no data URIs, check for nonce leak)
Tests reviewed (coverage, legitimacy)

…edged state useLiveRegion and the typing indicator queued live region nodes in useMemo, the "New messages" button marked activities as read and the transcript marked them as acknowledged in useMemo. React development builds log "Cannot update a component while rendering a different component" for each. Move the updates into layout effects (still before paint) and queue a live region node once per deps change, also in Strict Mode.

Part grouping registers itself while rendering and its activities read their group while rendering. The map was also kept in a state that was never read, so the registration updated <ActivityLogicalGroupingComposer> while rendering.

…component" in tests

…ut effects re-render A rehydrated store has activities before the graph is created, but the graph only added them on the next dispatch, and <GraphProvider> synchronized in a passive effect. Queuing the rehydrated notification to the live region in a layout effect re-renders the transcript (useActivities) after passive effects but before the graph state update, and the development assertion found 0 activities in the graph and 1 in Redux. - createGraphFromStore: add the store's activities when the graph is created - GraphProvider: subscribe and synchronize in a layout effect, so the graph state is updated in the same render as other layout effect updates

Activities subscribe to their own logical group key and re-render only when it changes. Part grouping adds its group in a layout effect instead of while rendering, so the group comes back after Strict Mode cleans up and re-runs effects. The group reads its own descendant ID by its group key.

The filter compared group keys with activity keys, so no activity was ever removed.

…roup-key-hook # Conflicts: # packages/component/src/providers/ActivityLogicalGrouping/ActivityLogicalGroupingComposer.ts

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

William Wong (compulim) · 2026-10-22T23:38:10Z

 export { default as useGetLogicalGroupKey } from './useGetLogicalGroupKey';
 export { default as useGetGroupState } from './useGetGroupState';


Remove useGetLogicalGroupKey, everyone should use useLogicalGroupKey() so they get proper subscription.

Do the same treatment for useGetGroupState -> useGroupState(groupKey: string): readonly [...].

Done in f1f2578. index.ts now exports useLogicalGroupKey(activityKey) and useGroupState(groupKey): readonly [GroupState | undefined]; both subscribe. Group state is a frozen value on the registration now (not a getter), and the part grouping re-registers from its layout effect when it collapses or expands, which notifies useGroupState(). Nothing renders from the group state yet (the only reader is keyboard navigation, below), so useGroupState() has no caller outside its spec.

One exception, on purpose: TranscriptFocusComposer's arrow-key handling looks up the group and state of other activities (the next or previous one) while handling the key press, and renders nothing from them. It reads them through private getters in ActivityLogicalGrouping/private/ (not exported from index.ts, JSDoc says why). A hook cannot subscribe to "whichever activity is next", and subscribing the composer to the whole map would re-render it and every focus consumer on each grouping change.

See this comment: index.ts exports no getter hooks; useLogicalGroupKey() subscribes. I dropped useGroupIsCollapsed() until a component renders from it; keyboard navigation reads isCollapsed from the store at key-press time.

William Wong (compulim) · 2026-10-22T23:42:41Z

  const logicalGroupingsRef = useRef<Map<string, LogicalGrouping>>(new Map());
-  // A ref, not a state: part grouping adds itself while rendering, and its activities read their group while rendering.
  const activityToGroupMapRef = useRef<ReadonlyMap<string, string | undefined>>(new Map());
+  const listenersRef = useRef<Set<() => void>>(new Set());


For performance optimization, listener should register its interest (what activity ID or kinda it is interested in), then on notify(), it will only notify listener who is interested in.

Done in f1f2578 and b9d83a6. Listeners now subscribe by key: subscribeLogicalGroupKey(activityKey, listener) and subscribeGroupState(groupKey, listener). On add/remove, the composer diffs the activity-to-group map and notifies only the activities whose group changed; the group's listeners are notified only when its state object changes or it is removed. ActivityLogicalGroupingComposer.spec.tsx checks each case (add, add an activity, move an activity to another group, collapse, remove).

William Wong (compulim) · 2026-10-22T23:45:36Z

Do the same treatment, useGroupDescendantIdByGroupKey(groupKey?: string): readonly [string | undefined]. And make sure caller of this hook will get re-rendered by the result changed.

Consider building a utility so all the hooks modified in this PR can share the same code that do listener registration and notification.

Done in f1f2578 and b9d83a6: useGroupDescendantIdByGroupKey(groupKey?): readonly [string | undefined]. The shared utility is hooks/internal/keyedSubscription/ (createKeyedSubscription() + useKeyedSubscriptionValue()); useLogicalGroupKey() and useGroupState() are one line each on top of it.

useGroupDescendantIdByGroupKey() does not use it: the ID is derived from groupKey and the transcript's per-instance prefix, which never changes, so it can only change when the argument or the context changes, and both already re-render the caller. A subscription would have nothing to notify. If you want the same for useGetDescendantIdByActivityKey() (ActivityRow), I can do it here or in a follow-up.

- Remove useGetLogicalGroupKey() and useGetGroupState() from the provider exports; TranscriptFocusComposer reads them at key-press time through private getters - Add useGroupState(groupKey), notified when a part group collapses or expands (re-registered from a layout effect) - Listeners register for an activity key or group key; add/remove notify only the keys that changed (createKeyedSubscription, useKeyedSubscriptionValue) - Replace useGetGroupDescendantIdByGroupKey() with useGroupDescendantIdByGroupKey(groupKey)

…ctivity between groups

William Wong (compulim) · 2026-10-23T00:55:21Z

+  const [, forceRender] = useReducer(increment, 0);
+
+  const value = getValue(key);
+  const valueRef = useRef<T>(value);


Can it use useRefFrom?

No ref is left after switching to useState (1e884ff, see the other thread), so useRefFrom is no longer needed here.

William Wong (compulim) · 2026-10-23T00:58:11Z

+  key: K,
+  getValue: (key: K) => T
+): readonly [T] {
+  const [, forceRender] = useReducer(increment, 0);


Will this function simpler if we use useState(value) instead of forceRender() and useRef?

Done in 1e884ff and 6adb788: the value is read in render, and useState holds a frozen [key, value] tuple that only triggers re-renders. A plain value was not enough: on a key switch, if the new key's value changed to the old key's value before subscribing, React saw an equal state and skipped the re-render. Trade-off: after a re-render, the next notification without a change calls the component function once more before React bails out (no children or effects re-render).

William Wong (compulim) · 2026-10-23T00:59:45Z

 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 [partGroupKey] = useState(() => `part-${random().toString(36).slice(2, 11)}`);


Suggested change

const [partGroupKey] = useState(() => `part-${random().toString(36).slice(2, 11)}`);

const partGroupKey = useMemo(() => `part-${random().toString(36).slice(2, 11)}`, []);

They are the same, but useMemo feels lighter and more straightforward.

Done in 39e625e.

…nd a forced render Test switching the key and function values.

…tric with removeLogicalGrouping(key) It adds or replaces the grouping with the same key. LogicalGrouping no longer carries the key.

…ject useGroupState() becomes useGroupIsCollapsed(): readonly [boolean | undefined], notified only when the boolean changes.

William Wong (compulim) · 2026-10-23T01:23:13Z

+ * The ID is derived from the group key and the transcript instance, so the component re-renders when either changes.
+ */
+export default function useGroupDescendantIdByGroupKey(groupKey?: string | undefined): readonly [string | undefined] {
+  const { getGroupDescendantIdByGroupKey } = useTranscriptFocusContext();


The getGroupDescendantIdByGroupKey function does not invalidate unless the prefix has changed.

That means, the result may not be updated through re-render. Use keyed subscription instead.

As we only read getGroupDescendantIdByGroupKey here, this hook can only based on the behavior of getGroupDescendantIdByGroupKey but not other stuff in the context.

We cannot know if other stuff would help invalidate the context because in this function, we never need to know how other stuff in the context work.

You're right — the hook shouldn't rely on how the getter is built. Done in 6af523a: TranscriptFocusComposer owns a keyed subscription for group descendant IDs and notifies every subscribed group when the getter changes (only on mount today, as the prefix is per instance); the hook is now useKeyedSubscriptionValue() on top of it.

Addressed differently in 14e31ea (the 6af523a subscription is gone), see this comment: the hook reads the prefix value and derives the ID with useMemo, so it depends only on prefix and groupKey. No getter is left to invalidate, so no keyed subscription is needed.

…so a key switch never misses a change The state held the last set value (the previous key's). On a key switch, if the new key's value changed to the previous key's value before the reader subscribed, the re-check equaled the state and React bailed out, leaving the reader on a stale value. A frozen [key, value] tuple always differs after a key switch, and wrapping the value removes the function-form workaround.

…ying on how the getter invalidates TranscriptFocusComposer owns a keyed subscription for group descendant IDs and notifies every subscribed group when the getter changes (today, the prefix is per instance, so only on mount). The hook reads with useKeyedSubscriptionValue(), returning [undefined] without a group key. Adds notifyAll() to createKeyedSubscription().

…cendant ID helper, useMemo for stores

Eugene (OEvgeny) · 2026-10-23T04:16:52Z

As discussed, #5966 now uses the keyed store: one createKeyedStore() (get/set/delete/subscribe on an EventTarget, one event type per key, notifies a key only when its value changes) and one useKeyedStoreValue(store, key): readonly [T | undefined]. The grouping composer is two stores plus setLogicalGrouping()/removeLogicalGrouping() from layout effects; useLogicalGroupKey() is one line, and keyboard navigation reads store.get at key-press time. Two deviations: (1) useGroupDescendantIdByGroupKey() has no keyed subscription: the context exposes the prefix value and the hook derives the ID with useMemo, so it depends only on values it reads (r4355209256). (2) useGroupIsCollapsed() is dropped until something renders from it (r4354652379). Production diff: +212/−164 (was +294/−136).

Eugene Olonov added 9 commits October 22, 2026 04:44

Stop ignoring "Cannot update a component while rendering a different …

d20a9f6

…component" in tests

Add the changelog fragment

1121f33

Remove an activity from its logical group when it leaves the group

46cf270

The filter compared group keys with activity keys, so no activity was ever removed.

Merge remote-tracking branch 'origin/main' into OEvgeny/fix-logical-g…

a90257f

…roup-key-hook # Conflicts: # packages/component/src/providers/ActivityLogicalGrouping/ActivityLogicalGroupingComposer.ts

Add the changelog fragment

a10e123

Eugene (OEvgeny) marked this pull request as ready for review October 22, 2026 23:26

Eugene (OEvgeny) requested review from William Wong (compulim), Chris Whitten (cwhitten) and TJ Durnford (tdurnford) as code owners October 22, 2026 23:26

Copilot AI balanced review requested due to automatic review settings October 22, 2026 23:26

Copilot AI reviewed Oct 22, 2026

William Wong (compulim) reviewed Oct 22, 2026

View reviewed changes

Comment thread ...onent/src/Middleware/ActivityGrouping/ui/PartGrouping/private/usePartGroupingLogicalGroup.ts

William Wong (compulim) reviewed Oct 22, 2026

View reviewed changes

Eugene Olonov added 2 commits October 22, 2026 17:10

Keep a newer listener set on a repeated unsubscribe; test moving an a…

b9d83a6

…ctivity between groups

William Wong (compulim) reviewed Oct 23, 2026

View reviewed changes

Eugene Olonov added 4 commits October 22, 2026 18:03

useKeyedSubscriptionValue: re-render with useState instead of a ref a…

1e884ff

…nd a forced render Test switching the key and function values.

usePartGroupingLogicalGroup: create the group key with useMemo

39e625e

Rename addLogicalGrouping to setLogicalGrouping(key, grouping), symme…

b7bb61d

…tric with removeLogicalGrouping(key) It adds or replaces the grouping with the same key. LogicalGrouping no longer carries the key.

Store isCollapsed on the logical grouping instead of a group state ob…

adecbdf

…ject useGroupState() becomes useGroupIsCollapsed(): readonly [boolean | undefined], notified only when the boolean changes.

William Wong (compulim) reviewed Oct 23, 2026

View reviewed changes

Eugene Olonov added 3 commits October 22, 2026 18:37

Simplify logical grouping to a keyed store

14e31ea

Address review nits: private stores hook, getLogicalGrouping, one des…

e0a12a0

…cendant ID helper, useMemo for stores

William Wong (compulim) approved these changes Oct 23, 2026

View reviewed changes

Eugene (OEvgeny) merged commit 8757080 into main Oct 23, 2026
60 of 61 checks passed

Eugene (OEvgeny) deleted the OEvgeny/fix-logical-group-key-hook branch October 23, 2026 08:52

Eugene (OEvgeny) mentioned this pull request Oct 23, 2026

Test that an activity updated into or out of a part grouping renders once #5970

Open

1 of 11 tasks

		export { default as useGetLogicalGroupKey } from './useGetLogicalGroupKey';
		export { default as useGetGroupState } from './useGetGroupState';

	const [partGroupKey] = useState(() => `part-${random().toString(36).slice(2, 11)}`);
	const partGroupKey = useMemo(() => `part-${random().toString(36).slice(2, 11)}`, []);

Conversation

Eugene (OEvgeny) commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog Entry

Fixed

Description

Design

Specific Changes

Review Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eugene (OEvgeny) commented Oct 23, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Eugene (OEvgeny) commented Oct 22, 2026 •

edited

Loading