Skip to content

fix(chat-messages): improve chat container layout and examples#1810

Open
Killusions wants to merge 1 commit intomainfrom
fix/chat-container-improvements
Open

fix(chat-messages): improve chat container layout and examples#1810
Killusions wants to merge 1 commit intomainfrom
fix/chat-container-improvements

Conversation

@Killusions
Copy link
Copy Markdown
Member

@Killusions Killusions commented Apr 2, 2026

  • add max-inline-size constraint to message bubble and wrapper to prevent
    overflow
  • fix attachment preview backdrop click behavior
  • use index-based tracking for message list and
    add scroll-to-bottom on message send
  • update example action labels for clarity

Documentation.
Examples.
Dashboards Demo.
Playwright report.

Coverage Reports:

Code Coverage

- add max-inline-size constraint to message bubble and wrapper to prevent
  overflow
- fix attachment preview backdrop click behavior
- use index-based tracking for message list and
  add scroll-to-bottom on message send
- update example action labels for clarity
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request renames chat actions from 'Copy' to 'Export' and 'Good response' to 'Add to list,' updates message bubble styling with max-inline-size, and adds automatic scrolling logic. Feedback suggests merging redundant CSS selectors and reverting the @for loop tracking to use the message object instead of $index to prevent re-rendering issues. Furthermore, log messages should be updated for terminology consistency, and a redundant setTimeout for scrolling should be removed.

Comment on lines 52 to 55
.message-wrapper {
min-inline-size: 0;
max-inline-size: 100%;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The .message-wrapper selector is defined again on line 93. To maintain a clean and organized stylesheet, it is recommended to merge these definitions. You can move the max-inline-size: 100% property to the block starting at line 93 and remove this redundant block.

<si-chat-container colorVariant="base-0">
@for (message of messages(); track message) {
<si-chat-container #chatContainer colorVariant="base-0">
@for (message of messages(); track $index) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using $index for tracking in @for is generally discouraged when items can be removed from the list (as seen with the 'Delete message' action on line 187 of the component). Tracking by the message object itself is more efficient in this case, as it prevents unnecessary re-renders of subsequent items when a message is deleted from the middle of the conversation.

Suggested change
@for (message of messages(); track $index) {
@for (message of messages(); track message) {

label: 'Export message',
icon: 'element-export',
action: (message: ChatMessage) =>
this.logEvent(`Copy user message ${message.content.slice(0, 20)}...`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The action label was updated to 'Export message', but the log message still refers to 'Copy'. It is recommended to update the log message to maintain consistency with the UI labels, following the Siemens UX writing guidelines for consistent terminology.

Suggested change
this.logEvent(`Copy user message ${message.content.slice(0, 20)}...`)
this.logEvent('Export user message ' + message.content.slice(0, 20) + '...')
References
  1. Use the same words and grammatical forms, lengths, and styles repeatedly to ensure consistency across the UI. (link)

label: 'Export message',
icon: 'element-export',
action: (_message: ChatMessage) =>
this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The action label was updated to 'Export message', but the log message still refers to 'Copy'.

Suggested change
this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`)
this.logEvent('Export user message ' + _message.content.slice(0, 20) + '...')
References
  1. Use the same words and grammatical forms, lengths, and styles repeatedly to ensure consistency across the UI. (link)

label: 'Export message',
icon: 'element-export',
action: (_message: ChatMessage) =>
this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The action label was updated to 'Export message', but the log message still refers to 'Copy'.

Suggested change
this.logEvent(`Copy user message ${_message.content.slice(0, 20)}...`)
this.logEvent('Export user message ' + _message.content.slice(0, 20) + '...')
References
  1. Use the same words and grammatical forms, lengths, and styles repeatedly to ensure consistency across the UI. (link)

Comment on lines +288 to +290
setTimeout(() => {
this.chatContainer()?.scrollToBottom();
}, 0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The setTimeout is redundant here. SiChatContainerComponent already implements a MutationObserver that automatically scrolls to the bottom when content changes, provided that isUserAtBottom is true. Calling this.chatContainer()?.scrollToBottom() synchronously is sufficient to set the internal state and ensure the container scrolls once the new message is rendered in the DOM.

    this.chatContainer()?.scrollToBottom();
References
  1. Avoid using asynchronous schedulers or setTimeout if the state being checked is updated synchronously by the same event.

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.

2 participants