UX improvement for chat window: Stop button, scroll bar fix and text box height increase#11266
Conversation
arjunkumargiri
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
d-buckner,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ps48,
raintygao,
ruanyl,
sejli,
virajsanghvi,
wanglam,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
❌ Invalid Prefix For Manual Changeset CreationInvalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files. If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR converts the chat input from a single-line text field to a multiline textarea with auto-resizing capabilities, adds stop streaming functionality with subscription management, implements smart auto-scroll behavior in chat messages, and provides comprehensive test coverage for these components. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ChatInput as ChatInput UI
participant ChatWindow as ChatWindow
participant ChatService as ChatService
participant Observable as Stream Observable
User->>ChatInput: Clicks Stop button (during streaming)
ChatInput->>ChatWindow: Calls onStop()
ChatWindow->>ChatService: Calls chatService.abort()
ChatService->>Observable: Triggers abort
Observable-->>ChatWindow: Unsubscribe
ChatWindow->>ChatWindow: Clears currentSubscriptionRef
ChatWindow->>ChatWindow: Sets isStreaming = false
ChatWindow->>ChatInput: Updates state (button switches to Send)
ChatInput-->>User: Displays Send button
sequenceDiagram
participant User as User
participant ChatMessages as ChatMessages Container
participant ScrollHandler as Scroll Event Handler
participant AutoScroll as Auto-Scroll Logic
User->>ChatMessages: Receives new message
ChatMessages->>ScrollHandler: Triggers scroll listener
ScrollHandler->>AutoScroll: Checks if user scrolled up
alt User near bottom or hasn't scrolled
AutoScroll->>ChatMessages: Scroll to bottom smoothly
ChatMessages-->>User: Message visible at bottom
else User scrolled up
AutoScroll->>ChatMessages: Skip auto-scroll
ChatMessages-->>User: Message appended but not scrolled into view
end
User->>ChatMessages: User scrolls down
ScrollHandler->>AutoScroll: Re-enables auto-scroll
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
Skip-Changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/plugins/chat/public/components/chat_input.test.tsx`:
- Around line 304-324: The tests fail because ChatInput binds handlers directly
via onClick={isStreaming ? onStop : onSend} and clicking when onStop/onSend is
undefined throws; update the ChatInput component (referencing the ChatInput
component and props onStop and onSend) to defensively call the callbacks (e.g.,
use a wrapper that checks for existence or provide no-op defaults) so the
button's click handler invokes onStop() or onSend() only if they are functions,
and add/update tests if you choose to enforce non-optional callbacks instead.
In `@src/plugins/chat/public/components/chat_window.tsx`:
- Around line 62-63: The stream stop path doesn't clear the "loading-"
placeholder because unsubscribing prevents error/complete callbacks; add a ref
to track the loading message id (e.g., loadingMessageIdRef alongside
currentSubscriptionRef) when you insert the "loading-" message, and ensure the
stop/unsubscribe handler (the function that cancels the current stream:
reference currentSubscriptionRef and the stop/unsubscribe function) explicitly
removes/clears that message id from the timeline state (call the existing
message removal/update logic) and null out the ref; also clear it in any
cleanup effects so the "Thinking…" placeholder is always removed when a stream
is stopped.
🧹 Nitpick comments (5)
src/plugins/chat/public/components/chat_input.test.tsx (3)
145-152: Test doesn't verify the stated behavior.The test is named "should show sortUp icon when not streaming" but it only checks that the button exists and is enabled. It doesn't actually verify the icon type.
Consider either renaming the test to match what it verifies, or adding actual icon verification:
💡 Option 1: Rename to match actual behavior
- it('should show sortUp icon when not streaming', () => { + it('should show enabled send button when not streaming with input', () => {💡 Option 2: Verify icon via data attribute or snapshot
If EUI exposes icon type through a data attribute or class, you could verify it directly. Otherwise, a snapshot test or visual regression test may be more appropriate for icon verification.
182-189: Test doesn't verify the stated behavior.Similar to the send button test, this test claims to verify "stop icon" but only checks button existence and enabled state.
💡 Rename to match actual behavior
- it('should show stop icon when streaming', () => { + it('should show enabled stop button when streaming', () => {
280-287: Autofocus test doesn't verify focus state.The test claims to verify autofocus but only checks that the input element exists. It doesn't actually verify that the element is focused.
💡 Proposed fix to verify focus
it('should have autofocus on input', () => { const { getByPlaceholderText } = render(<ChatInput {...defaultProps} />); const input = getByPlaceholderText('How can I help you today?') as HTMLTextAreaElement; - // Input should exist and be focused - expect(input).toBeTruthy(); + expect(document.activeElement).toBe(input); });Note: If jsdom doesn't properly handle
autoFocus, you may need to verify the attribute instead:expect(input).toHaveAttribute('autofocus');src/plugins/chat/public/components/chat_messages.test.tsx (2)
79-128: Missing test coverage for smart scroll's "user scrolled up" behavior.Based on the AI summary, the
ChatMessagescomponent has logic to detect when the user has scrolled up and should prevent auto-scroll. However, these tests only verify thatscrollIntoViewis called when new messages arrive.Consider adding tests that:
- Simulate user scrolling up (fire scroll event with
scrollTopnot near bottom)- Add new messages
- Verify
scrollIntoViewis NOT called (auto-scroll should be suppressed)This would require mocking the scroll container's dimensions (
scrollHeight,scrollTop,clientHeight) and dispatching scroll events. Example approach:it('should not auto-scroll when user has scrolled up', () => { const { container, rerender } = render(<ChatMessages {...defaultProps} timeline={[]} />); const messagesContainer = container.querySelector('.chatMessages'); // Mock scroll position as "not near bottom" Object.defineProperty(messagesContainer, 'scrollTop', { value: 0, writable: true }); Object.defineProperty(messagesContainer, 'scrollHeight', { value: 500 }); Object.defineProperty(messagesContainer, 'clientHeight', { value: 200 }); fireEvent.scroll(messagesContainer!); jest.clearAllMocks(); const newTimeline: Message[] = [{ id: '1', role: 'user', content: 'New message' }]; rerender(<ChatMessages {...defaultProps} timeline={newTimeline} />); expect(Element.prototype.scrollIntoView).not.toHaveBeenCalled(); });
285-298: Test doesn't verify the stated behavior.This test claims to verify that
onResendMessageis passed correctly, but it only asserts that the callback wasn't called during render (which is expected). It doesn't actually verify the prop is passed toMessageRow.Either remove this test or make it meaningful by verifying the prop is passed:
💡 Option 1: Use a mock that captures props
jest.mock('./message_row', () => ({ - MessageRow: ({ message }: any) => <div data-test-subj="message-row">{message.content}</div>, + MessageRow: jest.fn(({ message }: any) => <div data-test-subj="message-row">{message.content}</div>), })); +import { MessageRow } from './message_row'; // In test: it('should pass onResendMessage to MessageRow', () => { const onResendMessage = jest.fn(); const timeline: Message[] = [{ id: '1', role: 'user', content: 'Message' }]; render(<ChatMessages {...defaultProps} timeline={timeline} onResendMessage={onResendMessage} />); expect(MessageRow).toHaveBeenCalledWith( expect.objectContaining({ onResend: onResendMessage }), expect.anything() ); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/plugins/chat/public/components/chat_input.scsssrc/plugins/chat/public/components/chat_input.test.tsxsrc/plugins/chat/public/components/chat_input.tsxsrc/plugins/chat/public/components/chat_messages.scsssrc/plugins/chat/public/components/chat_messages.test.tsxsrc/plugins/chat/public/components/chat_messages.tsxsrc/plugins/chat/public/components/chat_window.test.tsxsrc/plugins/chat/public/components/chat_window.tsxsrc/plugins/chat/public/hooks/use_command_menu_keyboard.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/chat/public/components/chat_input.test.tsx (1)
src/plugins/chat/public/components/chat_input.tsx (1)
ChatInput(24-107)
src/plugins/chat/public/components/chat_window.test.tsx (1)
src/plugins/chat/public/components/chat_window.tsx (2)
ChatWindowInstance(30-33)ChatWindow(44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (69)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: bundle-analyzer
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Lint and validate
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: lighthouse
- GitHub Check: linkchecker
- GitHub Check: WhiteSource Security Check
🔇 Additional comments (18)
src/plugins/chat/public/components/chat_messages.scss (1)
8-8: Positioning context + keyframe cleanup look good.
Adding a relative positioning context should help any absolutely positioned children, and the keyframe formatting preserves behavior. A quick visual pass to confirm the fade-in still matches expectations would be enough.Also applies to: 47-54
src/plugins/chat/public/components/chat_input.scss (1)
1-64: Textarea + ghost text styling aligns well with the multiline UX.
Min/max height, padding, and overflow settings look consistent. Please do a quick UI check to confirm the 4‑line max height and scrollbar appearance across browsers.src/plugins/chat/public/components/chat_window.test.tsx (2)
42-43: scrollIntoView mock is a good safety net for jsdom.
Keeps the test environment stable when the API is missing.
688-1042: Stop‑streaming test coverage is thorough.
Nice coverage of abort/unsubscribe flows and post‑stop messaging. Please ensure the suite runs cleanly in CI (or run the focused test locally).src/plugins/chat/public/components/chat_messages.tsx (1)
6-127: Auto‑scroll locking + near‑bottom detection look solid.
Should prevent unexpected jumps while keeping the view pinned when appropriate; the shorter empty‑state copy is fine too. Please QA: scroll up, receive new messages (no auto‑jump), then scroll near bottom (auto‑scroll resumes).Also applies to: 155-161
src/plugins/chat/public/hooks/use_command_menu_keyboard.ts (1)
9-14: inputRef type update correctly aligns with textarea implementation.
Verified that all call sites properly create and pass HTMLTextAreaElement refs. The change is type-safe and consistent.src/plugins/chat/public/components/chat_input.test.tsx (4)
1-46: Well-structured test setup with appropriate mocks.The mocks for child components and the hook are properly configured, and the
defaultPropssetup withbeforeEachcleanup follows good testing practices.
48-74: LGTM!Rendering tests properly verify the presence of key UI elements under different states.
76-109: LGTM!Input handling tests correctly verify the onChange callback, value display, and disabled state based on streaming status.
213-245: LGTM!Button state transition tests properly verify bidirectional changes using
rerenderand both positive/negative assertions.src/plugins/chat/public/components/chat_messages.test.tsx (4)
1-44: Well-organized test setup with appropriate mocks.The mocks for child components and
scrollIntoVieware properly configured. ThedefaultPropspattern is clean and follows testing best practices.
130-183: LGTM!Good coverage of different message types including the loading state variations and special handling of tool messages.
185-283: LGTM!Tool calls and suggestions tests provide good coverage of conditional rendering logic based on message types and streaming state.
300-333: LGTM!Cleanup and accessibility tests verify basic component lifecycle and DOM structure.
src/plugins/chat/public/components/chat_input.tsx (4)
6-7: LGTM!Clean imports for the new functionality -
useEffectfor auto-resize andEuiTextAreafor multiline input.
50-60: Auto-resize implementation looks correct.The technique of resetting to
'auto'before readingscrollHeightis the standard approach. The 96px max height aligns with the PR requirement of ~4 lines.One minor note: If you want to make the max height more maintainable, consider extracting to a constant:
const MAX_TEXTAREA_HEIGHT = 96; // ~4 lines at default line-height
95-103: Button behavior is well-implemented.The conditional logic correctly:
- Enables the stop button during streaming regardless of input
- Disables the send button only when empty and not streaming
- Switches icons, colors, and aria-labels appropriately
74-85: LGTM!The
EuiTextAreaconfiguration is appropriate:
resize="none"correctly prevents manual resize since JS handles itrows={1}provides a compact initial statedisabled={isStreaming}prevents input during streaming
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| it('should handle empty onStop callback', () => { | ||
| const { getByLabelText } = render( | ||
| <ChatInput {...defaultProps} isStreaming={true} onStop={undefined as any} /> | ||
| ); | ||
|
|
||
| const button = getByLabelText('Stop generating'); | ||
|
|
||
| // Should not throw when clicking with undefined callback | ||
| expect(() => fireEvent.click(button)).not.toThrow(); | ||
| }); | ||
|
|
||
| it('should handle empty onSend callback', () => { | ||
| const { getByLabelText } = render( | ||
| <ChatInput {...defaultProps} input="test" onSend={undefined as any} /> | ||
| ); | ||
|
|
||
| const button = getByLabelText('Send message'); | ||
|
|
||
| // Should not throw when clicking with undefined callback | ||
| expect(() => fireEvent.click(button)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Edge case tests may fail - component doesn't guard against undefined callbacks.
Looking at the ChatInput implementation (line 97), the click handler directly calls onStop or onSend:
onClick={isStreaming ? onStop : onSend}If these props are undefined, clicking the button will throw TypeError: onStop is not a function. Either:
- The tests will fail (and should be removed if undefined callbacks aren't a valid use case), or
- The component should add defensive checks:
🛡️ Proposed fix in chat_input.tsx if undefined callbacks should be supported
<EuiButtonIcon
iconType={isStreaming ? 'stop' : 'sortUp'}
- onClick={isStreaming ? onStop : onSend}
+ onClick={isStreaming ? onStop : onSend}
+ onClick={() => {
+ if (isStreaming) {
+ onStop?.();
+ } else {
+ onSend?.();
+ }
+ }}If undefined callbacks are not a valid use case, consider removing these tests to avoid confusion.
🤖 Prompt for AI Agents
In `@src/plugins/chat/public/components/chat_input.test.tsx` around lines 304 -
324, The tests fail because ChatInput binds handlers directly via
onClick={isStreaming ? onStop : onSend} and clicking when onStop/onSend is
undefined throws; update the ChatInput component (referencing the ChatInput
component and props onStop and onSend) to defensively call the callbacks (e.g.,
use a wrapper that checks for existence or provide no-op defaults) so the
button's click handler invokes onStop() or onSend() only if they are functions,
and add/update tests if you choose to enforce non-optional callbacks instead.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11266 +/- ##
=======================================
Coverage 60.16% 60.17%
=======================================
Files 4652 4652
Lines 129576 129634 +58
Branches 22034 22049 +15
=======================================
+ Hits 77954 78001 +47
- Misses 46067 46074 +7
- Partials 5555 5559 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
arjunkumargiri
force-pushed
the
chat-ux-improvement
branch
from
449db16 to
4b4cb7d
Compare
| @import "@elastic/eui/src/global_styling/variables"; | ||
|
|
There was a problem hiding this comment.
we don't need to import this
| placeholder="Ask anything. Type / for actions" | ||
| placeholder="How can I help you today?" |
There was a problem hiding this comment.
why we changed this placeholder?
There was a problem hiding this comment.
Existing placeholder seems incorrect. I don't think '/' for actions is suggested yet.
There was a problem hiding this comment.
we have added a / command system for investigation with /investigation and there will more command next, i think we need to keep it
|
|
||
| // Update streaming state | ||
| setIsStreaming(false); | ||
| }, [chatService]); |
There was a problem hiding this comment.
what's the behavior stop the response in middle? should we clean the incomplete message for next round of llm call or keep it whatever it is? can we have a test about this?
There was a problem hiding this comment.
If stop is pressed in the middle of streaming, then the content is retained as is with incomplete message. This is the existing behavior in other LLM chat UI such as claude. There is existing test case to cover this scenario.
There was a problem hiding this comment.
so for next round LLM call, we will send the incomplete message to LLM? For now, it works. When we support store the conversation history and persist the conversation data, there will have a inconsistency between UI and the storage side. And agent side need to support stop as well
arjunkumargiri
force-pushed
the
chat-ux-improvement
branch
2 times, most recently
from
a5e72bc to
5c73724
Compare
…box height increase Signed-off-by: Arjun kumar Giri <arjung@amazon.com>
arjunkumargiri
force-pushed
the
chat-ux-improvement
branch
from
5c73724 to
ebb5c3b
Compare
|
@sicheng-lu, can you work with @arjunkumargiri to review the UX improvements before merging. Thanks. |
|
Stop button looks good. Is the textfield still accessible in the stop button state? |
No, it could be accessed only after streaming is stopped. |
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
SuZhou-Joe
left a comment
SuZhou-Joe
left a comment
There was a problem hiding this comment.
Similar PR for stop agent execution, will consolidate the PRs.
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
…box height increase (opensearch-project#11266) * UX improvement for chat window: Stop button, scroll bar fix and text box height increase Signed-off-by: Arjun kumar Giri <arjung@amazon.com> * feat: fix UT Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: Arjun kumar Giri <arjung@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>
6 participants
Description
Chat box UX improvements:
Screenshot
Testing the changes
Local testing
Changelog
Check List
yarn test:jestyarn test:jest_integration