-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Update bettertouchtool extension #17660
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?
Conversation
Thank you for your first contribution! 🎉 🔔 @dnnsmnstrr you might want to have a look. You can use this guide to learn how to check out the Pull Request locally in order to test it. Due to our current reduced availability, the initial review may take up to 10-15 business days |
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.
PR Summary
This PR significantly enhances the BetterTouchTool extension with new AI tools, variable management commands, and improved trigger handling functionality.
Key changes:
- Added 6 new AI tools for searching/running triggers, managing variables, and handling actions with comprehensive evals
- Implemented robust error handling using
showFailureToast
and standardized error patterns across the codebase - Added new variable management commands (get/set) with proper type validation and persistence options
- Improved named triggers page with better performance, grouping, and frecency-based sorting
- Added
tools
in package.json but missingevals
for AI tools - should reference https://developers.raycast.com/ai/write-evals-for-your-ai-extension
The implementation is thorough and well-structured, though there are a few areas that could be improved for robustness and maintainability.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
31 file(s) reviewed, 33 comment(s)
Edit PR Review Bot Settings | Greptile
"name": "trigger", | ||
"title": "Run BTT Named Trigger", | ||
"subtitle": "Search for and run named trigger in BTT", | ||
"description": "Find a named trigger in BetterTouchTool", | ||
"title": "Search Named Triggers", | ||
"description": "Search for and run named trigger in BTT", | ||
"mode": "view" |
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.
logic: Command name 'trigger' has changed from 'Run BTT Named Trigger' to 'Search Named Triggers'. Can we keep the old name? It is the unique ID where ranking, aliases, and hotkeys are saved.
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.
Willing to revert, but the updated names better follow Raycast conventions
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.
I think the new name makes sense, especially with the app name being displayed behind the command
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.
Yeah, it's confused here - as long as the name
doesn't change
"name": "action", | ||
"title": "Run BTT Action", | ||
"subtitle": "Search for and run an action in BTT", | ||
"description": "Find an action in BetterTouchTool", | ||
"title": "Search Actions", | ||
"description": "Search for and run an action in BTT", | ||
"mode": "view" | ||
}, |
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.
logic: Command name 'action' has changed from 'Run BTT Action' to 'Search Actions'. Can we keep the old name? It is the unique ID where ranking, aliases, and hotkeys are saved.
if (isNaN(numValue)) { | ||
// If number parsing fails, fall back to string type | ||
return { | ||
success: true, | ||
value: { type: "string", value: variableValue }, | ||
}; |
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.
logic: Silently falling back to string type when number parsing fails could lead to unexpected behavior. Consider throwing an error instead when variableType is 'number' but parsing fails.
if (isNaN(numValue)) { | |
// If number parsing fails, fall back to string type | |
return { | |
success: true, | |
value: { type: "string", value: variableValue }, | |
}; | |
if (isNaN(numValue)) { | |
return { | |
success: false, | |
error: `Failed to parse '${variableValue}' as a number`, | |
}; | |
} |
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.
Willing to change, but I believe this makes sense for AI tooling
- refactor: Improve code formatting and comment clarity in run-named-trigger.ts - feat(raycast-ai): Implement AI tools for triggers/actions/vars, improve named triggers, add set/get variable commands & prefs - Initial commit Update bettertouchtool extension - feat(raycast-ai): Implement AI tools for triggers/actions/vars, improve named triggers, add set/get variable commands & prefs - refactor: Improve code formatting and comment clarity in run-named-trigger.ts - feat(raycast-ai): Implement AI tools for triggers/actions/vars, improve named triggers, add set/get variable commands & prefs Delete extensions/bettertouchtool/.aider.input.history Removed aider
cc1fd85
to
c36688d
Compare
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
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.
PR Summary
(updates since last review)
Based on my review of the changes and the previous feedback, here are the key new points to highlight:
The PR adds significant functionality to the BetterTouchTool extension with a focus on AI integration and variable management.
- The CHANGELOG.md entry for "AI Tools & Variable Commands" correctly uses
{PR_MERGE_DATE}
template string but previous entries use actual dates which should be updated - Command names in
package.json
have been changed (e.g.,trigger
,action
) which could break existing user configurations - recommend keeping original command names - Lists and grids should implement
isLoading
state to avoid empty state flicker, particularly intrigger.tsx
and other view components getSelectedText()
calls in the codebase should be wrapped in try/catch blocks for graceful error handling- Consider adding subtitles to commands in
package.json
to better identify the service (BetterTouchTool) for users with multiple similar commands
The implementation is solid overall but these adjustments would improve robustness and user experience.
31 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings | Greptile
.aider* | ||
.idea* |
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.
style: Consider adding a comment above these lines to indicate they are for editor/tool specific files, similar to how other sections are labeled with comments
- **Search Named Triggers**: Find and run named triggers. The type of associated action will be displayed and in some cases you can hover over the action to see a preview of the code/file that will be executed. | ||
- **Search Actions**: Search for predefined BTT actions and run them. | ||
- **Get Variable Value**: Get the value of a variable from BTT (string or number). | ||
- **Set String Variable**: Set a string variable in BTT (persistent or temporary). | ||
- **Set Number Variable**: Set a number variable in BTT (persistent or temporary). |
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.
style: Command descriptions are inconsistent - some end with periods, others don't
- **Search Named Triggers**: Search for Named Triggers in BetterTouchTool. | ||
- **Run Named Trigger**: Run a Named Trigger. | ||
- **Get Variable Value**: Get the value of a variable from BetterTouchTool. | ||
- **Set Variable Value**: Set the value of a variable in BetterTouchTool. | ||
- **Search Actions**: Search for Actions in BetterTouchTool. | ||
- **Run Action**: Run an Action in BetterTouchTool. |
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.
style: AI tool descriptions are duplicates of command names without additional context or differentiation
"name": "variableValue", | ||
"type": "text", | ||
"placeholder": "Value", | ||
"required": false |
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.
logic: variableValue for number variables should use type 'number' instead of 'text' to ensure proper validation
console.error(error); | ||
showFailureToast(error); | ||
await showFailureToast(error); |
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.
style: console.error is unnecessary since error is already handled by showFailureToast
console.error(error); | |
showFailureToast(error); | |
await showFailureToast(error); | |
await showFailureToast(error); |
export default async function tool(input: Input) { | ||
const namedTriggersResult = await getNamedTriggers(); | ||
if (namedTriggersResult.status === "error") { | ||
return namedTriggersResult.error; |
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.
logic: Error handling is inconsistent - returning error string directly instead of maintaining Result<T> type pattern
return namedTriggersResult.error; | |
return { status: "error", error: namedTriggersResult.error }; |
* Sets a value for the specified variable in BetterTouchTool. | ||
*/ | ||
export default async function tool(input: Input): Promise<Output> { | ||
const { variableName, variableValue, variableType, isPersistent = false } = input; |
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.
style: Consider trimming variableName and variableValue to handle leading/trailing whitespace consistently
} | ||
: { | ||
status: "success", | ||
message: `\`${variableName}\` set to \`${parseResult.value.value}\` **(${parseResult.value.type})**`, |
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.
style: Consider escaping backticks in variableName and value to handle cases where they contain backticks
parentUUID?: string; | ||
} | ||
|
||
type Success<T> = T extends void ? { status: "success" } : { status: "success"; data: T }; |
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.
style: Consider making Success type exported since it's used as part of the exported Result type. This would help with type inference in consuming code.
|
||
type Success<T> = T extends void ? { status: "success" } : { status: "success"; data: T }; | ||
export type Result<T> = Success<T> | { status: "error"; error: string }; | ||
export type SetVariableValue = { type: "string"; value: string } | { type: "number"; value: number }; |
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.
style: Consider adding readonly modifiers to prevent accidental mutations of these value types
export type SetVariableValue = { type: "string"; value: string } | { type: "number"; value: number }; | |
export type SetVariableValue = { readonly type: "string"; readonly value: string } | { readonly type: "number"; readonly value: number }; |
@kmusick Wow, really cool 🤯 Was already thinking about how this extension has lots of potential for the new AI features, and you made it happen! Nice touch with the grouping of named triggers, too. I tried testing your changes and unfortunately only had mixed success with letting the AI run named triggers. After moving the |
message: `Are you sure you want to run the Named Trigger "${input.name}"${input.runType ? ` with run type "${input.runType}"` : ""}?`, | ||
}; | ||
}; | ||
|
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.
Adding this comment seemed to make the AI more reliably search for named triggers before running them:
/**
* Run a named trigger in BetterTouchTool
*
* IMPORTANT: You MUST use the search-named-triggers tool first to find the correct name of the trigger.
* The name MUST MATCH EXACTLY what is returned by search-named-triggers - this includes:
* - Exact capitalization (uppercase/lowercase)
* - Exact spacing
* - Exact punctuation
* - Exact special characters
*
* DO NOT modify the name in any way from what search-named-triggers returns, regardless of what the user inputs.
*
* For example, if search-named-triggers returns "My-Trigger_Name!", you MUST use "My-Trigger_Name!"
* (NOT "my-trigger_name!", "My Trigger Name", "mytriggername", etc.)
*
* Note: Do not make assumptions about whether a trigger is enabled or disabled.
* Simply run the trigger by name without commenting on its enabled status.
*/
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.
I added a condensed version of that IMPORTANT
message and updated Input
type to contain the full SearchResult
object. In my testing, these two changes have really increased run-named-trigger performance. If you are having issues, can you add an eval that demonstrates the issue? If you haven't used the eval functionality much, it seems to require significant adjustments after copying, especially with input that includes nested structures or arrays, in my experience.
"name": "trigger", | ||
"title": "Run BTT Named Trigger", | ||
"subtitle": "Search for and run named trigger in BTT", | ||
"description": "Find a named trigger in BetterTouchTool", | ||
"title": "Search Named Triggers", | ||
"description": "Search for and run named trigger in BTT", | ||
"mode": "view" |
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.
I think the new name makes sense, especially with the app name being displayed behind the command
} | ||
], | ||
"ai": { | ||
"instructions": "When dealing with BetterTouchTool variable values or names, do NOT change the format, capitalization, punctuation, etc. The verbatim values MUST be used. Always include the variable type (string or number) in the output. When the user asks to run something: if the word 'action' or 'actions' appears ANYWHERE in their request, ALWAYS use run-action. Only use run-named-trigger when they specifically mention 'trigger' or don't specify what to run.", |
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.
I would personally want the instructions to more explicitly use the named trigger tool by default, unless asking for an 'action' to be run.
Many of the actions don't really work without additional input and I never found the time to go through and filter out the ones that can't simply be run, so that feature could use some more work if the AI should interact. The list came from here, but it's not really documented, so it would require some manual work / reverse engineering (maybe @fifafu could help)
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.
Removing this text and instead having examples above both run-named-trigger
and run-action
(where it is pointed out multiple times that the user must directly mention action
) seems to have improved performance. npx run evals
has three tests on run-named-trigger
and one on run-action
. I have not had issues after this latest commit, but I imagine any real AI Command usage would need to be decently specific to guarantee the correct behavior.
I am really not very knowledgeable about actions. I agree that the current situation is probably not ideal, but maybe occasionally useful? I imagine power users would always prefer named triggers, and normal users probably wouldn't use this at all, so I don't really think it will see much use. However, I also think it makes sense to ship AI tooling that matches the command functionality, especially since there are a reasonable number that seem to function correctly.
…courage exact inputs, better handled invalid inputs, and better instructions to prefer named trigger vs action
Description
Focused on AI and improving the named triggers experience.
search-named-triggers
run-named-trigger
get-variable-value
set-variable-value
search-action
run-action
get-variable
set-string-variable
set-number-variable
Screencast
Checklist
npm run build
and tested this distribution build in Raycastassets
folder are used by the extension itselfREADME
are placed outside of themetadata
folder