fix(react): prevent race condition in useToolInvocation.ts

[ShobhitPatra]

closes #2656

---

[!IMPORTANT] Fixes race condition in useToolInvocations.ts by adding checks to prevent re-processing of tool calls and ensuring argsText integrity.

• Behavior:
  • Fixes race condition in useToolInvocations.ts by adding isCompleteToolCall check to prevent re-processing of tool calls.
  • Ensures argsText can only be appended, not updated, throwing an error if violated.
  • Logs a warning if argsText is updated after controller closure in non-production environments.
• Misc:
  • Corrects typo in isInititialState to isInitialState in useToolInvocations.ts.

^{This description was created by for 7dff08e26738af034aa1f1c3c13c766cb3410d6e. You can customize this summary. It will automatically update as commits are pushed.}

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 3f7b98890b90609d2a181fe06920abddd82eff6c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions[bot]]

Claude finished @ShobhitPatra's task —— View job

---

Code Review

I've analyzed this PR which fixes a race condition in useToolInvocations.ts when loading historical messages via ThreadHistoryAdapter.

---

Summary of Changes

The PR addresses issue #2656 by preventing re-processing of completed tool calls when they're loaded asynchronously (e.g., via ThreadHistoryAdapter). The key changes:

1. Adds isCompleteToolCall check: Tool calls with both a result and complete argsText are now added to ignoredToolIds regardless of isInitialState
2. Restructures control flow: Flattens the nested if/else logic for better readability
3. Fixes typo: isInititialState → isInitialState (though the typo remains in the actual code)

---

Critical Issues

1. Typo Not Fixed ⚠️

Location: Line 134, 147, 234-236

The PR description claims to fix the typo isInititialState → isInitialState, but the code still uses isInititialState throughout:

const isInititialState = useRef(true);  // Line 134 - still misspelled
if (isInititialState.current || isCompleteToolCall) {  // Line 147

Impact: Misleading PR description, future maintainability issue

Recommendation: Either fix the typo in this PR or remove the claim from the description.

---

2. Incomplete Race Condition Fix 🔴

Location: Lines 143-149

While this PR prevents the warning for completed tool calls, it doesn't fully address the underlying race condition described in issue #2656. The problem occurs when:

1. Component mounts with isInitialState = true, messages = []
2. useEffect runs, sets isInitialState = false (no messages to ignore yet)
3. ThreadHistoryAdapter finishes loading asynchronously
4. useEffect runs again with loaded messages

For in-progress tool calls loaded from history (tool calls with incomplete argsText but no result yet), this PR doesn't prevent them from being processed. Consider this scenario:

// Historical message loaded from adapter with streaming in progress
{
  type: "tool-call",
  toolCallId: "123",
  argsText: '{"partial": ',  // incomplete JSON
  result: undefined  // not complete yet
}

This would NOT match isCompleteToolCall (since result === undefined), and would still be processed as a new tool call, potentially creating duplicate controllers.

Recommendation: Consider whether in-progress tool calls can be loaded from history. If so, additional logic may be needed.

---

3. Missing Changeset ⚠️

Location: .changeset/ directory

The PR modifies source code in packages/react/ but doesn't include a changeset. This is required per the repository's contribution guidelines.

Recommendation: Add a changeset using:

npx changeset add

Select @assistant-ui/react with patch version bump and message: fix(react): prevent race condition

---

Suggested Improvements

1. Error Message Could Be More Helpful

Location: Lines 181-183

The error thrown when argsText is updated (not appended) could include more context:

throw new Error(
  `Tool call argsText can only be appended, not updated: ${content.argsText} does not start with ${lastState.argsText}`,
);

Suggestion:

throw new Error(
  `Tool call argsText can only be appended, not updated.\n` +
  `Tool ID: ${content.toolCallId}\n` +
  `Expected prefix: ${lastState.argsText}\n` +
  `Received: ${content.argsText}`
);

---

2. Comment Has Typo

Location: Line 151

//  skip if aready processed

Should be: // skip if already processed

---

3. Consider Adding JSDoc

Location: Line 18

The isArgsTextComplete helper function could benefit from documentation explaining why JSON parsing is used to determine completeness:

/**
 * Checks if the argsText string contains complete, valid JSON.
 * Used to determine if a tool call's arguments are fully received.
 */
const isArgsTextComplete = (argsText: string) => {
  // ...
}

---

Security & Performance

✅ No security issues identified

• Input validation present (checks for string prefix)
• Error thrown on invalid state transitions

✅ Performance considerations adequate

• isArgsTextComplete uses try/catch which is reasonable for this use case
• No unnecessary re-processing due to memoization in refs

---

Test Coverage

⚠️ No tests found for useToolInvocations.ts

This is a complex hook with critical race condition logic. Recommended test cases:

1. Historical messages with complete tool calls don't create controllers
2. Historical messages with incomplete tool calls (if supported)
3. argsText can only be appended, not replaced
4. Nested messages are processed correctly
5. Controller lifecycle (create → append → close)

---

Good Practices Observed

• Uses refs appropriately for mutable state that shouldn't trigger re-renders
• Proper abort controller cleanup
• Development-only warnings (non-production check)
• Recursive message processing for nested structures

---

Recommendation

Request changes - Address the typo inconsistency and add the required changeset before merging. Consider whether in-progress historical tool calls need handling.

---

[github-actions[bot]]

Claude finished @ShobhitPatra's task —— View job

---

Code Review Complete

Summary

This PR attempts to fix the race condition described in issue #2656, where ThreadHistoryAdapter asynchronously loads historical messages containing completed tool calls, causing spurious warnings. However, the current implementation has a critical logic flaw that breaks tool invocation processing entirely.

Critical Issues

1. Broken Logic: Completed Tool Calls Ignored Entirely ⚠️

Location: useToolInvocations.ts:147

The new isCompleteToolCall check adds completed tool calls to ignoredToolIds and returns early, preventing any processing. This means:

• Historical tool calls never create controllers
• Their data (args/results) never reaches the application
• ThreadHistoryAdapter functionality is broken

Root cause: The fix misunderstands the problem. Issue #2656 describes historical tool calls being processed too much (creating unnecessary controllers), but this fix swings to the opposite extreme by ignoring them completely.

What should happen: Completed historical tool calls should be skipped from controller creation, but only when they have no existing controller state. The current logic prevents ALL completed tool calls from being processed, even those that are legitimately streaming in.

Suggested Fix

if (isInititialState.current) {
  ignoredToolIds.current.add(content.toolCallId);
  return;
}

// Skip if already processed
if (ignoredToolIds.current.has(content.toolCallId)) {
  return;
}

// Skip completed tool calls from history (no existing controller state)
const isHistoricalCompletedCall = 
  content.result !== undefined &&
  isArgsTextComplete(content.argsText) &&
  !lastToolStates.current[content.toolCallId];

if (isHistoricalCompletedCall) {
  ignoredToolIds.current.add(content.toolCallId);
  return;
}

// Continue with normal processing for in-progress or new tool calls

This distinguishes between:

• Historical completed calls (skip) ✓
• In-progress calls with existing controllers (process) ✓
• New streaming calls (process) ✓

2. Incomplete Fix for Race Condition

Location: useToolInvocations.ts:143-149

The fix doesn't address all race condition scenarios:

• Scenario 1 (handled): Tool call with both result and complete argsText → skipped ✓
• Scenario 2 (not handled): Tool call with incomplete argsText and no result yet → processed as new, potentially creating duplicate controllers ✗

Historical messages could contain in-progress tool calls (e.g., user refreshed during execution). These would not match isCompleteToolCall and would be processed as new tool calls.

3. Typo Claim Not Implemented

Location: useToolInvocations.ts:134, 147, 234-236, 253

The PR description claims to fix isInititialState → isInitialState, but the variable remains misspelled throughout the file. Either fix the typo or remove the claim.

Additional Issues

4. Missing Changeset ⚠️

This PR modifies source code in packages/react (a published package) but lacks a changeset. Per repository requirements, a changeset is required for version bumping and changelog generation.

Action needed: Add a changeset using the provided link.

5. Code Structure Change Without Functional Benefit

The refactoring reduces indentation by inverting the isInitialState check, which is reasonable. However, combined with the flawed isCompleteToolCall logic, it introduces more problems than it solves.

Good Practices Observed

• Early returns reduce nesting
• Validation ensures argsText can only be appended
• Non-production warning helps catch unexpected state transitions

Test Coverage Concerns

No tests were added or modified. Given the complexity of race conditions and the multiple scenarios (initial load, async history, streaming updates), comprehensive tests are critical:

1. Test historical completed tool calls are skipped
2. Test historical in-progress tool calls are handled correctly
3. Test new streaming tool calls work during/after history load
4. Test the interaction between isInitialState and async loading

Recommendations

Immediate Actions Required

1. Fix the critical logic flaw - Use the suggested approach above or implement Option 2 from issue #2656
2. Add changeset - Required for published package changes
3. Either fix the typo or remove the claim - Current state is inconsistent

Follow-up Improvements

4. Add comprehensive tests - Race conditions need multiple test scenarios
5. Consider coordination with loading state - Option 1 from issue #2656 (coordinate with isLoading flag) might be more robust
6. Document the intended behavior - Add comments explaining when tool calls should be ignored vs processed

CI Status

• Code Quality: ⚠️ Action required (likely due to missing changeset)
• Tests: Not run or pending

---

Verdict: This PR requires significant changes before merging. The current implementation breaks core functionality and doesn't fully address the race condition described in issue #2656.

[github-actions[bot]]

Claude finished @ShobhitPatra's task —— View job

---

Code Review

Summary

This PR attempts to fix the race condition described in issue #2656, where ThreadHistoryAdapter asynchronously loads historical messages with completed tool calls, causing spurious development warnings. However, the current implementation introduces a critical bug that breaks tool invocation processing entirely for completed tool calls loaded from history.

Building on previous bot reviews (which correctly identified this issue), this review provides additional analysis and recommendations.

---

Changes Since Last Review

This appears to be the initial commit for this PR. The key change is adding an isCompleteToolCall check at line 161-163 to skip completed tool calls that don't have existing state.

---

Critical Issues

1. Logic Flaw Breaks Historical Tool Invocations 🔴

Location: useToolInvocations.ts:161-164

The new logic has a fundamental flaw that multiple bots have correctly identified:

if (isCompleteToolCall && !lastState) {
  ignoredToolIds.current.add(content.toolCallId);
  return;
}

Problem: When a completed tool call is encountered without existing state, it's added to ignoredToolIds and processing stops immediately. This means the subsequent code (lines 167-231) never executes, so:

• No controller is created
• argsText is never sent to the controller
• result is never set via setResponse()
• The tool invocation data never reaches the application

Impact: Historical tool calls loaded via ThreadHistoryAdapter will not be delivered to the UI. This completely breaks the history loading feature that issue #2656 is trying to fix.

Why the race condition still exists: The root cause isn't addressed. The race happens because:

1. Component mounts, isInitialState = true, messages = []
2. useEffect runs with empty array, sets isInitialState = false
3. ThreadHistoryAdapter loads asynchronously
4. useEffect runs again with loaded messages
5. Since isInitialState is now false, all historical tool calls are processed as "new"

Your fix attempts to skip completed tool calls, but it's too aggressive - it prevents them from being processed at all, rather than just preventing duplicate controller creation.

Recommended Fix

The real issue is that isInitialState becomes false before history finishes loading. Consider Option 1 from issue #2656 - coordinate with the loading state:

// In the hook params, add isLoading from useExternalHistory
type UseToolInvocationsParams = {
  state: AssistantTransportState;
  getTools: () => Record<string, Tool> | undefined;
  onResult: (command: AssistantTransportCommand) => void;
  setToolStatuses: (/* ... */) => void;
  isLoading?: boolean; // Add this
};

// In the useEffect
useEffect(() => {
  processMessages(state.messages);

  // Only set to false once loading completes
  if (isInititialState.current && !isLoading) {
    isInititialState.current = false;
  }
}, [state, controller, onResult, isLoading]);

This way, isInitialState stays true until history loading completes, and all historical tool calls are properly added to ignoredToolIds.

2. Typo Not Fixed Despite PR Description Claim

Location: useToolInvocations.ts:134, 148, 244, 263

The PR description claims:

Corrects typo in isInititialState to isInitialState

However, the variable remains misspelled as isInititialState throughout the file. Either fix the typo or remove this claim from the PR description.

3. Missing Changeset

Status: ⚠️ Missing

This PR modifies source code in packages/react/ (a published package) but doesn't include a changeset. According to repository requirements, this is needed for version bumping and changelog generation.

Action needed: Add a changeset with:

npx changeset add

Select @assistant-ui/react, choose patch, and use the message: fix(react): prevent race condition in useToolInvocations

---

Additional Issues

4. Incomplete Race Condition Coverage

The fix only handles completed tool calls (with both result and complete argsText). What about in-progress tool calls loaded from history?

Consider this scenario:

// Historical message with streaming in progress
{
  type: "tool-call",
  toolCallId: "123",
  argsText: '{"partial": ',  // incomplete JSON
  result: undefined  // not complete yet
}

This wouldn't match isCompleteToolCall (since result === undefined) and would still be processed as a new tool call, potentially creating duplicate controllers.

Question for author: Can ThreadHistoryAdapter load in-progress tool calls, or only completed ones?

5. Code Restructuring Without Functional Benefit

The refactoring reduces nesting by inverting the isInitialState check and using early returns. While this improves readability, combined with the flawed isCompleteToolCall logic, it introduces more problems than it solves.

---

Suggested Improvements

6. Enhanced Error Message

Location: useToolInvocations.ts:191-193

The error message could be more helpful by including the tool call ID:

throw new Error(
  `Tool call argsText can only be appended, not updated.\n` +
  `Tool ID: ${content.toolCallId}\n` +
  `Expected prefix: ${lastState.argsText}\n` +
  `Received: ${content.argsText}`
);

7. Add JSDoc for Helper Function

Location: useToolInvocations.ts:18-25

The isArgsTextComplete helper could benefit from documentation:

/**
 * Checks if argsText contains complete, valid JSON.
 * Used to determine if a tool call's arguments are fully received.
 */
const isArgsTextComplete = (argsText: string) => {
  // ...
}

---

Test Coverage

⚠️ No tests added

This is a complex hook with critical race condition logic. Given the multiple edge cases, comprehensive tests are essential:

1. Historical completed tool calls don't create duplicate controllers
2. Historical in-progress tool calls (if supported)
3. New streaming tool calls work correctly during/after history load
4. argsText append-only validation
5. Controller lifecycle management

---

Security & Performance

✅ No security issues identified

• Input validation present (string prefix check)
• Error thrown on invalid state transitions

✅ Performance considerations adequate

• isArgsTextComplete try/catch overhead is acceptable
• Refs prevent unnecessary re-renders

---

Good Practices Observed

• Early returns reduce nesting
• Development-only warnings for debugging
• Proper abort controller cleanup
• Refs for mutable state

---

Recommendation

Request changes - This PR requires significant modifications:

Required Changes

1. Fix the critical logic flaw - Implement proper coordination with loading state (Option 1 from issue #2656)
2. Add changeset - Required for published packages
3. Fix typo or update PR description - Current state is inconsistent

Recommended Changes

4. Add comprehensive tests - Essential for race condition scenarios
5. Consider loading state coordination - More robust than checking tool call completion
6. Document edge cases - Clarify when tool calls should be ignored vs processed

---