-
Notifications
You must be signed in to change notification settings - Fork 35
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
Custom play book #61
base: main
Are you sure you want to change the base?
Custom play book #61
Conversation
…able, and some db connections for the features added
…pt-4o playbook content generation
…book functionality to be more consistent with the style given in editing monitoring schedules edit form
…ok file under the db folder, added some error handling for action.ts and db/custom-playbook
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Left a few comments, but overall looks good!
export async function getCustomPlaybooks(projectId?: string, asUserId?: string): Promise<customPlaybook[]> { | ||
if (!projectId) { | ||
throw new Error('[INVALID_INPUT] Project ID is required'); | ||
} |
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.
Would it be possible to make projectId required as a parameter? The fact that you have to check for it existing here is dubious.
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 looked at the code, and there is no apparent reason for projectId to be an optional parameter. I updated the code to reflect this.
export async function getTools( | ||
connection: Connection, | ||
asUserId?: string, | ||
asProjectId?: 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.
Now I realized it might have worked to use connection.projectId
and then you don't need the asProjectId
param. Or is there any case when asProjectId would be different from connection.projectId?
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.
The asProjectId parameter is needed when getTools is called in runners.ts as getTools needs schedule.projectId to be passed into the method for the getPlaybooks portion of getTools to work with the scheduler. In all other cases, where getTools is called, connection.projectId is used instead.
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 guess we could have used connections.projectId
in runners.ts as well, it should be the same. Butthis way is maybe more explicit/clear, and I might have told you to add it :) Let's leave it like this, thanks for checking.
apps/dbagent/src/lib/ai/aidba.ts
Outdated
return listPlaybooks(); | ||
const playbookNames = listPlaybooks(); | ||
const projectId = asProjectId || connection.projectId; | ||
const customPlaybookNames = await actionListCustomPlaybooksNames(projectId, asUserId); |
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.
In general I'd say we have stuff in components
import stuff from lib
but preferably not the other way around. Just to reduce the chances of circular deps.
Since actionListCustomPlaybooksNames
really only calls getListOfCustomPlaybooksNames
I think you can just call that directly.
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.
Changed the method calls for actionListCustomPlaybooksNames and actionGetCustomPlaybookContent to getListOfCustomPlaybooksNames and getCustomPlaybookContent, respectively, in the aidba.ts file to remove potential circular dependencies.
} | ||
|
||
//get a custom playbook by Name (used in scheduler since names are given though getPlaybook) | ||
export async function actionGetCustomPlaybookByName( |
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.
Is this function used anywhere? I could find any matches.
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 agree, there are no references to this method. I will remove it from playbooks/action.ts.
… dependencies for actionGetCustomPlaybookContent and actionGetListOfCustomPlaybooksNames
…book functionality to be more consistent with the style given in editing monitoring schedules edit form
…ry to delete it, they are unable to delete the playbook until it is deleted from the scheduler
…ser is warned if a playbook is being used for monitoring when they try to delete it, they are unable to delete the playbook until it is deleted from the scheduler
…ser is warned if a playbook is being used for monitoring when they try to delete it, they are unable to delete the playbook until it is deleted from the scheduler
…able page, and added copy playbook feature for custom and built in playbooks
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.
Looks great! My only general comment is that there are many comments that feel redundant with the names of variables or functions. Code organization is good though and the feature seems to work as expected. Thanks!
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.
Good to merge from my PoV as well. Nice work @PineappleChild
… via a number, eg (copy 1) (copy 2) etc.
-The playbook table page only shows the run playbook button; all other features are in a dropdown menu to the right of the run playbook button