-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix streaming behavior by capturing typingActivityId in nextActivityIdToKeyMap #5609
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
base: main
Are you sure you want to change the base?
Fix streaming behavior by capturing typingActivityId in nextActivityIdToKeyMap #5609
Conversation
…her than channelData
This results in a single combined message rather than 3 separate messages
….com/benbrown/BotFramework-WebChat into users/benbro/fix-streaming-behavior
'textContent', | ||
'Adipisicing cupidatat eu Lorem anim ut aute magna occaecat id cillum.' | ||
); | ||
expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox jumped over'); |
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.
expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox jumped over'); | |
await waitFor(() => expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox jumped over')); |
This probably need waitFor()
to eliminate test flaky.
'textContent', | ||
'Adipisicing cupidatat eu Lorem anim ut aute magna occaecat id cillum.' | ||
); | ||
expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox'); |
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.
expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox'); | |
await waitFor(() => expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox')); |
Probably need waitFor()
to eliminate test flaky.
expect(pageElements.activityContents()[1]).toHaveProperty( | ||
'textContent', | ||
'A quick brown fox jumped over the lazy dogs.' | ||
); |
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.
expect(pageElements.activityContents()[1]).toHaveProperty( | |
'textContent', | |
'A quick brown fox jumped over the lazy dogs.' | |
); | |
await waitFor(() => | |
expect(pageElements.activityContents()[1]).toHaveProperty( | |
'textContent', | |
'A quick brown fox jumped over the lazy dogs.' | |
) | |
); |
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.
Maybe we need to pause the animation, or maybe it is already paused when running in WebDriver.
In case pause is needed, set accessibility "reduced motion" in Dev Tools.
'textContent', | ||
'Adipisicing cupidatat eu Lorem anim ut aute magna occaecat id cillum.' | ||
); | ||
expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox jumped over'); |
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.
expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox jumped over'); | |
await waitFor(() => expect(pageElements.activityContents()[1]).toHaveProperty('textContent', 'A quick brown fox jumped over')); |
expect(currentActivityKeysWithId).toEqual([ | ||
[firstActivityKey, ['a-00001']], | ||
[secondActivityKey, ['t-00001', 't-00002', 't-00003']] | ||
]); |
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.
expect(currentActivityKeysWithId).toEqual([ | |
[firstActivityKey, ['a-00001']], | |
[secondActivityKey, ['t-00001', 't-00002', 't-00003']] | |
]); | |
await waitFor(() => | |
expect(currentActivityKeysWithId).toEqual([ | |
[firstActivityKey, ['a-00001']], | |
[secondActivityKey, ['t-00001', 't-00002', 't-00003']] | |
]) | |
); |
expect(pageElements.activityContents()[1]).toHaveProperty( | ||
'textContent', | ||
'A quick brown fox jumped over the lazy dogs.' | ||
); |
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.
expect(pageElements.activityContents()[1]).toHaveProperty( | |
'textContent', | |
'A quick brown fox jumped over the lazy dogs.' | |
); | |
await waitFor(() => | |
expect(pageElements.activityContents()[1]).toHaveProperty( | |
'textContent', | |
'A quick brown fox jumped over the lazy dogs.' | |
) | |
); |
|
||
// THEN: Should have no typing. | ||
expect(currentActiveTyping).toHaveProperty('u-00001', 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.
For completeness, let's try sending another chunk after the livestream is concluded/finalized.
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 that different than the behavior you'd see with the channelData stream?
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.
You made a good point. Do we have every test against channelData
ported to test against entities
? Once we deprecated channelData
, we will remove those tests and the fortification will be gone.
We can do permutation inside the tests.
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, also few things need to be answered by Eugene.
Description
This fixes the streaming behavior where new interim messages will replace the old, appear to append the message as it streams in. This is a follow-up to #5517 which initially introduced support for parsing and processing those entities.
However, this small additional change was required for the rendering process to properly find the existing message and update it rather than insert a new message.
Specific Changes
This fix was achieved by capturing the typingActivityId along with the main activityId in the nextActivityToId map such that it can be found the second time around.
In addition, added html2/livestream/entities.*, a selection of tests that demonstrate that the behavior with entities matches the behavior with channelData.
-
CHANGELOG.md
Review Checklist
z-index
)package.json
andpackage-lock.json
reviewed