-
Notifications
You must be signed in to change notification settings - Fork 4
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
Bug/93503 improve progressive rendering #79
Conversation
50f8ae7
to
2f61aa0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
The animation works as mentioned in the success criteria |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Code changes look good to me |
I added a comment in chat components linking here |
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.
Pull Request Overview
This PR addresses Bug/93503 by improving progressive rendering and updating related state management.
- Restructures the messages state from an array to an object containing messageHistory and visibleOutputMessages.
- Updates reducers, middlewares, and components to handle the new state structure and streaming message finish reasons.
- Adjusts Cypress commands and compatibility with legacy message formats.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/webchat/store/reducer.ts | Refactors messages state and updates SET_PREV_STATE to use the new messages structure. |
src/webchat/store/previous-conversations/previous-conversations-middleware.ts | Updates messages access to use messageHistory. |
src/webchat/store/options/options-middleware.ts | Fixes backward compatibility for persisted messages by extracting messageHistory. |
src/webchat/store/messages/message-reducer.ts | Updates the reducer for adding and animating messages, including support for finishReason and message collation. |
src/webchat/store/messages/message-middleware.ts | Adjusts middleware to work with the new messages structure. |
src/webchat/store/messages/helper.ts | Adds helper function for determining if a message is animated. |
src/webchat/store/autoinject/autoinject-middleware.ts | Changes messages access to use messageHistory for filtering. |
src/webchat/components/ConnectedWebchatUI.tsx | Connects visibleOutputMessages from state and updates props accordingly. |
src/webchat-ui/components/WebchatUI.tsx | Filters visible messages based on progressive rendering and visibleOutputMessages. |
src/common/interfaces/message.ts | Extends IStreamingMessage to include finishReason. |
cypress/support/commands.ts | Updates state access for messages in test commands. |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (3)
src/webchat/store/reducer.ts:81
- Resetting visibleOutputMessages to an empty array in the SET_PREV_STATE action may unintentionally discard previously stored visible message IDs. Consider preserving the existing visibleOutputMessages if appropriate.
const visibleOutputMessages = [];
src/webchat/store/messages/message-reducer.ts:109
- [nitpick] The variable 'newMessageId' is re-declared inside the block, shadowing an outer variable. Consider renaming the inner variable or restructuring the code to avoid shadowing.
const newMessageId = generateRandomId();
src/webchat/store/messages/message-reducer.ts:90
- [nitpick] Consider using consistent camelCase naming for clarity, for example 'isProgressiveMessageRenderingEnabled' instead of 'isprogressiveMessageRenderingEnabled'.
(!isOutputCollationEnabled && !isprogressiveMessageRenderingEnabled) ||
Please test together with PRs in Cognigy repo and chat-components repo (Cognigy/chat-components#116).
Follow the PR description here: https://cognigy.visualstudio.com/Cognigy.AI/_git/cognigy/pullrequest/41098
before we merge this, we need to first merge and publish and install the chat-component changes here