Skip to content

Fix/stale tool params 988 #1107

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.

Sign up for GitHub

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

Draft
olaservo wants to merge 2 commits into
base: main
Choose a base branch
from
+172 −65
Draft

Fix/stale tool params 988 #1107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
841c42e
Remove misleading structured content compatibility warning
olaservo Feb 19, 2026
0a1330d
Fix stale tool parameters sent on Run Tool click (#988)
olaservo Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions client/src/components/DynamicJsonForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ interface DynamicJsonFormProps {
}

export interface DynamicJsonFormRef {
validateJson: () => { isValid: boolean; error: string | null };
validateJson: () => {
isValid: boolean;
error: string | null;
value?: JsonValue;
};
hasJsonError: () => boolean;
}

Expand Down Expand Up @@ -222,7 +226,7 @@ const DynamicJsonForm = forwardRef<DynamicJsonFormRef, DynamicJsonFormProps>(
}
onChange(parsed);
setJsonError(undefined);
return { isValid: true, error: null };
return { isValid: true, error: null, value: parsed };
} catch (err) {
const errorMessage =
err instanceof Error ? err.message : "Invalid JSON";
Expand Down
27 changes: 7 additions & 20 deletions client/src/components/ToolResults.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,14 @@ const checkContentCompatibility = (
text?: string;
[key: string]: unknown;
}>,
): { isCompatible: boolean; message: string } => {
): { hasMatch: boolean; message: string } | null => {
// Look for at least one text content block that matches the structured content
const textBlocks = unstructuredContent.filter(
(block) => block.type === "text",
);

if (textBlocks.length === 0) {
return {
isCompatible: false,
message: "No text blocks to match structured content",
};
return null;
}

// Check if any text block contains JSON that matches the structured content
Expand All @@ -49,7 +46,7 @@ const checkContentCompatibility = (

if (isEqual) {
return {
isCompatible: true,
hasMatch: true,
message: `Structured content matches text block${textBlocks.length > 1 ? " (multiple blocks)" : ""}${unstructuredContent.length > textBlocks.length ? " + other content" : ""}`,
};
}
Expand All @@ -59,10 +56,7 @@ const checkContentCompatibility = (
}
}

return {
isCompatible: false,
message: "No text block matches structured content",
};
return null;
};

const ToolResults = ({
Expand Down Expand Up @@ -195,16 +189,9 @@ const ToolResults = ({
<h5 className="font-semibold mb-2 text-sm">
Unstructured Content:
</h5>
{compatibilityResult && (
<div
className={`mb-2 p-2 rounded text-sm ${
compatibilityResult.isCompatible
? "bg-blue-100 dark:bg-blue-900 text-blue-800 dark:text-blue-200"
: "bg-yellow-100 dark:bg-yellow-900 text-yellow-800 dark:text-yellow-200"
}`}
>
{compatibilityResult.isCompatible ? "✓" : "⚠"}{" "}
{compatibilityResult.message}
{compatibilityResult?.hasMatch && (
<div className="mb-2 p-2 rounded text-sm bg-blue-100 dark:bg-blue-900 text-blue-800 dark:text-blue-200">
✓ {compatibilityResult.message}
</div>
)}
</>
Expand Down
108 changes: 65 additions & 43 deletions client/src/components/ToolsTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,30 @@ const ToolsTab = ({
const { copied, setCopied } = useCopy();

// Function to check if any form has validation errors
// When validateChildren is true, also collects fresh parsed values from JSON editors
const checkValidationErrors = (validateChildren: boolean = false) => {
const errors = Object.values(formRefs.current).some(
(ref) =>
ref &&
(validateChildren ? !ref.validateJson().isValid : ref.hasJsonError()),
);
setHasValidationErrors(errors);
return errors;
const freshValues: Record<string, JsonValue> = {};
let hasErrors = false;

for (const [key, ref] of Object.entries(formRefs.current)) {
if (!ref) continue;
if (validateChildren) {
const result = ref.validateJson();
if (!result.isValid) {
hasErrors = true;
}
if (result.value !== undefined) {
freshValues[key] = result.value;
}
} else {
if (ref.hasJsonError()) {
hasErrors = true;
}
}
}

setHasValidationErrors(hasErrors);
return { hasErrors, freshValues };
};

useEffect(() => {
Expand Down Expand Up @@ -333,8 +349,8 @@ const ToolsTab = ({
name={key}
checked={params[key] === null}
onCheckedChange={(checked: boolean) =>
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: checked
? null
: prop.type === "array"
Expand All @@ -349,7 +365,7 @@ const ToolsTab = ({
prop.type === "integer"
? undefined
: undefined,
})
}))
}
/>
<label
Expand All @@ -373,10 +389,10 @@ const ToolsTab = ({
name={key}
checked={!!params[key]}
onCheckedChange={(checked: boolean) =>
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: checked,
})
}))
}
/>
<label
Expand All @@ -395,15 +411,15 @@ const ToolsTab = ({
}
onValueChange={(value) => {
if (value === "") {
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: undefined,
});
}));
} else {
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: value,
});
}));
}
}}
>
Expand Down Expand Up @@ -436,16 +452,16 @@ const ToolsTab = ({
const value = e.target.value;
if (value === "") {
// Field cleared - set to undefined
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: undefined,
});
}));
} else {
// Field has value - keep as string
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: value,
});
}));
}
}}
className="mt-1"
Expand All @@ -466,10 +482,10 @@ const ToolsTab = ({
generateDefaultValue(prop)
}
onChange={(newValue: JsonValue) => {
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: newValue,
});
}));
// Check validation after a short delay to allow form to update
setTimeout(checkValidationErrors, 100);
}}
Expand All @@ -491,24 +507,24 @@ const ToolsTab = ({
const value = e.target.value;
if (value === "") {
// Field cleared - set to undefined
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: undefined,
});
}));
} else {
// Field has value - try to convert to number, but store input either way
const num = Number(value);
if (!isNaN(num)) {
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: num,
});
}));
} else {
// Store invalid input as string - let server validate
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: value,
});
}));
}
}
}}
Expand All @@ -526,10 +542,10 @@ const ToolsTab = ({
}}
value={params[key] as JsonValue}
onChange={(newValue: JsonValue) => {
setParams({
...params,
setParams((prev) => ({
...prev,
[key]: newValue,
});
}));
// Check validation after a short delay to allow form to update
setTimeout(checkValidationErrors, 100);
}}
Expand Down Expand Up @@ -764,8 +780,14 @@ const ToolsTab = ({
</div>
<Button
onClick={async () => {
// Validate JSON inputs before calling tool
if (checkValidationErrors(true)) return;
// Validate JSON inputs and collect fresh values before calling tool
const { hasErrors, freshValues } =
checkValidationErrors(true);
if (hasErrors) return;

// Merge fresh values from JSON editors into current params
// to bypass React's async setState
const currentParams = { ...params, ...freshValues };

try {
setIsToolRunning(true);
Expand All @@ -785,7 +807,7 @@ const ToolsTab = ({
}, {});
await callTool(
selectedTool.name,
params,
currentParams,
Object.keys(metadata).length ? metadata : undefined,
runAsTask,
);
Expand Down
94 changes: 94 additions & 0 deletions client/src/components/__tests__/ToolsTab.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,5 +1121,99 @@ describe("ToolsTab", () => {
// Tool should have been called (empty JSON is considered valid)
expect(mockCallTool).toHaveBeenCalled();
});

it("should send updated values on second Run Tool after editing (bug #988 exact scenario)", async () => {
const mockCallTool = jest.fn();
renderToolsTab({
tools: [toolWithJsonParams],
selectedTool: toolWithJsonParams,
callTool: mockCallTool,
});

const textareas = screen.getAllByRole("textbox");

// Step 1: Set initial value and run tool
fireEvent.change(textareas[0], {
target: { value: '{ "key": "value1" }' },
});
fireEvent.change(textareas[1], {
target: { value: '["item1"]' },
});

// Wait for debounce to settle
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 350));
});

const runButton = screen.getByRole("button", { name: /run tool/i });
await act(async () => {
fireEvent.click(runButton);
});

// First call should have value1
expect(mockCallTool).toHaveBeenCalledTimes(1);
const firstParams = mockCallTool.mock.calls[0][1] as Record<
string,
unknown
>;
expect(firstParams.config).toEqual({ key: "value1" });

// Step 2: Update the value and run tool again
fireEvent.change(textareas[0], {
target: { value: '{ "key": "value2" }' },
});

// Wait for debounce to settle
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 350));
});

await act(async () => {
fireEvent.click(runButton);
});

// Second call should have value2, NOT value1
expect(mockCallTool).toHaveBeenCalledTimes(2);
const secondParams = mockCallTool.mock.calls[1][1] as Record<
string,
unknown
>;
expect(secondParams.config).toEqual({ key: "value2" });
});

it("should send updated JSON values immediately without waiting for debounce (bug #988)", async () => {
const mockCallTool = jest.fn();
renderToolsTab({
tools: [toolWithJsonParams],
selectedTool: toolWithJsonParams,
callTool: mockCallTool,
});

const textareas = screen.getAllByRole("textbox");
expect(textareas.length).toBe(2);

// Enter valid JSON without waiting for the 300ms debounce
fireEvent.change(textareas[0], {
target: { value: '{ "updated": true }' },
});
fireEvent.change(textareas[1], {
target: { value: '["fresh"]' },
});

// Click Run Tool immediately (within 300ms debounce window)
const runButton = screen.getByRole("button", { name: /run tool/i });
await act(async () => {
fireEvent.click(runButton);
});

// Tool should have been called with the fresh values, not stale defaults
expect(mockCallTool).toHaveBeenCalledTimes(1);
const calledParams = mockCallTool.mock.calls[0][1] as Record<
string,
unknown
>;
expect(calledParams.config).toEqual({ updated: true });
expect(calledParams.data).toEqual(["fresh"]);
});
});
});