Skip to content

feat(chat): Add the ability to support content parts in the chat (Issue #3769) #3777

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

Draft
wants to merge 8 commits into
base: development
Choose a base branch
from

Conversation

denys-kolomiitsev
Copy link
Contributor

@denys-kolomiitsev denys-kolomiitsev commented May 7, 2025

Description:

Add the ability to support content parts in the chat

Issues:

Related:

Checklist:

  • the pull request name complies with Conventional Commits
  • the pull request name starts with fix(<scope>):, feat(<scope>):, feature(<scope>):, chore(<scope>):, hotfix(<scope>): or e2e(<scope>):. If contains breaking changes then the pull request name must start with fix(<scope>)!:, feat(<scope>)!:, feature(<scope>)!:, chore(<scope>)!:, hotfix(<scope>)!: or e2e(<scope>)!: where <scope> is name of affected project: chat, chat-e2e, overlay, shared, sandbox-overlay, etc.
  • the pull request name ends with (Issue #<TICKET_ID>) (comma-separated list of issues)
  • I confirm that do not share any confidential information like API keys or any other secrets and private URLs

@denys-kolomiitsev
Copy link
Contributor Author

denys-kolomiitsev commented May 7, 2025

/deploy-review

@denys-kolomiitsev
Copy link
Contributor Author

denys-kolomiitsev commented May 7, 2025

/deploy-review

Environment: review-environment | pipeline
Overlay E2E tests status: failed
Chat E2E tests status: failed

@denys-kolomiitsev denys-kolomiitsev marked this pull request as draft May 8, 2025 09:41
@denys-kolomiitsev denys-kolomiitsev marked this pull request as ready for review May 8, 2025 11:07
@denys-kolomiitsev
Copy link
Contributor Author

denys-kolomiitsev commented May 8, 2025

/deploy-review

Environment: review-environment | pipeline
Overlay E2E tests status: success
Chat E2E tests status: success

@denys-kolomiitsev denys-kolomiitsev enabled auto-merge (squash) May 8, 2025 11:58
@denys-kolomiitsev
Copy link
Contributor Author

denys-kolomiitsev commented May 8, 2025

/deploy-review

Environment: review-environment | pipeline
Overlay E2E tests status: success
Chat E2E tests status: success

})}

{/* Keep support old render for backward compatibility */}
{!!(message.content || (!message.parts && isShowResponseLoader)) && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have content and parts together?
according to this code we can render both

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It could be either content or parts, but we need to have compatibility with old chats which have only content and if user wants to continue the chat.

content += part.content;
}
});
return content;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return content;
return content.trim();

let content = '';
parts.forEach((part) => {
if (part.content) {
content += part.content;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use any separator here?

const lastIndex = newSource.parts.length - 1;
if (newSource.parts[lastIndex]?.content) {
const newContent: ContentPart = {
partId: nanoid(4),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you update partId here? if this part already have content that means that partId already was generated and need to update only content

newSource.parts[lastIndex].content += newData.content;


if (newSource.parts[lastIndex]?.attachments) {
const newAttachments = {
partId: nanoid(4),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same. See previous comment

Comment on lines +59 to +60
content?: string;
attachments?: Attachment[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose type should be { partId:string } & ({ content: string } | { attachments: Attachment[] }) because one kind of payload is required here (XOR)

@@ -67,10 +68,24 @@ export const mergeMessages = (
}

if (newData.content) {
if (!newSource.content) {
newSource.content = '';
if (!newSource.parts) {
Copy link
Collaborator

@IlyaBondar IlyaBondar May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check some flag to start putting the content into parts? It looks like now we started to use parts always without conditions

@denys-kolomiitsev denys-kolomiitsev marked this pull request as draft May 9, 2025 10:17
auto-merge was automatically disabled May 9, 2025 10:17

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants