-
Notifications
You must be signed in to change notification settings - Fork 744
Cache tool call params #646
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
base: main
Are you sure you want to change the base?
Cache tool call params #646
Conversation
return key; | ||
} | ||
|
||
export function saveToolParamsForCache( |
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.
We create a tool cache helper util that helps save and load things from localStorage
@@ -688,6 +689,12 @@ const App = () => { | |||
const callTool = async (name: string, params: Record<string, unknown>) => { | |||
lastToolCallOriginTabRef.current = currentTabRef.current; | |||
|
|||
// Save tool parameters to cache before making the call | |||
const tool = tools.find((t) => t.name === name); |
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.
Save the tool parameters to localStorage cache on execution.
tool: Tool, | ||
): string { | ||
const paramNames = Object.keys(tool.inputSchema.properties ?? {}).sort(); | ||
const key = `tool_params_${btoa(`${serverUrl}_${toolName}_${JSON.stringify(paramNames)}`)}`; |
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.
This ensures uniqueness in localStorage
); | ||
|
||
// Merge cached params with defaults, giving preference to cached values | ||
const mergedParams = { ...defaultParamsObj, ...cachedParams }; |
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.
First load the empty default params, then load the cached ones if they exist.
} | ||
|
||
export function saveToolParamsForCache( | ||
serverUrl: string, |
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.
We can probably remove serverUrl
. Lmk if that'd be better, but also down to just keep it.
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.
Hm, I can see advantages either way. Probably safer to start conservative, with the additional uniqueness of the server URL?
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.
Hi, thanks for adding this. One thing I think this might not address yet is unicode characters when setting the cache key (like when tools have non-English names). I think you could just do something like this:
export function generateCacheKey(
serverUrl: string,
toolName: string,
tool: Tool,
): string {
const paramNames = Object.keys(tool.inputSchema.properties ?? {}).sort();
const keyData = `${serverUrl}_${toolName}_${JSON.stringify(paramNames)}`;
// Replace any non-alphanumeric characters (except _ and -) with underscore
const sanitized = keyData.replace(/[^a-zA-Z0-9_-]/g, '_');
// Ensure it starts with our prefix
return `tool_params_${sanitized}`;
}
@olaservo I wondered about that so I did some searching, and it seems like localStorage allows quite a range of key names: |
We cache tool parameters after you run the tool
Motivation and Context
This is a quality of life improvement. The tool parameters get saved when you run the tool. That way, when you switch in and out of a tool, the parameters are saved and can be re-used.
See this suggestion: #609
How Has This Been Tested?
Did a functional test. The params are saved after tool execution.
Breaking Changes
No
Types of changes
Checklist
Additional context