Repository navigation
feat(core/chat): add new chat components - #2569
Conversation
🦋 Changeset detectedLatest commit: 3c11bd6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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;
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/components/chat-input/chat-input.tsx (3)
536-538:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the action button semantics in sync with component state.
When
state === 'processing', the icon changes to a stop glyph, but the control still exposesaria-label="Submit prompt", anddisabledSubmitButtonstops honoringdisabled/readonly. That leaves a disabled chat input with a focusable action button and the wrong accessible name.Also applies to: 601-605
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/chat-input/chat-input.tsx` around lines 536 - 538, The action button's enabled/aria semantics must follow the component state: update the disabledSubmitButton calculation to also disable the button when this.state === 'processing' (not just !this.canSubmit() && this.state === 'input'), and ensure submitButtonIcon and the button's accessible name/aria-label change when processing (e.g., set aria-label to "Stop processing" or similar when this.state === 'processing'); modify uses around disabledSubmitButton, submitButtonIcon, and the button render (including the code mirrored at 601-605) so the control is non-focusable/disabled and has the correct aria-label while processing. Ensure references to canSubmit(), submitButtonIcon, and this.state are updated consistently.
490-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the mutation observer to slot-relevant changes.
This observer listens to every attribute mutation in the host subtree. Because
valueis reflected, normal typing can retriggerupdateHasFollowUp()and attachment scrollbar measurement on each keystroke, which pushes unnecessary layout work onto the main input path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/chat-input/chat-input.tsx` around lines 490 - 499, The mutation observer in initAttachmentMutationObserver is too broad and reacts to reflected 'value' attribute changes; narrow it so typing doesn't retrigger updateHasFollowUp() and scheduleAttachmentScrollbarUpdate(). Modify the observe call on attachmentMutationObserver to either target the specific attachment slot/container node instead of this.hostElement, or disable attribute watching and rely on childList/subtree only, or supply attributeFilter that excludes 'value' (e.g., attributes: true with attributeFilter: ['class','style', ...] but not 'value'); keep the callback and method names (updateHasFollowUp, scheduleAttachmentScrollbarUpdate, attachmentMutationObserver, initAttachmentMutationObserver) unchanged.
247-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument or de-publicize
getAssociatedFormElement().
@Method()makes this part of the element API, so/**@internal*/is misleading and the method is missing the required@sincemetadata. Either add the public JSDoc or remove@Method()if this is only for internal/test use. As per coding guidelines, "For new components, properties, methods, or events, include JSDocs with a@sincetag indicating the version number in which it was added."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/chat-input/chat-input.tsx` around lines 247 - 251, The method getAssociatedFormElement is currently exposed via the `@Method`() decorator but marked /** `@internal` */ without `@since` metadata; either document it as a public API by replacing the internal JSDoc with a proper public JSDoc including a `@since` tag and description, or de-publicize it by removing the `@Method`() decorator so it remains internal (keeping the implementation that returns this.formInternals.form). Update the JSDoc and/or decorator accordingly to match the intended API surface.Source: Coding guidelines
packages/core/src/components/chat-input/tests/chat-input.ct.ts (1)
29-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required accessibility and hydration smoke coverage.
This new
.ct.tssuite never runs an axe scan, has no'renders'test, and starts querying/interacting withix-chat-inputimmediately aftermount()without first waiting for\bhydrated\b. That misses the repo’s required baseline coverage and makes the interaction tests more race-prone. As per coding guidelines, "Every test file should include an 'accessibility' test usingmakeAxeBuilder", "Every test file should include a 'renders' test verifying basic hydration", and "Always verify components are hydrated before making further assertions usingawait expect(page.locator('ix-component')).toHaveClass(/\bhydrated\b/)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/chat-input/tests/chat-input.ct.ts` around lines 29 - 383, Add the required hydration "renders" test and an accessibility/axe scan, and ensure every test waits for hydration before interacting: after mounting use await expect(page.locator('ix-chat-input')).toHaveClass(/\bhydrated\b/) (used throughout) and add a regressionTest named 'renders' that mounts the component and asserts hydrated, plus an 'accessibility' regressionTest that uses makeAxeBuilder(page).include('ix-chat-input')/analyze to run axe on the component; update other tests to wait for hydration immediately after mount before querying or interacting with the element (references: regressionTest, makeAxeBuilder, and the hydration assertion expect(...).toHaveClass(/\bhydrated\b/)).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/components/chat-input/chat-input.tsx`:
- Around line 536-538: The action button's enabled/aria semantics must follow
the component state: update the disabledSubmitButton calculation to also disable
the button when this.state === 'processing' (not just !this.canSubmit() &&
this.state === 'input'), and ensure submitButtonIcon and the button's accessible
name/aria-label change when processing (e.g., set aria-label to "Stop
processing" or similar when this.state === 'processing'); modify uses around
disabledSubmitButton, submitButtonIcon, and the button render (including the
code mirrored at 601-605) so the control is non-focusable/disabled and has the
correct aria-label while processing. Ensure references to canSubmit(),
submitButtonIcon, and this.state are updated consistently.
- Around line 490-499: The mutation observer in initAttachmentMutationObserver
is too broad and reacts to reflected 'value' attribute changes; narrow it so
typing doesn't retrigger updateHasFollowUp() and
scheduleAttachmentScrollbarUpdate(). Modify the observe call on
attachmentMutationObserver to either target the specific attachment
slot/container node instead of this.hostElement, or disable attribute watching
and rely on childList/subtree only, or supply attributeFilter that excludes
'value' (e.g., attributes: true with attributeFilter: ['class','style', ...] but
not 'value'); keep the callback and method names (updateHasFollowUp,
scheduleAttachmentScrollbarUpdate, attachmentMutationObserver,
initAttachmentMutationObserver) unchanged.
- Around line 247-251: The method getAssociatedFormElement is currently exposed
via the `@Method`() decorator but marked /** `@internal` */ without `@since` metadata;
either document it as a public API by replacing the internal JSDoc with a proper
public JSDoc including a `@since` tag and description, or de-publicize it by
removing the `@Method`() decorator so it remains internal (keeping the
implementation that returns this.formInternals.form). Update the JSDoc and/or
decorator accordingly to match the intended API surface.
In `@packages/core/src/components/chat-input/tests/chat-input.ct.ts`:
- Around line 29-383: Add the required hydration "renders" test and an
accessibility/axe scan, and ensure every test waits for hydration before
interacting: after mounting use await
expect(page.locator('ix-chat-input')).toHaveClass(/\bhydrated\b/) (used
throughout) and add a regressionTest named 'renders' that mounts the component
and asserts hydrated, plus an 'accessibility' regressionTest that uses
makeAxeBuilder(page).include('ix-chat-input')/analyze to run axe on the
component; update other tests to wait for hydration immediately after mount
before querying or interacting with the element (references: regressionTest,
makeAxeBuilder, and the hydration assertion
expect(...).toHaveClass(/\bhydrated\b/)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94a290fd-805b-4df7-83b0-6c5d8859e8d8
📒 Files selected for processing (7)
packages/angular/src/components.tspackages/angular/standalone/src/components.tspackages/core/src/components.d.tspackages/core/src/components/chat-input/chat-input.tsxpackages/core/src/components/chat-input/tests/chat-input.ct.tspackages/react/src/components/components.server.tspackages/vue/src/components/ix-chat-input.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/chat-input/tests/chat-input.ct.ts (1)
75-75: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winFollow defensive class-matching patterns to prevent partial matches.
Several class assertions do not follow the required patterns from the coding guidelines:
- Non-hyphenated classes (e.g.,
disabled) should use word boundaries:/\bdisabled\b/- Hyphenated classes (e.g.,
has-attachments,character-limit--hard) should use negative lookahead:/has-attachments(?!-)/Current patterns like
/has-attachments/would incorrectly matchhas-attachments-overflowif such a class were added later.Affected lines:
- Line 75:
/disabled/→/\bdisabled\b/- Line 244:
/character-limit--hard/→/character-limit--hard(?!-)/- Line 285:
/has-attachments/→/has-attachments(?!-)/- Line 287:
/attachments--scroll/→/attachments--scroll(?!-)/- Lines 291, 317, 325, 349:
/has-attachment-scrollbar/→/has-attachment-scrollbar(?!-)/- Line 374:
/has-follow-up/→/has-follow-up(?!-)/As per coding guidelines: "Use word boundary
\bin regex patterns to avoid false matches when checking classes likehydrated" and "Use hyphenated class name assertions with negative lookahead to avoid partial matches (e.g.,overflow-shadowmust not matchoverflow-shadow-container)".Also applies to: 244-244, 285-291, 317-317, 325-325, 349-349, 374-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/chat-input/tests/chat-input.ct.ts` at line 75, Update the class-assertion regexes to use defensive patterns: replace /disabled/ with /\bdisabled\b/ for non-hyphenated classes and replace hyphenated checks like /has-attachments/, /character-limit--hard/, /attachments--scroll/, /has-attachment-scrollbar/, /has-follow-up/ with negative-lookahead forms (e.g., /has-attachments(?!-)/, /character-limit--hard(?!-)/, etc.) in the chat-input.ct.ts tests — locate the assertions using variables like submitButton and other class checks in the file and swap the raw substring regexes for the appropriate /\b...\b/ or ...(?!-)/ forms to avoid partial matches.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/chat-input/chat-input.tsx`:
- Around line 536-541: handleKeyDown currently allows Enter to call submitPrompt
even when the component is processing; update the handler (handleKeyDown) to
early-return or skip invoking submitPrompt when this.state === 'processing' (in
addition to existing canSubmit() checks) so keyboard submission is blocked
during processing and duplicate promptSubmit events cannot be emitted.
---
Outside diff comments:
In `@packages/core/src/components/chat-input/tests/chat-input.ct.ts`:
- Line 75: Update the class-assertion regexes to use defensive patterns: replace
/disabled/ with /\bdisabled\b/ for non-hyphenated classes and replace hyphenated
checks like /has-attachments/, /character-limit--hard/, /attachments--scroll/,
/has-attachment-scrollbar/, /has-follow-up/ with negative-lookahead forms (e.g.,
/has-attachments(?!-)/, /character-limit--hard(?!-)/, etc.) in the
chat-input.ct.ts tests — locate the assertions using variables like submitButton
and other class checks in the file and swap the raw substring regexes for the
appropriate /\b...\b/ or ...(?!-)/ forms to avoid partial matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19f80e9e-b956-4b46-babc-a6249274b2ea
📒 Files selected for processing (2)
packages/core/src/components/chat-input/chat-input.tsxpackages/core/src/components/chat-input/tests/chat-input.ct.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/chat-input/chat-input.tsx (1)
415-429: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for Enter during
processing.This fixes the original bug, but the supplied component tests only cover Enter submission and the processing state separately. Please add a case that sets
state="processing", presses Enter, and asserts thatpromptSubmitis not emitted, so this path cannot regress.🧪 Suggested test shape
regressionTest( 'ix-chat-input does not emit promptSubmit on Enter while processing', async ({ mount, page }) => { await mount( '<ix-chat-input state="processing" value="Analyze alarms"></ix-chat-input>' ); await page.evaluate(() => { globalThis.__promptSubmitValue = undefined; document .querySelector('ix-chat-input') ?.addEventListener('promptSubmit', ((event: CustomEvent<string>) => { globalThis.__promptSubmitValue = event.detail; }) as EventListener); }); const textarea = page.locator('ix-chat-input').locator('textarea'); await textarea.focus(); await textarea.press('Enter'); expect(await page.evaluate(() => globalThis.__promptSubmitValue)).toBe( undefined ); } );As per coding guidelines, "Prioritize correctness, regressions, accessibility, release impact, and missing validation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/chat-input/chat-input.tsx` around lines 415 - 429, Add a regression test that verifies ix-chat-input in the "processing" state ignores Enter: create a test named like "ix-chat-input does not emit promptSubmit on Enter while processing" that mounts '<ix-chat-input state="processing" value="..."></ix-chat-input>', attaches a listener to the promptSubmit event (storing event.detail on globalThis for inspection), focuses the textarea, simulates pressing Enter, and asserts that the stored global value remains undefined; this ensures the behavior guarded by handleKeyDown (which checks this.state === 'processing' and calls submitPrompt only when appropriate) does not regress.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/components/chat-input/chat-input.tsx`:
- Around line 415-429: Add a regression test that verifies ix-chat-input in the
"processing" state ignores Enter: create a test named like "ix-chat-input does
not emit promptSubmit on Enter while processing" that mounts '<ix-chat-input
state="processing" value="..."></ix-chat-input>', attaches a listener to the
promptSubmit event (storing event.detail on globalThis for inspection), focuses
the textarea, simulates pressing Enter, and asserts that the stored global value
remains undefined; this ensures the behavior guarded by handleKeyDown (which
checks this.state === 'processing' and calls submitPrompt only when appropriate)
does not regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 921832be-336a-4547-9aa1-5aa574f6d61d
⛔ Files ignored due to path filters (2)
testing/visual-testing/__screenshots__/tests/chat/chat.e2e.ts/chat-overview-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/chat/chat.e2e.ts/chat-overview-1-chromium---classic-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
packages/core/src/components/chat-input/chat-input.tsx
|
|
💡 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
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests