[Chat] Fix tool call positioning in conversation timeline#11115
[Chat] Fix tool call positioning in conversation timeline#11115wanglam merged 3 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Lin Wang <wonglam@amazon.com>
📝 WalkthroughWalkthroughThis pull request fixes tool call positioning in the conversation timeline by reworking the placement logic in the chat event handler to intelligently attach tool calls to existing messages or create new placeholder messages, with corresponding test cases validating both scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11115 +/- ##
=======================================
Coverage 60.36% 60.37%
=======================================
Files 4591 4591
Lines 126396 126410 +14
Branches 21183 21184 +1
=======================================
+ Hits 76295 76314 +19
+ Misses 44687 44682 -5
Partials 5414 5414
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:
|
wanglam
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
ashwin-pc,
bandinib-amzn,
curq,
huyaboo,
joshuali925,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
wanglam
requested review from
Maosaic,
TackAdam,
angle943,
d-buckner,
paulstn,
ps48,
raintygao,
ruchidh and
sumukhswamy
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugins/chat/public/services/chat_event_handler.ts (1)
275-289: LGTM! Strategy 3 correctly creates a placeholder when needed.The fake message creation handles the case where the LLM returns only tool calls without a text message, ensuring proper timeline structure.
Minor consistency suggestion: Consider using
Date.now()instead ofnew Date().getTime()for consistency with other parts of the file (e.g., line 233).🔎 Optional consistency improvement
- const fakeAssistantMessageId = `fake-assistant-message-` + new Date().getTime(); + const fakeAssistantMessageId = `fake-assistant-message-${Date.now()}`;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelogs/fragments/11115.ymlsrc/plugins/chat/public/services/chat_event_handler.test.tssrc/plugins/chat/public/services/chat_event_handler.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/chat/public/services/chat_event_handler.ts (1)
src/core/public/chat/types.ts (1)
AssistantMessage(54-58)
🪛 YAMLlint (1.37.1)
changelogs/fragments/11115.yml
[error] 2-2: syntax error: expected , but found ''
(syntax)
⏰ 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). (20)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
🔇 Additional comments (5)
src/plugins/chat/public/services/chat_event_handler.test.ts (2)
365-403: LGTM! Comprehensive test for Strategy 2 (timeline-based placement).The test correctly validates that when the last assistant message appears after the last user message in the timeline, the tool call is attached to the existing assistant message rather than creating a new one.
405-447: LGTM! Comprehensive test for Strategy 3 (fake message creation).The test correctly validates that when a user message appears after the last assistant message, a new placeholder assistant message is created to hold the tool call. This ensures proper conversation flow when users send new messages after an assistant response.
src/plugins/chat/public/services/chat_event_handler.ts (3)
213-224: LGTM! Clear documentation of the multi-strategy approach.The comments effectively explain the three placement strategies and their rationale, making the code maintainable and easy to understand.
249-254: LGTM! Strategy 1 implementation is correct.Using the explicitly provided
parentMessageIdwhen available is the most reliable approach and avoids any ambiguity in tool call placement.
256-273: LGTM! Strategy 2 correctly determines tool call placement based on timeline positions.The logic properly handles edge cases:
- When
lastTextMessageStartIdis null, it falls through to Strategy 3- When there are no user messages (
lastUserMessageIndex === -1), the assistant message takes precedence- The use of
findLastIndexensures the most recent messages are compared
| fix: | ||
| - [Chat] Fix tool call positioning in conversation timeline ([#11115](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11115)) No newline at end of file |
There was a problem hiding this comment.
Fix YAML syntax error by quoting the changelog entry.
The URL contains special characters (brackets and parentheses) that must be quoted in YAML to avoid parsing errors.
🔎 Proposed fix
fix:
-- [Chat] Fix tool call positioning in conversation timeline ([#11115](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11115))
+- "[Chat] Fix tool call positioning in conversation timeline ([#11115](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11115))"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fix: | |
| - [Chat] Fix tool call positioning in conversation timeline ([#11115](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11115)) | |
| fix: | |
| - "[Chat] Fix tool call positioning in conversation timeline ([#11115](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/11115))" |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
In changelogs/fragments/11115.yml around lines 1 to 2, the changelog entry
contains unquoted special characters (brackets and parentheses) in the URL which
breaks YAML parsing; wrap the entire list item string in quotes (single or
double) so the dash is followed by a quoted string that includes the bracketed
PR link, e.g. change - [Chat] Fix tool... ([#11115](...)) to a single quoted
line; ensure matching quotes and no unescaped internal quotes.
| const targetMessageId = parentMessageId || this.lastTextMessageStartId; | ||
| // Strategy 1: Use explicitly provided parent message ID | ||
| // This is the most reliable approach when the backend provides it | ||
| if (parentMessageId) { |
There was a problem hiding this comment.
Under what situation the parentMessageId will not available?
There was a problem hiding this comment.
This parentMessageId comes from the chat agent event stream and depends on the chat agent implementation. Typically, it has a valid value when the agent responds with a message that initiates tool calls. All tool calls from the same response share the same parentMessageId.
Previously, the implementation would fall back to the latest chat agent message when parentMessageId wasn't provided. In this PR, I added logic to check if there are any user input messages after the latest chat agent message. If so, it creates a virtual chat agent message (frontend-only) to properly group these tool calls.
| } | ||
|
|
||
| // Strategy 3: Create a new assistant message placeholder | ||
| // This handles the case where the LLM responds with tool calls but without any text message. |
There was a problem hiding this comment.
is this an abnormal case because LLM didn't follow our prompt?
There was a problem hiding this comment.
This depends on the chat agent implementation. From my perspective, if the user asks the chat agent to do something straightforward, having no text message before the tool calling section makes sense to me.
|
can you paste a screenshot of before and after? |
|
Hi @Hailong-am , I've added the screenshots of before and after. Feel free to help me review it. Thank you. |
4 participants
Description
This PR improves the reliability of tool call placement in the chat conversation timeline by implementing a multi-strategy approach for determining where tool calls should be attached. Previously, tool calls could be misplaced in the timeline when users sent new messages after an assistant's response, leading to incorrect conversation flow and potential confusion.
The fix introduces three strategies for tool call placement: (1) using an explicit parent message ID when provided by the backend, (2) analyzing timeline positions to determine if the last assistant message is still the current conversation turn, and (3) creating a placeholder assistant message when the LLM responds with only tool calls and no text message. This ensures tool calls are always correctly associated with their corresponding assistant responses, maintaining proper chronological ordering in the chat interface. Comprehensive unit tests have been added to validate all three placement strategies.
Screenshots
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.