Conversation
- Add LLM simulator adapter for deterministic testing of tool flows - Add comprehensive unit tests for tool execution scenarios: - Server tool execution - Client tool execution - Approval workflows - Multi-tool and parallel tool execution - Tool sequences - Error handling - Add E2E test infrastructure for tool scenarios - Fix client tool race conditions by tracking pending tool executions - Prevent duplicate continuation attempts with continuationPending flag - Guard against concurrent stream processing in streamResponse - Add approval info to ToolCall type for server-side decision tracking - Include approval info in model message conversion for approval workflows - Check ModelMessage format for approval info extraction in chat activity This change improves the reliability of tool execution, especially for: - Client tools with async execute functions - Approval-based tool workflows - Sequential tool execution scenarios
📝 WalkthroughWalkthroughThis pull request addresses tool execution race conditions in the AI framework by introducing pending execution tracking, stream concurrency guards, and server-side state preservation. Changes include modifications to chat client streaming and tool handling, server-side message conversion and approval tracking, comprehensive test infrastructure with an LLM simulator adapter, and E2E test suites validating various tool execution scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
|
View your CI Pipeline Execution ↗ for commit 91c9c6f
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-devtools-core
@tanstack/ai-gemini
@tanstack/ai-grok
@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-vue
@tanstack/ai-vue-ui
@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: 12
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/messages.ts (1)
178-187:⚠️ Potential issue | 🟠 MajorPreserve approval metadata when converting ModelMessage → UIMessage.
Right now approvals on
toolCall.approvalare dropped, so approval state/decision is lost in UI conversions.Suggested fix
- parts.push({ - type: 'tool-call', - id: toolCall.id, - name: toolCall.function.name, - arguments: toolCall.function.arguments, - state: 'input-complete', // Model messages have complete arguments - }) + const approval = toolCall.approval + ? { + id: toolCall.approval.id, + needsApproval: true, + approved: toolCall.approval.approved, + } + : undefined + parts.push({ + type: 'tool-call', + id: toolCall.id, + name: toolCall.function.name, + arguments: toolCall.function.arguments, + state: approval ? 'approval-responded' : 'input-complete', + ...(approval && { approval }), + })
🤖 Fix all issues with AI agents
In `@packages/typescript/ai-client/src/chat-client.ts`:
- Around line 317-320: The current guard in streamResponse() returns early when
isLoading is true, so calls to append() can add messages without triggering a
response; implement a follow-up queue: when append() is called while isLoading
is true, push a lightweight marker (e.g., a pendingStream flag or enqueue the
message id) onto a small queue/flag on the ChatClient instance and, when the
active stream finishes (in the completion/cleanup path of streamResponse()),
check that queue/flag and immediately call streamResponse() again to process the
queued append; update append(), streamResponse(), and the stream completion
handler (look for methods named append, streamResponse, and the isLoading
checks) to set/enqueue on append and to dequeue/clear and re-invoke
streamResponse after the current load completes.
In `@packages/typescript/smoke-tests/adapters/src/tests/tools/sequences.test.ts`:
- Line 301: The test declares callOrder with the disallowed shorthand type
`string[]`; update each declaration of `callOrder` (e.g., the `const callOrder:
string[] = []` instances around the tests) to use the preferred generic form
`Array<string>` so the lint rule passes (replace `string[]` with `Array<string>`
in both occurrences).
In `@packages/typescript/smoke-tests/e2e/package.json`:
- Around line 25-30: Package.json currently lists build-only plugin
"vite-tsconfig-paths" alongside runtime deps; move only "vite-tsconfig-paths"
into devDependencies. Edit package.json to remove "vite-tsconfig-paths" from the
dependencies block and add it under devDependencies, while keeping
"@tanstack/tests-adapters" and "zod" in dependencies because
"@tanstack/tests-adapters" is imported at runtime in
src/routes/api.tools-test.ts and "zod" is used at runtime in
src/routes/api.tools-test.ts and src/routes/tools-test.tsx; ensure package.json
retains correct versions for react/react-dom/tailwindcss and other runtime deps.
In `@packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx`:
- Around line 355-370: The click handler for the approve button currently uses
non-null assertions (tc.approval!.id!) which can throw; instead, read const
approval = tc.approval and const approvalId = approval?.id and early-return if
approvalId is falsy, then use approvalId for
respondedApprovals.current.has(...), respondedApprovals.current.add(...),
addEvent({... toolCallId: tc.id ...}) and addToolApprovalResponse({ id:
approvalId, approved: true }); update the handler in the approve button (the
onClick closure) to use these guarded variables and avoid any `!` assertions.
In `@packages/typescript/smoke-tests/e2e/tests/tools/race-conditions.spec.ts`:
- Line 1: Reorder and split the Playwright import so named exports are
alphabetized and the Page type is a type-only import: change the combined import
of "test, expect, type Page" into two imports — a value import with alphabetized
members (expect, test) and a separate type-only import for Page using "import
type { Page }" referencing the existing import statement in the file.
In `@testing/panel/src/routes/api.simulator-chat.ts`:
- Around line 5-12: The import list at the top is failing the sort-imports rule;
reorder the named imports from '@/lib/simulator-tools' into ascending
alphabetical order to satisfy ESLint (e.g., arrange clientServerTool,
clientServerToolWithApproval, clientToolDef, clientToolWithApprovalDef,
serverTool, serverToolWithApproval). Update the single import statement that
contains serverTool, serverToolWithApproval, clientServerTool,
clientServerToolWithApproval, clientToolDef, and clientToolWithApprovalDef so
the specifiers are alphabetized and then run the linter to confirm the error is
resolved.
- Around line 25-32: Replace the T[] bracket-style array type with the generic
Array<T> form: update the declaration of toolCalls and the function return type
in parseToolCalls from ParsedToolCall[] to Array<ParsedToolCall> (references:
ParsedToolCall type, parseToolCalls function, and the toolCalls variable) so the
file complies with the `@typescript-eslint/array-type` rule.
- Around line 23-55: The regex TOOL_CALL_REGEX is declared with the global flag,
so its internal lastIndex can persist between calls and cause missed matches;
inside parseToolCalls(message) reset TOOL_CALL_REGEX.lastIndex = 0 before
entering the while loop so each invocation starts scanning from the beginning of
the message, then proceed with the existing matching/JSON-parsing logic in
parseToolCalls.
In `@testing/panel/src/routes/simulator.tsx`:
- Around line 232-237: The UI directly calls JSON.parse on part.arguments inside
the JSX (the JSON.stringify(JSON.parse(part.arguments), null, 2) expression)
which will throw if the string is invalid; wrap the parse in a safe path: either
create a small helper (e.g., safeParseJSON) or run a try/catch around JSON.parse
to fall back to a sensible value (the raw part.arguments string or an error
message) before passing to JSON.stringify, and update the JSX to render that
safe result so invalid JSON doesn't crash the component.
- Around line 3-11: Reorder the named imports from 'lucide-react' so they
satisfy the project's sort rule: replace the current list (Send, Square, Zap,
FlaskConical, Server, Monitor, ShieldCheck) with an alphabetically-sorted list:
FlaskConical, Monitor, Send, Server, ShieldCheck, Square, Zap; apply the same
reordering to the other identical import on the other occurrence (lines
referenced in the review).
- Around line 202-209: The code registers remarkGfm in rehypePlugins so GFM
won't be applied; move remarkGfm into the remarkPlugins array for the
ReactMarkdown component. Locate the ReactMarkdown usage and remove remarkGfm
from rehypePlugins (which should keep rehypeRaw, rehypeSanitize,
rehypeHighlight) and add remarkGfm to remarkPlugins so that remarkGfm runs
during Markdown AST processing.
- Around line 488-499: The dynamic Tailwind classes using template literals
(e.g., text-${category.color}-400 and hover:border-${category.color}-500/30) are
not safe for JIT purging; create a static mapping object (e.g., colorClasses = {
cyan: { icon: 'text-cyan-400', hoverBorder: 'hover:border-cyan-500/30' },
purple: {...}, yellow: {...}, green: {...} }) and replace the template literals:
use category.icon with className={colorClasses[category.color].icon} and build
the button className by including colorClasses[category.color].hoverBorder (and
any other static pieces) instead of the template strings; mirror the approach
used in structured.tsx to ensure Tailwind picks up the classes.
🧹 Nitpick comments (11)
packages/typescript/smoke-tests/e2e/tests/tools/approval-flow.spec.ts (3)
1-1: Import statement can be improved per ESLint rules.The static analysis flags import sorting and inline type specifier style. Consider using a top-level type import.
🔧 Suggested fix
-import { test, expect, type Page } from '@playwright/test' +import type { Page } from '@playwright/test' +import { expect, test } from '@playwright/test'
18-46: Helper functions are duplicated across multiple test spec files.The
selectScenario,waitForTestComplete,getMetadata, andgetEventLoghelpers are nearly identical acrossapproval-flow.spec.ts,client-tool.spec.ts, andserver-client-sequence.spec.ts. Consider extracting these to a shared test utilities module to reduce maintenance burden.
119-124: Empty catch block silently swallows errors.The catch block returns an empty array without any indication that parsing failed. While acceptable for test helpers, consider logging a warning for debugging purposes.
🔧 Optional improvement
try { return JSON.parse(el.textContent || '[]') } catch { + console.warn('Failed to parse event-log-json') return [] }packages/typescript/smoke-tests/adapters/src/llm-simulator.ts (2)
1-6: Import sorting can be improved per ESLint rules.🔧 Suggested fix
import type { + ChatOptions, StreamChunk, - ChatOptions, StructuredOutputOptions, StructuredOutputResult, } from '@tanstack/ai'
302-389: Complex but necessary iteration detection logic.The
determineIterationFromMessagesmethod handles multiple message formats correctly:
- Server tool results (
role: 'tool')- UIMessage format (parts with
outputorapproval.approved)- ModelMessage format (toolCalls with approval info)
The logic is well-documented with inline comments explaining each case. Consider adding a unit test specifically for this method to guard against regressions.
packages/typescript/smoke-tests/e2e/src/routes/api.tools-test.ts (1)
9-12: Import statement can be improved per ESLint rules.🔧 Suggested fix
+import type { SimulatorScript } from '@tanstack/tests-adapters' import { createLLMSimulator, - type SimulatorScript, } from '@tanstack/tests-adapters'packages/typescript/smoke-tests/e2e/tests/tools/server-client-sequence.spec.ts (1)
1-1: Import statement can be improved per ESLint rules.🔧 Suggested fix
-import { test, expect, type Page } from '@playwright/test' +import type { Page } from '@playwright/test' +import { expect, test } from '@playwright/test'packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx (1)
1-5: Import statement can be improved per ESLint rules.🔧 Suggested fix
-import { useState, useCallback, useRef, useEffect } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import { createFileRoute } from '@tanstack/react-router' -import { useChat, fetchServerSentEvents } from '@tanstack/ai-react' +import { fetchServerSentEvents, useChat } from '@tanstack/ai-react'packages/typescript/smoke-tests/e2e/tests/tools/client-tool.spec.ts (1)
1-1: Import statement can be improved per ESLint rules.🔧 Suggested fix
-import { test, expect, type Page } from '@playwright/test' +import type { Page } from '@playwright/test' +import { expect, test } from '@playwright/test'packages/typescript/smoke-tests/adapters/src/tests/tools/server-tool.test.ts (1)
7-16: ExtractcollectChunksinto a shared test helper.This helper is repeated across multiple tool test files; consider a shared utility to keep the test suite DRY.
testing/panel/src/routes/simulator.tsx (1)
55-126: RenameTOOL_TEMPLATESto camelCase.Keeping variable names in camelCase improves consistency and aligns with the style rule.
As per coding guidelines: Use camelCase for function and variable names throughout the codebase.♻️ Suggested rename
-const TOOL_TEMPLATES = [ +const toolTemplates = [ { category: 'Server Tools', icon: Server, color: 'cyan', tools: [ { name: 'Server Tool', template: 'serverTool({ text: "hello from server" })', description: 'Executes on server only', }, @@ - {TOOL_TEMPLATES.map((category) => ( + {toolTemplates.map((category) => (Also applies to: 485-505
packages/typescript/smoke-tests/adapters/src/tests/tools/sequences.test.ts
Outdated
Show resolved
Hide resolved
packages/typescript/smoke-tests/e2e/tests/tools/race-conditions.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| for (const message of this.messages) { | ||
| // todo remove any and fix this | ||
| // Check for UIMessage format (parts array) |
There was a problem hiding this comment.
Maybe keep TODO about removing any at some point?
There was a problem hiding this comment.
This seems to be completely deprecated and yet in place due to the any cast, I think this should be completely removed @jherr
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/typescript/ai-client/src/chat-client.ts`:
- Around line 384-397: Add a brief clarifying comment above the block that
checks streamCompletedSuccessfully to explain that lastPart is read from
this.processor.getMessages() which may have been updated by nested streams
(e.g., via drainPostStreamActions), so lastPart reflects the current message
state rather than the original stream's final state and therefore prevents
triggering a continuation when nested streams produced text; reference the
surrounding symbols like streamCompletedSuccessfully,
this.processor.getMessages(), lastPart, shouldAutoSend(), and
checkForContinuation() to show the intent.
In `@packages/typescript/smoke-tests/adapters/src/llm-simulator.ts`:
- Around line 1-6: Import members in the type-only import are not alphabetized;
reorder the imported type names in the import statement in llm-simulator.ts so
they follow alphabetical order (ChatOptions, StreamChunk,
StructuredOutputOptions, StructuredOutputResult) to satisfy ESLint's member
ordering rule.
In `@packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx`:
- Around line 1-5: Alphabetize the named import specifiers for each import
statement to satisfy the sort-imports rule: reorder "useState, useCallback,
useRef, useEffect" to "useCallback, useEffect, useRef, useState" in the React
import, reorder "useChat, fetchServerSentEvents" to "fetchServerSentEvents,
useChat" in the `@tanstack/ai-react` import, and ensure other single-name imports
like "toolDefinition" (from `@tanstack/ai`) and "z" (from zod) remain unchanged;
keep the import lines themselves as-is but update the member ordering for a
lint-clean result.
- Around line 597-617: Keep using dangerouslySetInnerHTML for the two script
tags but escape any "<" in the JSON to prevent closing </script> injection: wrap
the JSON.stringify(...) results for the script with .replace(/</g, '\\u003c') so
the event-log script (using toolEvents) and the tool-calls script (using
toolCalls.map(...) producing id/name/state/hasOutput) both output safe JSON
strings while remaining parseable.
In `@testing/panel/test-results/.last-run.json`:
- Around line 2-7: The committed .last-run.json is an auto-generated Playwright
test state file and must be ignored; add ".last-run.json" to .gitignore (either
the repo root or the test panel folder's .gitignore) and remove the tracked
.last-run.json from the commit by removing it from git's index (e.g., git rm
--cached <the .last-run.json>), then commit the .gitignore update and the
removal so the file is no longer tracked.
🧹 Nitpick comments (7)
testing/panel/tests/tool-flow.spec.ts (1)
158-161: Assertion always passes — provides no validation.The assertion
expect(available.length).toBeGreaterThanOrEqual(0)is always true since array length cannot be negative. If the intent is to verify that at least one tool-capable provider is available for testing, the assertion should betoBeGreaterThan(0).♻️ Proposed fix
test('at least one tool-capable provider should be available', async () => { const available = getToolCapableProviders() - expect(available.length).toBeGreaterThanOrEqual(0) + expect(available.length).toBeGreaterThan(0) })testing/panel/playwright-report/index.html (1)
23428-23430: Exclude Playwright reports from version control.This generated Playwright test report (945 KB, base64-encoded ZIP) should not be committed to the repository. It creates significant bloat, produces unreadable binary diffs, and adds noise to the git history. Add
playwright-report/to.gitignoreand configure your CI to store reports as artifacts instead.packages/typescript/ai-client/src/connection-adapters.ts (1)
299-312: Consider aligning comment terminology with fetch adapters.The signature change to accept both
UIMessageandModelMessagearrays is a backward-compatible widening that provides flexibility.Minor inconsistency: Lines 307-308 reference "Server-side chat()" while the fetch adapters (lines 145-146, 244-245) reference "Server-side TextEngine" for conversion handling. If both paths ultimately use the same conversion mechanism, consider using consistent terminology across all adapters for clarity.
packages/typescript/ai/src/activities/chat/index.ts (1)
762-766: Consider adding a defensive check forpart.id.If
part.idis undefined for any reason (malformed message), this would create an entry with key"undefined"in the map, which could cause subtle issues when looking up by actual tool call IDs.🛡️ Optional defensive check
// Extract client tool results (tools without approval that have output) - if (part.output !== undefined && !part.approval) { + if (part.id && part.output !== undefined && !part.approval) { clientToolResults.set(part.id, part.output) }packages/typescript/smoke-tests/adapters/src/llm-simulator.ts (1)
398-519: RenameSimulatorScriptsto camelCase to match naming guidelines.
SimulatorScriptsis a variable export; prefer camelCase to align with the project’s naming rule.♻️ Suggested rename (remember to update usages)
-export const SimulatorScripts = { +export const simulatorScripts = { /** * Script for a single server tool call */ singleServerTool(packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx (2)
27-95: Add.server()implementations to keep tools isomorphic.Guidelines call for dual-environment tool definitions; these tools are currently client-only. Consider adding a server stub (or a real server implementation) to avoid divergence.
♻️ Suggested pattern (adjust chaining order per toolDefinition docs)
const showNotificationTool = toolDefinition({ name: 'show_notification', description: 'Show a notification to the user', inputSchema: z.object({ message: z.string(), type: z.enum(['info', 'warning', 'error']), }), outputSchema: z.object({ displayed: z.boolean(), timestamp: z.number(), }), - }).client(async (args) => { + }) + .server(async () => { + throw new Error('show_notification is client-only in tools-test') + }) + .client(async (args) => { addEvent({ type: 'execution-start', toolName: 'show_notification', details: args.message, })
99-140: RenameSCENARIOSto camelCase.The codebase guideline calls for camelCase variable names.
♻️ Suggested rename
-const SCENARIOS = [ +const scenarios = [ { id: 'text-only', label: 'Text Only (No Tools)', category: 'basic' }, { id: 'server-tool-single', label: 'Single Server Tool', category: 'basic' }, { id: 'client-tool-single', label: 'Single Client Tool', category: 'basic' },- {SCENARIOS.filter((s) => s.category === 'basic').map((s) => ( + {scenarios.filter((s) => s.category === 'basic').map((s) => ( <option key={s.id} value={s.id}> {s.label} </option> ))}- {SCENARIOS.filter((s) => s.category === 'race').map((s) => ( + {scenarios.filter((s) => s.category === 'race').map((s) => ( <option key={s.id} value={s.id}> {s.label} </option> ))}
|
|
||
| // Continue conversation if the stream ended with a tool result (server tool completed) | ||
| if (streamCompletedSuccessfully) { | ||
| const messages = this.processor.getMessages() | ||
| const lastPart = messages.at(-1)?.parts.at(-1) | ||
|
|
||
| if (lastPart?.type === 'tool-result' && this.shouldAutoSend()) { | ||
| try { | ||
| await this.checkForContinuation() | ||
| } catch (error) { | ||
| console.error('Failed to continue flow after tool result:', error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find the chat-client.ts file
find . -name "chat-client.ts" -type fRepository: TanStack/ai
Length of output: 105
🏁 Script executed:
# Read the specific lines mentioned in the review (384-397)
sed -n '370,420p' packages/typescript/ai-client/src/chat-client.ts | cat -nRepository: TanStack/ai
Length of output: 1896
🏁 Script executed:
# Look at the drainPostStreamActions method
rg -n "drainPostStreamActions" packages/typescript/ai-client/src/chat-client.ts -A 20Repository: TanStack/ai
Length of output: 1426
🏁 Script executed:
# Look at the checkForContinuation method
rg -n "checkForContinuation" packages/typescript/ai-client/src/chat-client.ts -A 15Repository: TanStack/ai
Length of output: 2180
🏁 Script executed:
# Check for any tests related to tool continuation and nested streams
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs rg -l "tool.*result|continuation" | head -5Repository: TanStack/ai
Length of output: 444
🏁 Script executed:
# Check the server-client-sequence test for nested tool scenarios
head -100 packages/typescript/smoke-tests/e2e/tests/tools/server-client-sequence.spec.tsRepository: TanStack/ai
Length of output: 3195
🏁 Script executed:
# Look for tests that involve multiple tool calls or nested tool scenarios
rg -n "tool.*result|nested|continuation" packages/typescript/smoke-tests/e2e/tests/tools/server-client-sequence.spec.ts -B 2 -A 5Repository: TanStack/ai
Length of output: 702
🏁 Script executed:
# Check if there are any comments in chat-client.ts explaining the streamCompletedSuccessfully logic
rg -n "streamCompletedSuccessfully" packages/typescript/ai-client/src/chat-client.ts -B 5 -A 2Repository: TanStack/ai
Length of output: 993
🏁 Script executed:
# Look at the broader context where streamCompletedSuccessfully is set
sed -n '300,400p' packages/typescript/ai-client/src/chat-client.ts | cat -nRepository: TanStack/ai
Length of output: 3857
Consider adding a clarifying comment about state synchronization after nested streams.
The implementation correctly handles nested streams from drainPostStreamActions(): streamCompletedSuccessfully reflects the original stream's completion, while lastPart fetches the current message state (potentially updated by nested streams). This prevents spurious continuations when nested streams produce text responses.
Adding a comment like the following would clarify this pattern for future maintainers:
// Note: lastPart reflects the current message state (potentially updated by
// drainPostStreamActions), not the original stream's final state.
// If nested streams produced a text response, lastPart won't be 'tool-result'.
🤖 Prompt for AI Agents
In `@packages/typescript/ai-client/src/chat-client.ts` around lines 384 - 397, Add
a brief clarifying comment above the block that checks
streamCompletedSuccessfully to explain that lastPart is read from
this.processor.getMessages() which may have been updated by nested streams
(e.g., via drainPostStreamActions), so lastPart reflects the current message
state rather than the original stream's final state and therefore prevents
triggering a continuation when nested streams produced text; reference the
surrounding symbols like streamCompletedSuccessfully,
this.processor.getMessages(), lastPart, shouldAutoSend(), and
checkForContinuation() to show the intent.
| import type { | ||
| StreamChunk, | ||
| ChatOptions, | ||
| StructuredOutputOptions, | ||
| StructuredOutputResult, | ||
| } from '@tanstack/ai' |
There was a problem hiding this comment.
Fix import member ordering to satisfy lint rules.
ESLint expects alphabetized members in the type-only import.
♻️ Suggested fix
-import type {
- StreamChunk,
- ChatOptions,
- StructuredOutputOptions,
- StructuredOutputResult,
-} from '@tanstack/ai'
+import type {
+ ChatOptions,
+ StreamChunk,
+ StructuredOutputOptions,
+ StructuredOutputResult,
+} from '@tanstack/ai'📝 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.
| import type { | |
| StreamChunk, | |
| ChatOptions, | |
| StructuredOutputOptions, | |
| StructuredOutputResult, | |
| } from '@tanstack/ai' | |
| import type { | |
| ChatOptions, | |
| StreamChunk, | |
| StructuredOutputOptions, | |
| StructuredOutputResult, | |
| } from '@tanstack/ai' |
🧰 Tools
🪛 ESLint
[error] 3-3: Member 'ChatOptions' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
In `@packages/typescript/smoke-tests/adapters/src/llm-simulator.ts` around lines 1
- 6, Import members in the type-only import are not alphabetized; reorder the
imported type names in the import statement in llm-simulator.ts so they follow
alphabetical order (ChatOptions, StreamChunk, StructuredOutputOptions,
StructuredOutputResult) to satisfy ESLint's member ordering rule.
| import { useState, useCallback, useRef, useEffect } from 'react' | ||
| import { createFileRoute } from '@tanstack/react-router' | ||
| import { useChat, fetchServerSentEvents } from '@tanstack/ai-react' | ||
| import { toolDefinition } from '@tanstack/ai' | ||
| import { z } from 'zod' |
There was a problem hiding this comment.
Sort import members to satisfy lint rules.
This aligns with the configured sort-imports checks.
♻️ Suggested fix
-import { useState, useCallback, useRef, useEffect } from 'react'
+import { useCallback, useEffect, useRef, useState } from 'react'
-import { useChat, fetchServerSentEvents } from '@tanstack/ai-react'
+import { fetchServerSentEvents, useChat } from '@tanstack/ai-react'📝 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.
| import { useState, useCallback, useRef, useEffect } from 'react' | |
| import { createFileRoute } from '@tanstack/react-router' | |
| import { useChat, fetchServerSentEvents } from '@tanstack/ai-react' | |
| import { toolDefinition } from '@tanstack/ai' | |
| import { z } from 'zod' | |
| import { useCallback, useEffect, useRef, useState } from 'react' | |
| import { createFileRoute } from '@tanstack/react-router' | |
| import { fetchServerSentEvents, useChat } from '@tanstack/ai-react' | |
| import { toolDefinition } from '@tanstack/ai' | |
| import { z } from 'zod' |
🧰 Tools
🪛 ESLint
[error] 1-1: Member 'useCallback' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 3-3: Member 'fetchServerSentEvents' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
In `@packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx` around lines 1
- 5, Alphabetize the named import specifiers for each import statement to
satisfy the sort-imports rule: reorder "useState, useCallback, useRef,
useEffect" to "useCallback, useEffect, useRef, useState" in the React import,
reorder "useChat, fetchServerSentEvents" to "fetchServerSentEvents, useChat" in
the `@tanstack/ai-react` import, and ensure other single-name imports like
"toolDefinition" (from `@tanstack/ai`) and "z" (from zod) remain unchanged; keep
the import lines themselves as-is but update the member ordering for a
lint-clean result.
| <script | ||
| id="event-log-json" | ||
| type="application/json" | ||
| dangerouslySetInnerHTML={{ __html: JSON.stringify(toolEvents) }} | ||
| /> | ||
|
|
||
| {/* Tool calls as JSON for easy parsing in tests */} | ||
| <script | ||
| id="tool-calls-json" | ||
| type="application/json" | ||
| dangerouslySetInnerHTML={{ | ||
| __html: JSON.stringify( | ||
| toolCalls.map((tc) => ({ | ||
| id: tc.id, | ||
| name: tc.name, | ||
| state: tc.state, | ||
| hasOutput: !!tc.output, | ||
| })), | ||
| ), | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | sed -n '590,625p'Repository: TanStack/ai
Length of output: 1169
🌐 Web query:
React text children script tag type application/json escaping JSON parsing
💡 Result:
When you put JSON as a React text child inside a <script type="application/json"> (or application/ld+json) tag, React will HTML-escape it during SSR/HTML rendering, so quotes become ", etc.—which breaks consumers that expect raw JSON text (e.g., JSON-LD parsers). The common fix is to inject the raw string with dangerouslySetInnerHTML. [1]
Safe pattern (raw JSON text, plus XSS-safe escaping)
const data = { /* possibly user-influenced data */ };
<script
type="application/json"
dangerouslySetInnerHTML={{
__html: JSON.stringify(data).replace(/</g, '\\u003c'),
}}
/>Next.js explicitly recommends escaping < like this (or using a library such as serialize-javascript) because JSON.stringify alone does not prevent </script>-based injection. [2]
Reading it back
const el = document.getElementById('my-data');
const obj = JSON.parse(el.textContent);Gotchas
- Don’t set both
childrenanddangerouslySetInnerHTMLon the same element. [3] - If any fields can contain untrusted text, treat this as an XSS boundary: escape as above (at minimum) or use a hardened serializer. [2][4]
Sources: Next.js JSON-LD guidance (including the replace(/</g, '\\u003c') mitigation) [2], Next.js metadata JSON-LD example using dangerouslySetInnerHTML [1], React <script> docs / constraints [3], SSR/script-tag XSS discussion in React security notes [4].
[1] (nextjs.org)
[2] (nextjs.org)
[3] (react.dev)
[4] (0xn3va.gitbook.io)
Citations:
- 1: https://nextjs.org/docs/13/app/building-your-application/optimizing/metadata?utm_source=openai
- 2: https://nextjs.org/docs/app/guides/json-ld?utm_source=openai
- 3: https://react.dev/reference/react-dom/components/script?utm_source=openai
- 4: https://0xn3va.gitbook.io/cheat-sheets/framework/react/security-issues?utm_source=openai
🏁 Script executed:
grep -n "const toolEvents\|let toolEvents\|toolEvents =" packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | head -20Repository: TanStack/ai
Length of output: 37
🏁 Script executed:
grep -n "const toolCalls\|let toolCalls\|toolCalls =" packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | head -20Repository: TanStack/ai
Length of output: 104
🏁 Script executed:
cat -n packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | sed -n '200,240p'Repository: TanStack/ai
Length of output: 1433
🏁 Script executed:
cat -n packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | sed -n '100,160p'Repository: TanStack/ai
Length of output: 2610
🏁 Script executed:
cat -n packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | sed -n '160,200p'Repository: TanStack/ai
Length of output: 1681
🏁 Script executed:
grep -n "createTrackedTools\|ToolEvent" packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | head -30Repository: TanStack/ai
Length of output: 501
🏁 Script executed:
cat -n packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx | sed -n '1,50p'Repository: TanStack/ai
Length of output: 1719
Use replace(/</g, '\\u003c') to safely escape JSON in script tags—the proposed text-child approach breaks JSON parsing.
The security concern is valid: </script> sequences in toolEvents.details or similar fields could break out of the script tag. However, using direct text children (as suggested) will cause React to HTML-escape the entire JSON string, making it unparseable when read back.
The correct fix is to keep dangerouslySetInnerHTML but add escaping:
-dangerouslySetInnerHTML={{ __html: JSON.stringify(toolEvents) }}
+dangerouslySetInnerHTML={{ __html: JSON.stringify(toolEvents).replace(/</g, '\\u003c') }}Apply the same escaping to both script tags' JSON.stringify() calls.
📝 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.
| <script | |
| id="event-log-json" | |
| type="application/json" | |
| dangerouslySetInnerHTML={{ __html: JSON.stringify(toolEvents) }} | |
| /> | |
| {/* Tool calls as JSON for easy parsing in tests */} | |
| <script | |
| id="tool-calls-json" | |
| type="application/json" | |
| dangerouslySetInnerHTML={{ | |
| __html: JSON.stringify( | |
| toolCalls.map((tc) => ({ | |
| id: tc.id, | |
| name: tc.name, | |
| state: tc.state, | |
| hasOutput: !!tc.output, | |
| })), | |
| ), | |
| }} | |
| /> | |
| <script | |
| id="event-log-json" | |
| type="application/json" | |
| dangerouslySetInnerHTML={{ __html: JSON.stringify(toolEvents).replace(/</g, '\\u003c') }} | |
| /> | |
| {/* Tool calls as JSON for easy parsing in tests */} | |
| <script | |
| id="tool-calls-json" | |
| type="application/json" | |
| dangerouslySetInnerHTML={{ | |
| __html: JSON.stringify( | |
| toolCalls.map((tc) => ({ | |
| id: tc.id, | |
| name: tc.name, | |
| state: tc.state, | |
| hasOutput: !!tc.output, | |
| })), | |
| ).replace(/</g, '\\u003c'), | |
| }} | |
| /> |
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 599-599: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
[warning] 606-606: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.3.13)
[error] 600-600: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 607-608: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In `@packages/typescript/smoke-tests/e2e/src/routes/tools-test.tsx` around lines
597 - 617, Keep using dangerouslySetInnerHTML for the two script tags but escape
any "<" in the JSON to prevent closing </script> injection: wrap the
JSON.stringify(...) results for the script with .replace(/</g, '\\u003c') so the
event-log script (using toolEvents) and the tool-calls script (using
toolCalls.map(...) producing id/name/state/hasOutput) both output safe JSON
strings while remaining parseable.
| "status": "failed", | ||
| "failedTests": [ | ||
| "e6d271510b4cd4631a47-390d9afc5f702f185ad7", | ||
| "e6d271510b4cd4631a47-abc15f00eeb116802b74", | ||
| "e6d271510b4cd4631a47-22ee07d931418c7e699e" | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's an existing .gitignore in the testing/panel directory
echo "=== Checking for .gitignore in testing/panel ==="
cat testing/panel/.gitignore 2>/dev/null || echo "No .gitignore found in testing/panel"
# Check root .gitignore for test-results patterns
echo ""
echo "=== Root .gitignore patterns ==="
grep -E "(test-results|playwright|\.last-run)" .gitignore 2>/dev/null || echo "No relevant patterns found"
# Check if the file exists and its current status
echo ""
echo "=== Checking file existence and git status ==="
ls -la testing/panel/test-results/.last-run.json 2>/dev/null || echo "File not found in working directory"
cd testing/panel && find . -name ".gitignore" -type f 2>/dev/null && echo "--- .gitignore files found in testing/panel ---" || echo "No .gitignore files in testing/panel"
cd - > /dev/nullRepository: TanStack/ai
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Search for Playwright references in config files
echo "=== Playwright configuration and references ==="
find . -maxdepth 2 -name "playwright.config.*" -o -name "vitest.config.*" -o -name "package.json" | head -5
echo ""
# Check root package.json for test scripts
echo "=== Test scripts in package.json ==="
grep -A5 '"test' package.json 2>/dev/null | head -20
echo ""
# Look for any documentation about test-results
echo "=== References to test-results or .last-run in docs ==="
find . -type f \( -name "README*" -o -name "*.md" \) | xargs grep -l "test-results\|\.last-run" 2>/dev/null || echo "No doc references found"
echo ""
# Check .gitignore files in the entire repo to see patterns
echo "=== All .gitignore patterns related to test or build artifacts ==="
find . -name ".gitignore" -type f | xargs cat 2>/dev/null | grep -E "(test|build|dist|node_modules|coverage)" | head -20Repository: TanStack/ai
Length of output: 1700
🏁 Script executed:
#!/bin/bash
# Search for Playwright in dependencies
echo "=== Playwright in package.json ==="
grep -i "playwright" package.json 2>/dev/null || echo "No Playwright found in root package.json"
echo ""
echo "=== Checking testing/panel directory ==="
ls -la testing/panel/ 2>/dev/null | head -20
echo ""
echo "=== If testing/panel/package.json exists ==="
cat testing/panel/package.json 2>/dev/null | head -50 || echo "No package.json in testing/panel"
echo ""
echo "=== Contents of .last-run.json ==="
cat testing/panel/test-results/.last-run.json 2>/dev/null
echo ""
echo "=== Search for .last-run.json references in codebase ==="
rg "\.last-run\.json" --type json --type ts --type js 2>/dev/null | head -10 || echo "No references found"Repository: TanStack/ai
Length of output: 2838
Add .last-run.json to .gitignore and remove from the commit.
This file is auto-generated by Playwright to track test run state and should not be committed. The presence of "status": "failed" in the committed artifact is particularly problematic because:
- It will cause merge conflicts when multiple developers run tests locally
- It introduces machine-generated state into version control that should be ephemeral
- The failed status in a committed artifact indicates test failures were recorded
Recommended fix: Add the following to the root .gitignore (or create testing/panel/.gitignore if preferred):
+# Playwright test artifacts
+testing/panel/test-results/
+testing/panel/playwright-report/Then remove testing/panel/test-results/.last-run.json from this commit.
🤖 Prompt for AI Agents
In `@testing/panel/test-results/.last-run.json` around lines 2 - 7, The committed
.last-run.json is an auto-generated Playwright test state file and must be
ignored; add ".last-run.json" to .gitignore (either the repo root or the test
panel folder's .gitignore) and remove the tracked .last-run.json from the commit
by removing it from git's index (e.g., git rm --cached <the .last-run.json>),
then commit the .gitignore update and the removal so the file is no longer
tracked.
3 participants
🎯 Changes
Fixes some issues around client side tool execution of multiple tool calls. Adds tests to validate the fixes.
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
New Features
Tests