feat(core/chat): add new chat components#2569
Conversation
🦋 Changeset detectedLatest commit: b7e605b The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the new ix-chat component suite for AI prompt entry and conversation layouts, including ix-chat-input, ix-chat-prompt-attachment, ix-chat-user-message, and ix-chat-ai-message. It also includes a fix for ix-chip to ensure interactive close buttons render correctly when the chip is inactive. The review identified several critical issues in the new chat components, including missing lifecycle handling for observers on component re-attachment, incorrect state handling for the submit button during processing, premature form submission during IME composition, a missing key prop in dynamic lists, potential runtime TypeErrors with null/undefined file names, and a regression in the ix-chip pointer-events styling.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/components/chat-input/chat-input.tsx (229-244)
The component does not implement connectedCallback to re-initialize the MutationObserver and ResizeObserver when the component is disconnected and re-attached to the DOM. This causes the observers to remain disconnected and breaks the attachment overflow and follow-up updates on re-attachment. Implementing connectedCallback with a guard flag ensures proper re-initialization.
private hasDisconnected = false;
connectedCallback() {
if (this.hasDisconnected) {
this.initAttachmentMutationObserver();
if (this.attachmentsRef.current) {
this.initAttachmentResizeObserver(this.attachmentsRef.current);
}
this.hasDisconnected = false;
}
}
componentDidLoad() {
this.updateHasFollowUp();
this.initAttachmentMutationObserver();
this.scheduleAttachmentOverflowUpdate();
this.updateTextareaHeight();
}
componentDidRender() {
this.scheduleAttachmentOverflowUpdate();
this.updateTextareaHeight();
}
disconnectedCallback() {
this.attachmentResizeObserver?.disconnect();
this.attachmentMutationObserver?.disconnect();
this.hasDisconnected = true;
}
packages/core/src/components/chat-input/chat-input.tsx (292-315)
When state === 'processing', the submit button acts as a "Stop" button. However, canSubmit() does not check this.state, which means if the input is empty, the "Stop" button is disabled and cannot be clicked. Furthermore, if the input is not empty, clicking it or pressing Enter will submit the prompt again instead of stopping the processing. We should ensure the button is enabled during processing and handles the stop action correctly.
private canSubmit() {
return (
!this.disabled &&
!this.readonly &&
(this.state === 'processing' ||
((this.value || '').trim().length > 0 &&
!this.isCharacterLimitReached()))
);
}
private emitIxChangeIfNeeded() {
if (this.initialValue !== this.value) {
this.ixChange.emit(this.value);
this.initialValue = this.value;
}
}
private submitPrompt() {
if (this.state === 'processing') {
this.promptSubmit.emit('');
return;
}
if (!this.canSubmit()) {
return;
}
this.emitIxChangeIfNeeded();
this.promptSubmit.emit(this.value);
}
packages/core/src/components/chat-input/chat-input.tsx (420-433)
Pressing Enter while composing text in an IME (Input Method Editor) will trigger the keydown event and submit the prompt prematurely. We should check event.isComposing to prevent this.
private handleKeyDown(event: KeyboardEvent) {
if (
event.key !== 'Enter' ||
event.isComposing ||
event.shiftKey ||
event.altKey ||
event.ctrlKey ||
event.metaKey ||
this.insertLineBreakOnEnter
) {
return;
}
event.preventDefault();
this.submitPrompt();
}
packages/core/src/components/chat-input/chat-input.tsx (784-811)
The dynamic list rendered by this.attachmentOverflowEntries.map is missing a key prop on the outer element. This can cause rendering and reconciliation issues when attachments are dynamically added or removed.
private renderAttachmentOverflowItems() {
return this.attachmentOverflowEntries.map((entry) => (
<div
key={entry.index}
class="attachment-overflow-menu-item"
data-attachment-overflow-generated
data-attachment-overflow-label={entry.label}
>
<ix-dropdown-item
class="attachment-overflow-generated-item"
icon={entry.icon}
ariaLabelButton={entry.label}
onClick={() => this.handleAttachmentOverflowItemClick(entry.index)}
>
{entry.label}
</ix-dropdown-item>
{entry.canRemove && (
<ix-icon-button
aria-label={entry.removeAriaLabel}
class="attachment-overflow-remove-button"
icon={iconCloseSmall}
size="24"
variant="subtle-tertiary"
onClick={(event: MouseEvent) =>
this.handleAttachmentOverflowRemoveClick(event, entry.index)
}
></ix-icon-button>
)}
</div>
));
}
packages/core/src/components/chat-prompt-attachment/chat-prompt-attachment.tsx (75-87)
If this.fileName is null or undefined, calling this.fileName.trim() will throw a runtime TypeError. We should guard against null/undefined values to ensure robust defensive programming.
private splitFileName() {
const fileName = (this.fileName || '').trim();
const extensionStart = fileName.lastIndexOf('.');
if (extensionStart <= 0 || extensionStart === fileName.length - 1) {
return { name: fileName, extension: '' };
}
return {
name: fileName.slice(0, extensionStart),
extension: fileName.slice(extensionStart),
};
}
packages/core/src/components/chip/chip.scss (74-76)
Removing pointer-events: none from :host(.inactive) and only applying it to .chip-main allows click events on the chip body to bubble up to the host. This means any click listeners on ix-chip will still be triggered even when the chip is inactive. A cleaner CSS-only solution is to keep pointer-events: none on :host(.inactive) and explicitly set pointer-events: auto on .chip-close.
:host(.inactive) {
pointer-events: none;
.chip-close {
pointer-events: auto;
}
}
|
1 participant
💡 What is the current behavior?
JIRA: IX-4307
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support