Conversation
Co-authored-by: me <me@kentcdodds.com>
WalkthroughCancellation support for tool calls was added to the application. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ToolsTab
participant App
participant ToolAPI
User->>ToolsTab: Clicks "Run Tool"
ToolsTab->>App: callTool(name, params)
App->>ToolAPI: makeRequest(params, { signal })
Note right of App: Creates AbortController, stores in state
alt User cancels
User->>ToolsTab: Clicks "Cancel"
ToolsTab->>App: cancelTool()
App->>ToolAPI: abort request via AbortController
ToolAPI-->>App: Aborted error
App->>ToolsTab: Set result as cancelled
else Tool completes
ToolAPI-->>App: Result
App->>ToolsTab: Set result as success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| const cancelTool = () => { | ||
| if (toolAbortController) { | ||
| toolAbortController.abort(); |
There was a problem hiding this comment.
Bug: Tool Call Race Conditions and Cancellation Issues
Concurrent tool calls introduce several race conditions and inconsistencies: Rapid "Run Tool" clicks can overwrite the toolAbortController state, making earlier calls uncancellable. The finally block unconditionally clears toolAbortController, which can prematurely clear the controller for a still-running or subsequent call, leading to an inconsistent UI state (tool appears not running while active) and making the active call uncancellable. Additionally, the cancellation detection logic is unreliable, checking abortController.signal.aborted instead of e.name === 'AbortError', potentially misclassifying natural failures as cancellations. Finally, errors are not cleared upon tool cancellation, leaving stale error messages visible in the UI.
Locations (1)
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/src/components/__tests__/ToolsTab.test.tsx (2)
199-202: Consider using more specific button identification.The cancel button is identified by
{ name: "" }which relies on it being an icon-only button. Consider adding a more explicit test identifier or accessible name.- const cancelButton = screen.getByRole("button", { name: "" }); // X icon button + const cancelButton = screen.getByRole("button", { name: /cancel/i });Or add an
aria-labelto the cancel button in the component for better accessibility and testing.
213-220: Same button identification issue in interaction test.The same pattern of using
{ name: "" }appears here. Consistent with the previous comment, this should use a more specific identifier.Apply the same fix as suggested above for better test reliability and accessibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/CANCELLATION_TESTS.md(1 hunks)client/src/__tests__/toolCancellation.unit.test.tsx(1 hunks)client/src/components/__tests__/ToolsTab.test.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/CANCELLATION_TESTS.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with proper type annotations
Use PascalCase for component names and types
Files:
client/src/__tests__/toolCancellation.unit.test.tsxclient/src/components/__tests__/ToolsTab.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use ES modules (import/export) not CommonJS
Use camelCase for variables and functions
Use async/await for asynchronous operations
Implement proper error handling with try/catch blocks
Files:
client/src/__tests__/toolCancellation.unit.test.tsxclient/src/components/__tests__/ToolsTab.test.tsx
**/*.{js,jsx,ts,tsx,json,css,md}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use Prettier for formatting (auto-formatted on commit)
Files:
client/src/__tests__/toolCancellation.unit.test.tsxclient/src/components/__tests__/ToolsTab.test.tsx
**/*.{js,jsx,ts,tsx,css,scss}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use kebab-case for file names
Files:
client/src/__tests__/toolCancellation.unit.test.tsxclient/src/components/__tests__/ToolsTab.test.tsx
🔇 Additional comments (10)
client/src/__tests__/toolCancellation.unit.test.tsx (7)
1-42: Well-structured test setup with comprehensive mocking.The test setup properly mocks all necessary dependencies and creates realistic error objects for testing different scenarios. The mock implementation for
setToolAbortControllercorrectly handles both direct values and function updates.
44-119: Excellent implementation of the core cancellation logic for testing.The
createCallToolFunctionaccurately mirrors the real implementation with proper:
- Concurrent call prevention
- AbortController lifecycle management
- Race condition protection through controller identity checks
- Proper error handling for AbortError vs other errors
The logic correctly handles the complex scenarios that could occur in the actual application.
129-171: Thorough testing of concurrent call prevention.These tests effectively verify that:
- Multiple concurrent calls are prevented when a tool is already running
- New calls are allowed after previous calls complete
- The abort controller state is properly managed
The promise-based approach to simulate long-running requests is well-designed.
173-211: Comprehensive AbortError detection testing.The tests properly distinguish between cancellation (AbortError) and regular errors, ensuring:
- AbortError results in cancellation message with
isError: false- Regular errors are treated as failures with
isError: true- Appropriate state management for each scenario
213-251: Excellent race condition protection tests.These tests cover critical edge cases where the abort controller might change during execution, ensuring:
- State updates are skipped if the controller is no longer active
- Cleanup only occurs for the correct controller
- No stale state updates occur
253-274: Solid cancellation function testing.The tests verify that:
cancelToolproperly callsabort()on the active controller- It handles gracefully when no tool is running
- No errors are thrown in edge cases
276-309: Comprehensive state lifecycle management tests.These tests ensure the abort controller is properly managed throughout all scenarios:
- Normal completion
- Error conditions
- Cancellation scenarios
- Proper cleanup in
finallyblocksThe assertion counts verify the exact sequence of setter calls.
client/src/components/__tests__/ToolsTab.test.tsx (3)
53-54: Proper addition of new props to test setup.The
cancelToolmock function andisToolRunningboolean are correctly added todefaultProps, aligning with the updated component interface.
139-180: Improved test using explicit state management.The refactored test is much clearer by using
rerenderwith explicitisToolRunningvalues instead of relying on promise timing. This approach:
- Makes the test more deterministic and readable
- Tests all three states: not running → running → completed
- Verifies button text and disabled state at each stage
182-264: Comprehensive cancellation UI testing.The new test suite thoroughly covers:
- Cancel button visibility based on running state
- Proper button styling and layout
- Cancel button interaction and callback invocation
- Integration between run and cancel buttons
- Edge cases like no active tool
The tests properly use accessible queries and verify the expected UI behavior.
- Replace 'any' types with proper function signatures - Use jest.MockedFunction with specific type parameters - Fix mock function implementations to satisfy TypeScript strict mode - All tests still pass and linting is clean
- Apply Prettier formatting to unit test file - Maintain consistent code style across the project
2 participants
Add a cancellation button for tool execution to allow users to stop long-running tool calls.
This PR leverages the existing
AbortControllerinfrastructure inmakeRequestto provide a user-facing cancellation mechanism for tool calls. AnAbortControlleris now managed inApp.tsxfor the current tool execution, and a "Cancel" button is displayed inToolsTab.tsxwhen a tool is running, allowing users to abort the ongoing request.Learn more about Cursor Agents
Summary by CodeRabbit
New Features
User Interface
Tests