-
Notifications
You must be signed in to change notification settings - Fork 750
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
Restart Conversation from any point Functionality #2089
Conversation
@@ -107,7 +106,7 @@ const defaultConfig = { | |||
], | |||
}, | |||
|
|||
devtool: 'source-map', | |||
devtool: 'eval-source-map', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
export interface WebChatActivityChannel { | ||
sendWcEvents: (args: ChannelPayload) => void; | ||
getWebchatChannelSubscriber: () => Channel<ChannelPayload>; | ||
} | ||
|
||
export interface WebchatEventPayload { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
align on capitalization pattern for Web Chat:
WebChatActivityChannel
WebchatEventPayload
is the C
capitalized or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async
not necessary
speechKey?: string; | ||
speechRegion?: string; | ||
user: User; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep user
here -- sorted alphabetically
meta.conversationQueue.getNextActivityForPost, | ||
]); | ||
if (postActivity) { | ||
yield call(dispatchActivityToWebchat, dispatch, postActivity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you aren't using put
here instead of calling dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented below
} finally { | ||
yield put(updatePendingSpeechTokenRetrieval(documentId, false)); | ||
yield fork(ChatSagas.handleReplayIfRequired, { documentId, action, dispatch, meta }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does dispatch
need to be passed in if we have access to put
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change back to throwErrorFromResponse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.. Effects of resolving the Merge conflict
meta.conversationQueue.getNextActivityForPost, | ||
]); | ||
if (postActivity) { | ||
yield call(dispatchActivityToWebchat, dispatch, postActivity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented below
} finally { | ||
yield put(updatePendingSpeechTokenRetrieval(documentId, false)); | ||
yield fork(ChatSagas.handleReplayIfRequired, { documentId, action, dispatch, meta }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do u think?
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>
fbbb974
to
6ae438e
Compare
Signed-off-by: Srinaath Ravichandran <srravich@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Great job 😃
thanks Tony |
Fixes #2039
This PR adds the capability to replay activities in a conversation. It allows multiple conversations being replayed at the same time.