lib/shared/edit-messages-utils.js
25 ↗	(On Diff #23991)	In what scenario will `messageID` not be set? It would be easier to understand how this function is used if you included it in the same diff where you introduced the first usage
54–55 ↗	(On Diff #23991)	Why are we doing this? Do we need this info in the reducer?
56 ↗	(On Diff #23991)	I think you forgot to remove this `console.log`

This revision now requires changes to proceed.Mar 22 2023, 3:14 PM

Address review comments

lib/shared/edit-messages-utils.js

25 ↗

(On Diff #23991)

In what scenario will messageID not be set? It would be easier to understand how this function is used if you included it in the same diff where you introduced the first usage

It is not currently used anywhere, but I use it locally to test it (tooltip-utils.js):

function useMessageEditAction(
  item: ChatMessageInfoItem,
  threadInfo: ThreadInfo,
): ?MessageTooltipAction {
  const { messageInfo } = item;

  const canEditMessage = useCanEditMessage(threadInfo, messageInfo);
  const editMessage = useEditMessage(messageInfo.id);

  return React.useMemo(() => {
    if (!canEditMessage) {
      return null;
    }

    if (item.messageInfoType !== 'composable') {
      return null;
    }

    const buttonContent = (
      <SWMansionIcon icon="edit-1" size={18} disableFill={false} />
    );
    const onClickEdit = () => {
      editMessage('Test ' + new Date().toLocaleString());
    };

    return {
      actionButtonContent: buttonContent,
      onClick: onClickEdit,
      label: 'Edit',
    };
  }, [canEditMessage, editMessage, item]);
}

Scenario: Here sometimes messageInfo.id can be sometimes undefined, and I cannot use condition here, before calling the useEditMessage, because of React Hook "useEditMessage" is called conditionally error.

Harbormaster completed remote builds in B17573: Diff 24061.Mar 24 2023, 3:54 AM

ashoat added inline comments.Mar 24 2023, 10:54 AM

lib/shared/edit-messages-utils.js
42–44 ↗	(On Diff #24061)	Why are we doing this? What does it achieve?
38 ↗	(On Diff #23991)	Should we instead have an `invariant` here? From your code snippet, it looks like `useCanEditMessage` should prevent the callback from being called if `!messageID`