Skip to content
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

RFC: Architecture of chat adapter and supporting different chat providers #3713

Open
3 of 15 tasks
compulim opened this issue Feb 9, 2021 · 1 comment
Open
3 of 15 tasks
Labels
backlog Out of scope for the current iteration but it will be evaluated in a future release. feature-request Azure report label front-burner

Comments

@compulim
Copy link
Contributor

compulim commented Feb 9, 2021

Feature Request

To ready Web Chat into the future, I am proposing an architecture to allow Web Chat to operate on first party and third party chat providers.

Related issues:

Today

Today, Web Chat is quite coupled with the behavior of Direct Line protocol (implemented by Azure Bot Services). Especially, the following behaviors are built into Web Chat (non-exhaustive):

  • Read receipt
    • Thru the use of "channel data", a property bag of client-specific data
    • Assume the protocol will "echo back" the activity with filled data, e.g. conversation ID was not filled when we send the message, but ABS will echo back a newer version of activity with the conversation ID filled
  • No send receipt is supported
  • Order of activities
    • Chronologically sorted using server timestamp, which is not available for user-sent activities until echo back is received
    • When server timestamp is not available, we measure clockskew to guess a reasonable server timestamp
    • replyToID is used to delay activity if its precedent is not received yet
  • ID
    • Bot-sent activities have ID
    • User-sent activities don't have ID until it was "echo back" from the service, currently, we use clientActivityID as "client ID"

These behaviors are built into Redux saga of Web Chat. We need to move them out into a separate Direct Line chat adapter.

Tomorrow

I am proposing a stateful approach of chat adapter. For stateful, I mean the chat adapter is responsible for storing all activities in the transcript. This includes: received activities, sending activities, and sent activities.

In this approach, Web Chat will not in the business of (non-exhaustive):

  • Inserting activity into correct order
    • Web Chat may requires the chat adapter to insert new activity only at the end of the transcript
    • In future, Web Chat may support infinite scrolling by send requests to the chat adapter to populate activities by prepending to the transcript
    • It is unlikely that Web Chat will support populating activities in the middle of an existing transcript, due to accessibility concern
  • Updating existing activity
    • Send and read receipts are maintained by chat adapter
  • Saving and restoring chat history
    • It is up to the chat adapter how to keep the chat history

Also, core features of Web Chat will not requires "channel data". If the chat adapter support backchannel data, developer can use Web Chat to send backchannel data. However, it is up to the chat adapter on how to send these data:

  • One-off data will be send as channel data, a property bag dedicated for backchannel data
  • One-off data will be send as an an attachment
  • One-off data will be send as a separate activity

Restrictions

To provide best user experience, there are restrictions on the design of chat adapter:

  • All activities in the transcript must have a permanent ID, including activities that are pending to send
  • Activities can only be appended
    • This is requirement for accessibility
  • (TBC) If activities are updated (must be in-place) or deleted, it may break accessibility experience

Potential chat adapter interface

Internally, it would work like this:

ReactDOM.render(
  <ACSChatAdapter endpointURL="..." token="...">
    {(chatAdapterProps: ChatAdapterProps) => <ReactWebChat {...chatAdapterProps} styleOptions={...} />}
  </ACSChatAdapter>,
  document.getElementById('root')
);

type ChannelDataExtension = {
  channelData: {
    'webchat:avatar:initials'?: string;
    'webchat:avatar:image'?: string;
    'webchat:display-name'?: string | '__BOT__';
    'webchat:delivery-status': 'error' | 'sending' | 'sent';
    'webchat:key': string;
    'webchat:read-by'?: 'some' | 'all';
    'webchat:tracking-number'?: string;
    'webchat:who': 'channel' | 'others' | 'self';
  };
};

type Activity = DirectLineActivity & ChannelDataExtension;

type Notification = {
  alt?: string;
  data?: any;
  id: string;
  level?: string;
  message?: string;
};

type ConnectivityStatusNotification = Notification & {
  id: 'connectivitystatus';
  data: 'connected' | 'connecting' | 'fatal' | 'reconnecting';
};

type ChatAdapterProps = {
  activities: Activity[];
  notifications?: Notification[];
  userId?: string;
  username?: string;

  emitTypingIndicator?: () => void;
  resend?: (trackingNumber: string) => string;
  returnReadReceipt?: (activityKey: string) => void;
  sendEvent?: (name: string, value: any) => string;
  sendFiles?: (files: File[]) => string;
  sendMessage?: (message: string) => string;
  sendMessageBack?: (value: any, text: string, displayText: string) => string;
  sendPostBack?: (value: any) => string;

  typingUsers?: {
    [userId: string]: {
      at: number; // Change to Date
      name?: string;
      who: 'others' | 'self';
    };
  };

  // Direct Line specific props
  directLineReferenceGrammarId?: string;
  getDirectLineOAuthCodeChallenge: () => Promise<string>;
};

Although not required, a chat adapter should minimally implement both activities and sendMessage. For example,

ReactDOM.render(
  <ReactWebChat 
    activities={[
      channelData: {
        'webchat:key': 'id-1'
        'webchat:who': 'others'
      },
      text: 'Hello, World!'
      type: 'message'
    ]}
    sendMessage={message => alert(message)}
    styleOptions={...} 
  />,
  document.getElementById('root')
);

Bridging to DirectLineJS interface

Internally, if directLine object is passed, along with optional store parameter, we will use the older botframework-webchat-core to instantiate a Redux store, and use a <LegacyChatAdapterBridge> to bridge it from Redux store to new chat adapter interface.

Internally,

BridgeableWebChat({ directLine, store, ...props }) {
  return (
    directLine ?
      <LegacyChatAdapterBridge directLine={directLine} store={store}>
        {chatAdapterProps => <ReactWebChat {...chatAdapterProps} {...props} />}
      </LegacyChatAdapterBridge>
    :
      <ReactWebChat {...props} />
  );
}

const LegacyChatAdapterBridge = ({ directLine, store }) => {
  const { dispatch } = store;
  const activities = useSelector(({ activities }) => activities); // For clarity only, we actually use <Redux.Provider> before useSelector.
  const sendMessage = useCallback(message => dispatch({ payload: message, type: 'SEND_MESSAGE' }), [dispatch])

  // Also for other chat adapter interface:
  const notifications = [...];
  const sendEvent = useCallback(...);
  const sendFiles = useCallback(...);
  const sendMessageBack = useCallback(...);
  const sendPostBack = useCallback(...);

  return children({ activities, notifications, sendEvent, sendFiles, sendMessage, sendMessageBack, sendPostBack });
}

MVP for ACS chat adapter

  • Able to send and receive plain text messages
  • Able to send and receive typing indicators
  • All operations are efficient, a.k.a. no obvious performance penalty
  • Able to handle conversation with 3+ users
  • Works with IE11
  • Bonus: Speech recognition and synthesis works out of the box

Web Chat side change

  • Instead of activity.id, use activity.channelData['webchat:key']
    • It will be persistent ID, mostly "client activity ID" coalesced by "server-generated activity ID"
  • Instead of activity.role, use activity.channelData['webchat:who'] to determines whether the message is from self, others, or system
  • Instead of activity.channelData.sendState, rename to activity.channelData['webchat:send-state'] with value of sent or sending
  • bf-wc-component should call useEmitTypingIndicator(), instead of by saga inside bf-wc-core
    • Currently, only dictation will use useEmitTypingIndicator() hook
  • Should show system messages like "John Doe has joined the conversation"
  • (Maybe already done) How long typing indicator appears on the screen is based on the chat adapter
  • Should warn if the activity array is updated in an order not supported:
    • Appending activities at the end of the transcript
    • (Need accessibility design) Replacing an activity and deleting an activity
      • (Need more thoughts) When "filling in the blank" for outgoing activity, this is not counted as replacing an activity
  • (Need thoughts) Retrieving avatar initials and images from chat adapter
  • Backcompat for the new Redux store to work with previous actions

Additional logics need to add on top of ACS chat adapter

  • Read receipts
    • ✔ Read receipts is stored in a different array than messages. We have to add a memoizable function to merge them together in an array of message which contains read receipt for that individual message
  • Typing notification
    • (TBD) After we receive a message, we should remove that specific user from the typing notification immediately until the next typing notification
    • It is up to the chat adapter on how to send typing notification. In ACS, the adapter should send every 2-5 seconds. We have to add a interval function for sending typing notification
      • Some chat adapter may send on/off, some may use a different interval, etc

Comments to ACS

General

  • Typo: case sensitivity on endpointUrl -> endpointURL, references from MDN:
  • How to detect connection status?
    • ACS: TBD
  • When issuing token, sometimes, the ACS will return 404
    • {"error":{"code":"IdentityNotFound","message":"Provided identity doesn't exist."}}
  • Should somehow notify the end-developer that the token is invalid?
    • Let investigate more about this before raising this question to ACS
  • What is the official term of user ID? communicationUserId or identity?
    • ACS: userId
  • Does ACS UI SDK and its dependencies works on React Native?
  • How to resend failed messages?

Related to read receipts

  • useSendReadReceipt is only needed for the very last message, not needed for every messages in the transcript
  • If a message is being deleted, the read receipt will be voided as the chatMessageId reference is not in the transcript
    • Thus, the chatMessageId is not useful and should be ignored. The readOn should be used as a way to determine which message in the transcript is read (compare readOn vs. createdOn)
    • For the same reason, the time represented by readOn is not be reliable way to determine the exact time a message is read

Related to useChatMessages

(Some questions in this section is obsoleted as ACS is moving to another style of API.)

  • useChatMessages is not exported
  • useChatMessages requires both useFetchMessages and useSubscribeMessage
    • If useFetchMessages is not called, conversation history will not be fetched by useChatMessages
    • If useSubscribeMessage is not called, new incoming messages will not appears in useChatMessages
  • useChatMessages always creates a new instance of Array even if its content did not change
  • While useChatMessages (or useFetchMessages) are downloading conversation history, the end-user sending a message may end up in a wrong position, repro:
    • Create a long conversation (~100 messages)
    • Restart the conversation, so it show up as blank for a few seconds while downloading the conversation history
    • Send a message immediately
    • Expected: the newly sent message should appear as the last message in the transcript
    • Actual: the newly sent message appear in the middleware of the transcript
  • How can we know which messages returned from useChatMessages is new or from conversation history?
    • New = received after the first useChatMessages call
    • Message from conversation history are considered read
    • The other way, do we know which message is read? And how can we notify the service which one are read by us?

Related to useSendMessage

(Some questions in this section is obsoleted as ACS is moving to another style of API.)

  • After useSendMessage to send a message, useChatMessages will have a new message appended
    • That message do not have type field, it should have
  • Can we send non-textual message? Such as event activity
  • How to resend messages that were failed to send?
  • How to detect if a message failed to send?
  • useSendMessage should know about identity without the need of passing it manually
    • Meanwhile, useSendTypingNotification does not requires any parameters including identity
  • If messages are sent fast enough, the order perceived by others could be different
    • See screenshot below

Related to useTypingUsers and useSendTypingNotification

(Some questions in this section is obsoleted as ACS is moving to another style of API.)

  • useTypingUsers should automatically call useThreadMembers to get list of thread members
  • useTypingUsers is triggering re-render every 500 ms while anyone in the thread is typing
    • This has significant performance penalty
  • How to get displayName? It's not in @azure/communication-common/CommunicationUser
    • useTypingUsers only returns communicationUserId without displayName
  • After calling useSendTypingNotification, how to stop it after the message has been sent? Repro:
    • Call the function returned by useSendTypingNotification()
    • Call the function returned by useSendMessage()
    • Expect: on the other side of the conversation, the "send typing notification" should stop
    • Actual: on the other side, the useSendTypingNotification() did return an empty array

Related to other hooks or providers

  • @azure/communication-common supports refreshTokenCallback without initialToken, but <ChatProvider> does not
    • Uncaught TypeError: Cannot read property 'split' of undefined at getIdFromToken (SDKUtils.ts:74) at ChatProvider (ChatProvider.tsx:47)
  • Splash screen should not be provided by <ChatProvider> (or <ChatThreadProvider>)
    • Instead, allowing developers to specify their own splash screen, probably using children({ connected })
  • Seems <ChatThreadProvider> is not needed as <ChatProvider> will internally create it
  • useThreadMembers requires to call useFetchThreadMembers
    • This is very similar to useChatMessages which requires useFetchChatMessages and useSubscribeChatMessages
    • But it don't have useSubscribeThreadMembers
    • If a new member joined the thread, useThreadMembers will not report it automatically
  • How to retrieve user ID?
    • Currently, it's behind import { useUserId } from '@azure/acs-ui-sdk/dist/providers/ChatProvider';

Outgoing messages appended in the middle

If a message is being sent while fetching user messages, it could be added in the middle of the transcript

image

Perceived order of messages could differs between clients

This is probably SDK issue. Right-hand side is the sender, although it is sending from 1 to 0, the transcript was perceived in a random order. Left-hand side is the receiver.

image

After token refresh or reloading the conversation history, both sender and receiver perceive the conversation with a correct order.

image

(To be continued)

[feature-request]

@compulim compulim added customer-reported Required for internal Azure reporting. Do not delete. Bot Services Required for internal Azure reporting. Do not delete. Do not change color. feature-request Azure report label labels Feb 9, 2021
@corinagum
Copy link
Contributor

corinagum commented Feb 10, 2021

This isn't a customer-reported issue per se, right? Can we remove customer reported and Bot Services?

Do we want a new Area label regarding adapters?

@compulim compulim removed Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Required for internal Azure reporting. Do not delete. labels Feb 19, 2021
@compulim compulim pinned this issue Mar 1, 2021
@corinagum corinagum added backlog Out of scope for the current iteration but it will be evaluated in a future release. front-burner labels Mar 19, 2021
@compulim compulim unpinned this issue May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Out of scope for the current iteration but it will be evaluated in a future release. feature-request Azure report label front-burner
Projects
None yet
Development

No branches or pull requests

2 participants