Skip to content

Conversation

yuhongsun96
Copy link
Contributor

@yuhongsun96 yuhongsun96 commented Sep 23, 2025

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.

  • This PR should be backported (make sure to check that the backport attempt succeeds)
  • [Optional] Override Linear Check

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.

@yuhongsun96 yuhongsun96 requested a review from a team as a code owner September 23, 2025 00:09
Copy link

vercel bot commented Sep 23, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
internal-search Building Building Preview Comment Sep 23, 2025 0:09am

@yuhongsun96 yuhongsun96 merged commit 082897e into main Sep 23, 2025
19 of 23 checks passed
@yuhongsun96 yuhongsun96 deleted the fix-toggles branch September 23, 2025 00:09
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

Edit Code Review Bot Settings | Greptile

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a 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 =&gt; 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));
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 23, 2025

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 =&gt; prev.filter(...)).</comment>

<file context>
@@ -268,6 +268,11 @@ function MCPToolsList({
+
+    // If we&#39;re disabling a tool that is currently forced, remove it from forced tools
+    if (!disabled &amp;&amp; forcedToolIds.includes(toolId)) {
+      setForcedToolIds(forcedToolIds.filter((id) =&gt; id !== toolId));
+    }
   };
</file context>
Suggested change
setForcedToolIds(forcedToolIds.filter((id) => id !== toolId));
setForcedToolIds((prev) => prev.filter((id) => id !== toolId));
Fix with Cubic

brijsiyag-meesho pushed a commit to brijsiyag-meesho/onyx that referenced this pull request Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant