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

Bug/93503 improve progressive rendering #79

Merged
merged 10 commits into from
Mar 25, 2025

Conversation

dshire
Copy link
Member

@dshire dshire commented Mar 18, 2025

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

@dshire dshire force-pushed the bug/93503-improve-progressive-rendering branch from 50f8ae7 to 2f61aa0 Compare March 20, 2025 10:54
@sushmi21

This comment was marked as resolved.

@sushmi21
Copy link
Contributor

The animation works as mentioned in the success criteria

@sushmi21

This comment was marked as resolved.

@dshire

This comment was marked as resolved.

@sushmi21

This comment was marked as outdated.

@dshire

This comment was marked as outdated.

@sushmi21

This comment was marked as outdated.

@sushmi21

This comment was marked as resolved.

@dshire

This comment was marked as resolved.

@sushmi21

This comment was marked as resolved.

@sushmi21

This comment was marked as outdated.

@dshire

This comment was marked as outdated.

@sushmi21

This comment was marked as resolved.

@sushmi21
Copy link
Contributor

Code changes look good to me

sushmi21
sushmi21 previously approved these changes Mar 25, 2025
@vj-venkatesan
Copy link
Contributor

I added a comment in chat components linking here

Cognigy/chat-components#116 (review)

vj-venkatesan
vj-venkatesan previously approved these changes Mar 25, 2025
@dshire dshire dismissed stale reviews from vj-venkatesan and sushmi21 via 1db138f March 25, 2025 15:11
@dshire dshire merged commit 2c0b1d0 into main Mar 25, 2025
5 of 6 checks passed
@kwinto kwinto requested a review from Copilot March 28, 2025 09:00
Copy link
Contributor

@Copilot Copilot AI left a 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) ||

@vj-venkatesan vj-venkatesan mentioned this pull request Mar 28, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants