-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix Toggles #5469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Toggles #5469
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR fixes inconsistent UI behavior in the chat interface's tool management system by adding defensive logic to coordinate between two separate toggle states: disabled/enabled and forced/unforced. The change addresses a scenario where tools could exist in a contradictory state of being both "disabled" and "forced" simultaneously.
The implementation adds identical logic in two components (MCPToolsList
and ActionToggle
) within the toggleToolForCurrentAssistant
function. When a user disables a tool that is currently in the "forced" state, the system now automatically removes that tool from the forcedToolIds
array. This prevents the logical inconsistency where a forced tool (which should always be available) could also be disabled.
The fix integrates with the existing tool management system in the chat interface, which allows users to both enable/disable individual tools and separately designate certain tools as "forced" (always active). By adding this coordination logic, the UI maintains consistent state relationships and provides a more intuitive user experience when managing tool availability in chat sessions.
Confidence score: 4/5
- This PR is safe to merge with minimal risk of breaking existing functionality
- Score reflects straightforward defensive logic that prevents UI inconsistencies without modifying core business logic
- Pay close attention to the ActionManagement.tsx file to ensure the logic correctly handles edge cases in tool state management
1 file reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="web/src/app/chat/components/input/ActionManagement.tsx">
<violation number="1" location="web/src/app/chat/components/input/ActionManagement.tsx:274">
Use functional state update to avoid stale state when deriving next value from previous (setForcedToolIds should use prev => prev.filter(...)).</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
|
||
// If we're disabling a tool that is currently forced, remove it from forced tools | ||
if (!disabled && forcedToolIds.includes(toolId)) { | ||
setForcedToolIds(forcedToolIds.filter((id) => id !== toolId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use functional state update to avoid stale state when deriving next value from previous (setForcedToolIds should use prev => prev.filter(...)).
Prompt for AI agents
Address the following comment on web/src/app/chat/components/input/ActionManagement.tsx at line 274:
<comment>Use functional state update to avoid stale state when deriving next value from previous (setForcedToolIds should use prev => prev.filter(...)).</comment>
<file context>
@@ -268,6 +268,11 @@ function MCPToolsList({
+
+ // If we're disabling a tool that is currently forced, remove it from forced tools
+ if (!disabled && forcedToolIds.includes(toolId)) {
+ setForcedToolIds(forcedToolIds.filter((id) => id !== toolId));
+ }
};
</file context>
setForcedToolIds(forcedToolIds.filter((id) => id !== toolId)); | |
setForcedToolIds((prev) => prev.filter((id) => id !== toolId)); |
Description
Unintuitive toggle behavior now fixed
How Has This Been Tested?
[Describe the tests you ran to verify your changes]
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.
Summary by cubic
Fix toggle behavior so disabling a tool automatically clears it from forced tools, preventing contradictory states in MCPToolsList and ActionToggle. Users can no longer have a tool both disabled and forced; toggles now stay in sync.