Restart Conversation from any point Functionality #2089
Conversation
srinaath
requested review from
a-b-r-o-w-n,
compulim,
corinagum,
cwhitten,
tdurnford and
tonyanziano
as code owners
| @@ -107,7 +106,7 @@ const defaultConfig = { | |||
| ], | |||
| }, | |||
|
|
|||
| devtool: 'source-map', | |||
| devtool: 'eval-source-map', | |||
There was a problem hiding this comment.
Does this fix the Web Chat source maps and preserve the Emulator source maps? I tried to change it locally from devtool: sourcemap to using the SourceMapDevTool plugin from web chat and it managed to fix the Web Chat source maps, but downgraded the quality of the Emulator source maps to point to the transpiled code.
There was a problem hiding this comment.
It does not Tony. So it still shows the transpiled code for webchat and debuggable code for emulator. So I was facing an issue with step through debugging on Chrome Dev tools failing when i use source-map. The moment I use eval-source-maps it does not break or Chrome dev tools does not seem to break when i step though and debug. Have u faced this issue before?
There was a problem hiding this comment.
The issue I was facing was that the Web Chat sourcemaps were pointing to the wrong files (usually off by one file in the parent directory). So if in some Web Chat directory, there were files A, B, and C, in my debugger, trying to pull up file C would point to the sourcemap for file B.
Switching to the source map plugin fixed that for me, but also downgraded the sourcemaps of the Emulator code to point to the transpiled code instead of the TypeScript source.
There was a problem hiding this comment.
I have reverted it to use just source-map. Seems like we can come up with a good solution for R9 that enables source maps for emulator as well as enable it for our shared libs (webchat, shared/app, shared/sdk)
| FROM_USER_ROLE: 'user', | ||
| FROM_BOT_ROLE: 'bot', |
There was a problem hiding this comment.
Consider changing these to USER_ROLE and BOT_ROLE because they are the same values used in the recipient property of activities, and then could be used elsewhere in the application without having to make new constants for TO_USER_ROLE and TO_BOT_ROLE
| const activities = await resp.json(); | ||
| return activities; | ||
| } catch (ex) { | ||
| return []; |
There was a problem hiding this comment.
The pattern in the ConversationService is to return an HTTP response, so maybe consider returning the fetch and performing the resp.json() in the service-calling code.
Also, it might be a good idea to surface the exception in some way for debugging.
| @@ -107,7 +106,7 @@ const defaultConfig = { | |||
| ], | |||
| }, | |||
|
|
|||
| devtool: 'source-map', | |||
| devtool: 'eval-source-map', | |||
There was a problem hiding this comment.
The issue I was facing was that the Web Chat sourcemaps were pointing to the wrong files (usually off by one file in the parent directory). So if in some Web Chat directory, there were files A, B, and C, in my debugger, trying to pull up file C would point to the sourcemap for file B.
Switching to the source map plugin fixed that for me, but also downgraded the sourcemaps of the Emulator code to point to the transpiled code instead of the TypeScript source.
| // import { EventEmitter } from 'events'; | ||
|
|
||
| // import { Activity } from 'botframework-schema'; |
| export interface WebChatActivityChannel { | ||
| sendWcEvents: (args: ChannelPayload) => void; | ||
| getWebchatChannelSubscriber: () => Channel<ChannelPayload>; | ||
| } | ||
|
|
||
| export interface WebchatEventPayload { |
There was a problem hiding this comment.
align on capitalization pattern for Web Chat:
WebChatActivityChannel
WebchatEventPayload
is the C capitalized or not?
There was a problem hiding this comment.
WebChat is what I'll go with. Reason being there are multiple usages like createWebChatStore etc written before. I'll probably stick to that pattern. Though, i think Webchat semantically makes more sense (https://github.com/microsoft/BotFramework-webchat)
| } | ||
|
|
||
| function createReplayActivitySniffer(documentId: string, meta: ReplayActivitySnifferProps = undefined) { | ||
| return ({ dispatch }) => next => async action => { |
| speechKey?: string; | ||
| speechRegion?: string; | ||
| user: User; |
There was a problem hiding this comment.
keep user here -- sorted alphabetically
| meta.conversationQueue.getNextActivityForPost, | ||
| ]); | ||
| if (postActivity) { | ||
| yield call(dispatchActivityToWebchat, dispatch, postActivity); |
There was a problem hiding this comment.
is there a reason you aren't using put here instead of calling dispatch?
| } finally { | ||
| yield put(updatePendingSpeechTokenRetrieval(documentId, false)); | ||
| yield fork(ChatSagas.handleReplayIfRequired, { documentId, action, dispatch, meta }); |
There was a problem hiding this comment.
why does dispatch need to be passed in if we have access to put ?
There was a problem hiding this comment.
So, this is dispatching on the webchat store. Put automatically dispatches to our emulator store. So, thats why I'm using call on the dispatch i get from the middleware
There was a problem hiding this comment.
Ahhh, I didn't catch that. Sounds good to me 👍
| yield* throwErrorFromResponse('Error occurred while retrieving the web socket port', res); | ||
| throw new Error( | ||
| `Error occurred while retrieving the WebSocket server port: ${res.status}: ${res.statusText || | ||
| 'No status text'}` | ||
| ); |
There was a problem hiding this comment.
change back to throwErrorFromResponse?
There was a problem hiding this comment.
Nice catch.. Effects of resolving the Merge conflict
| meta.conversationQueue.getNextActivityForPost, | ||
| ]); | ||
| if (postActivity) { | ||
| yield call(dispatchActivityToWebchat, dispatch, postActivity); |
| } finally { | ||
| yield put(updatePendingSpeechTokenRetrieval(documentId, false)); | ||
| yield fork(ChatSagas.handleReplayIfRequired, { documentId, action, dispatch, meta }); |
There was a problem hiding this comment.
So, this is dispatching on the webchat store. Put automatically dispatches to our emulator store. So, thats why I'm using call on the dispatch i get from the middleware
| public static *watchForWcEvents() { | ||
| const wcEventChannel = ChatSagas.wcActivityChannel.getWebchatChannelSubscriber(); | ||
| while (true) { | ||
| const { documentId, action, dispatch, meta }: ChannelPayload = yield take(wcEventChannel); |
There was a problem hiding this comment.
Dispatch I receive from webchat middleware
| yield* throwErrorFromResponse('Error occurred while retrieving the web socket port', res); | ||
| throw new Error( | ||
| `Error occurred while retrieving the WebSocket server port: ${res.status}: ${res.statusText || | ||
| 'No status text'}` | ||
| ); |
There was a problem hiding this comment.
Nice catch.. Effects of resolving the Merge conflict
| export interface WebChatActivityChannel { | ||
| sendWcEvents: (args: ChannelPayload) => void; | ||
| getWebchatChannelSubscriber: () => Channel<ChannelPayload>; | ||
| } | ||
|
|
||
| export interface WebchatEventPayload { |
There was a problem hiding this comment.
WebChat is what I'll go with. Reason being there are multiple usages like createWebChatStore etc written before. I'll probably stick to that pattern. Though, i think Webchat semantically makes more sense (https://github.com/microsoft/BotFramework-webchat)
| requireNewUserId: boolean = false | ||
| requireNewUserId: boolean = false, | ||
| activity: Activity = undefined, | ||
| createObjectUrl: Function = undefined |
There was a problem hiding this comment.
So the reason was I just thought accessing the window object directly inside chatSaga might not be a good idea. Rather injecting it as a dependacy would help us with unit testing as well.
| requireNewUserId: boolean = false | ||
| requireNewUserId: boolean = false, | ||
| activity: Activity = undefined, | ||
| createObjectUrl: Function = undefined |
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> get activities for a conversation Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Return updated activity Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Adding declaration maps for inspection Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Conversation routes set for fetch activities Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Safe commit Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Queues updated Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Safe commit Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Safe saga commit Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Resart multiple times the same conversation working. Major landmark Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Stable replay Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Restart from specific activity working Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> UI updates Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Safe saga flow Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Safe replay Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Forked post activity to let moving on to next activity Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Blocking webchat Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Simenatous conversations complete Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Error notficaition viewer completed Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Handling Dlspeech bot sniffer Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Conversation service spec completed Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Post activity test Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Mount conversation routes test Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Spec files updated Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Tests wrapped up for conversation queue Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Progressive response handling Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Stable commit to replay chat Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> UI tests restored to normality Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Tests working for conversationQueue Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Chatsagas stable tests Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> one more test working Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> All tests passing Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Chat saga test wrapped up Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> Restart conversation queue tests completed Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> UI tests updated Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> More UI tests Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Renaming for consistent webchat captialized handling Renaming functions Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com> More PR feedback handled Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
srinaath
force-pushed
the
srravich/feature/2039-restart-conversation
branch
from
fbbb974
to
6ae438e
Compare
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
thanks Tony |
srinaath
changed the title
Fixes #2039
This PR adds the capability to replay activities in a conversation. It allows multiple conversations being replayed at the same time.