fix: complete tool calls with server results#596
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements the fix for Issue ChangesTool call complete state lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 Changeset Version Preview3 package(s) bumped directly, 16 bumped as dependents. 🟩 Patch bumps
|
|
View your CI Pipeline Execution ↗ for commit 1774aae
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
![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
🤖 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 @.changeset/server-tool-results-complete.md:
- Around line 2-4: The changeset currently uses patch bumps for packages
'`@tanstack/ai`', '`@tanstack/ai-client`', and '`@tanstack/ai-event-client`' but the
addition of 'complete' to the exported ToolCallState type is a public type-shape
change and must be released as a minor bump per the repo's pre-1.0 convention;
update the changeset entry to use "minor" for those three packages (and ensure
the changeset message references the ToolCallState/complete change) so the
release tooling will produce a minor version rather than a patch.
In `@testing/e2e/tests/tools-test/server-client-sequence.spec.ts`:
- Around line 72-76: The inline function passed to page.waitForFunction can
throw when JSON.parse reads transient DOM content; update the anonymous function
used in waitForFunction (the one querying 'messages-json-content' and assigning
messages) to guard JSON.parse with a try/catch: if parsing fails return false so
the wait continues, otherwise proceed to compute toolCalls and return the
intended truthy value; in short, wrap JSON.parse(...) in try/catch and return
false on parse error to avoid flaky polling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 284da935-0046-4678-a880-828337b71771
📒 Files selected for processing (11)
.changeset/server-tool-results-complete.mdpackages/typescript/ai-client/src/types.tspackages/typescript/ai-event-client/src/index.tspackages/typescript/ai/src/activities/chat/messages.tspackages/typescript/ai/src/activities/chat/stream/message-updaters.tspackages/typescript/ai/src/activities/chat/stream/processor.tspackages/typescript/ai/src/types.tspackages/typescript/ai/tests/message-updaters.test.tspackages/typescript/ai/tests/stream-processor.test.tstesting/e2e/tests/tools-test/helpers.tstesting/e2e/tests/tools-test/server-client-sequence.spec.ts
| '@tanstack/ai': patch | ||
| '@tanstack/ai-client': patch | ||
| '@tanstack/ai-event-client': patch |
There was a problem hiding this comment.
Use minor bumps for this exported type-shape change.
Adding 'complete' to ToolCallState is a public shape change and should be released as minor for this repo’s pre-1.0 convention.
Suggested fix
-'`@tanstack/ai`': patch
-'`@tanstack/ai-client`': patch
-'`@tanstack/ai-event-client`': patch
+'`@tanstack/ai`': minor
+'`@tanstack/ai-client`': minor
+'`@tanstack/ai-event-client`': minor📝 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.
| '@tanstack/ai': patch | |
| '@tanstack/ai-client': patch | |
| '@tanstack/ai-event-client': patch | |
| '`@tanstack/ai`': minor | |
| '`@tanstack/ai-client`': minor | |
| '`@tanstack/ai-event-client`': minor |
🤖 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 @.changeset/server-tool-results-complete.md around lines 2 - 4, The changeset
currently uses patch bumps for packages '`@tanstack/ai`', '`@tanstack/ai-client`',
and '`@tanstack/ai-event-client`' but the addition of 'complete' to the exported
ToolCallState type is a public type-shape change and must be released as a minor
bump per the repo's pre-1.0 convention; update the changeset entry to use
"minor" for those three packages (and ensure the changeset message references
the ToolCallState/complete change) so the release tooling will produce a minor
version rather than a patch.
| await page.waitForFunction( | ||
| () => { | ||
| const messagesEl = document.getElementById('messages-json-content') | ||
| const messages = JSON.parse(messagesEl?.textContent || '[]') | ||
| const toolCalls = messages.flatMap((msg: any) => |
There was a problem hiding this comment.
Guard JSON parsing inside waitForFunction to avoid flaky polling failures.
JSON.parse(messagesEl?.textContent || '[]') can throw during transient DOM updates and fail the wait early. Return false on parse failure so polling continues.
Suggested fix
await page.waitForFunction(
() => {
const messagesEl = document.getElementById('messages-json-content')
- const messages = JSON.parse(messagesEl?.textContent || '[]')
+ let messages: Array<any> = []
+ try {
+ messages = JSON.parse(messagesEl?.textContent || '[]')
+ } catch {
+ return false
+ }
const toolCalls = messages.flatMap((msg: any) =>
(msg.parts || []).filter((part: any) => part.type === 'tool-call'),
)📝 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.
| await page.waitForFunction( | |
| () => { | |
| const messagesEl = document.getElementById('messages-json-content') | |
| const messages = JSON.parse(messagesEl?.textContent || '[]') | |
| const toolCalls = messages.flatMap((msg: any) => | |
| await page.waitForFunction( | |
| () => { | |
| const messagesEl = document.getElementById('messages-json-content') | |
| let messages: Array<any> = [] | |
| try { | |
| messages = JSON.parse(messagesEl?.textContent || '[]') | |
| } catch { | |
| return false | |
| } | |
| const toolCalls = messages.flatMap((msg: any) => | |
| (msg.parts || []).filter((part: any) => part.type === 'tool-call'), | |
| ) |
🤖 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 `@testing/e2e/tests/tools-test/server-client-sequence.spec.ts` around lines 72
- 76, The inline function passed to page.waitForFunction can throw when
JSON.parse reads transient DOM content; update the anonymous function used in
waitForFunction (the one querying 'messages-json-content' and assigning
messages) to guard JSON.parse with a try/catch: if parsing fails return false so
the wait continues, otherwise proceed to compute toolCalls and return the
intended truthy value; in short, wrap JSON.parse(...) in try/catch and return
false on parse error to avoid flaky polling.
|
Added follow-up commit 4b36015 for the server-result history/conversion path: modelMessagesToUIMessages now backfills the matching tool-call part output and marks it complete when merging a tool result. Verified with pnpm.cmd --filter @tanstack/ai exec vitest run tests/message-converters.test.ts tests/stream-processor.test.ts tests/message-updaters.test.ts. |
|
Added e2e coverage for the edge case from the follow-up: tests/tools-test/server-tool-history.spec.ts loads /tools-test with model-message history containing a server tool-call plus matching tool result, then asserts UIMessage.parts has the original tool-call populated with output and state: complete. Verified with pnpm.cmd --filter @tanstack/ai run build, pnpm.cmd --filter @tanstack/ai-e2e build, and pnpm.cmd --filter @tanstack/ai-e2e exec playwright test tests/tools-test/server-tool-history.spec.ts --project=chromium. |
|
Added a manual ts-react-chat repro page in commit 18790cf. Visit /issue-176-tool-result to inspect the model-message history fixture and hydrated UIMessage.parts; the page marks the tool-call state/output as fixed when the original server tool-call has state: complete and output populated. I also added it to the example navigation as " Issue |
|
Updated the manual repro page with a live LLM flow in commit ca4db20. The page now has a Live LLM repro section: send the default guitar recommendation prompt through /api/tanchat, then inspect the live getGuitars server tool-call part next to its matching tool-result. The deterministic history fixture remains below it for the exact modelMessagesToUIMessages hydration edge case. |
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/typescript/ai/src/activities/chat/stream/processor.ts (1)
1144-1144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle empty-string tool results as valid results.
The truthy guard drops valid empty results (
''), so the tool-call never getsoutput/completeand no tool-result part is emitted in that case.Proposed fix
- if (chunk.result) { + if (chunk.result !== undefined) {🤖 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/typescript/ai/src/activities/chat/stream/processor.ts` at line 1144, The guard currently uses a truthy check on chunk.result ("if (chunk.result) {") which drops valid empty-string results; change it to an explicit null/undefined check (e.g., "if (chunk.result !== undefined && chunk.result !== null)" or "if (chunk.result != null)") in the processor where chunk.result is handled so empty strings are treated as valid results and the tool-call receives output/complete and the tool-result part is emitted.
🤖 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/typescript/ai/src/activities/chat/stream/processor.ts`:
- Line 1144: The guard currently uses a truthy check on chunk.result ("if
(chunk.result) {") which drops valid empty-string results; change it to an
explicit null/undefined check (e.g., "if (chunk.result !== undefined &&
chunk.result !== null)" or "if (chunk.result != null)") in the processor where
chunk.result is handled so empty strings are treated as valid results and the
tool-call receives output/complete and the tool-result part is emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8191dea7-2548-4b10-83ba-c55b8b182963
📒 Files selected for processing (2)
packages/typescript/ai/src/activities/chat/messages.tspackages/typescript/ai/src/activities/chat/stream/processor.ts
AlemTuzlak
changed the title
AlemTuzlak
force-pushed
the
issue-176-server-tool-results
branch
from
66fd559 to
2aa7244
Compare
AlemTuzlak
changed the title
1 participant
Summary
Closes #176.
Verification
Notes
Summary by CodeRabbit
New Features
completestate when results are availableTests
Documentation