-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Lifei/create save recipe to file #4895
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
Conversation
| .config_dir() | ||
| .join("recipes") | ||
| } else { | ||
| std::env::current_dir().unwrap().join(".goose/recipes") |
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.
unfortunately this is not going to work. we'd need the directory of the session here, not the current directory.
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.
How about the current non global recipes that persists on user's machine? (ie: they were saved on the current working directory before) Shall we still support them?
Are we also going to have recipes in the directory of the session in the future?
|
|
||
| (self.status, body).into_response() | ||
| } | ||
| } |
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 is cute and we certainly need something like this, but I would leave it out for now; the real refactor here is to have something that converts from our errors to a ErrorResponse globally and then apply it everywhere. would love to have it, but I don't think it is in scope /cc @jamadeo
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.
axum is very permissive about what you can return: https://docs.rs/axum/latest/axum/response/index.html
You can already return a (StatusCode, String) and it will convert
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.
cool. we should do this for everything some time. maybe goose can
| throwOnError: true, | ||
| }); | ||
| return response.data.recipe; | ||
| } |
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 know we do this elsewhere, but I am not really a fan of these thin layers that hide where things happen - can we not just call things 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.
These functions are used in multiple times, that's why I extract them here
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.
but what function, this is just calling parseRecipe and naming it parseRecipeFromFile? why the overhead
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.
At the moment the in ImportFileModal calls parseRecipe 2 times. The UI change I included is just for testing. I have revert UI change in this PR and I am going to merge this backend change PR soon.
When we get the latest changes from Zane's PR, we can check whether the Modal has to call parseRecipe multiple time
| if (titleValidationError) { | ||
| throw new Error(titleValidationError); | ||
| } | ||
| // const titleValidationError = await validateTitleUniqueness(value.recipeTitle.trim()); |
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.
what? why would names have to be unique :(
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.
Name and title are different attributes. Name is what's currently displayed in the UI, title is only used for the file name. We added uniqueness check because otherwise it'd overwrite existing recipes altogether, which is also a confusing UX
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.
uhm, if name and title are different attributes and title is use for the file name, I think we are in trouble
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 feel name and title are very confusing in the frontend, and they are used interchangeably. That's why I took this chance to make it simpler.
Currently name is defined in the wrapper of the recipe in the recipe management. Since we don't want the wrap version of the recipe, we can use recipe title instead to display on the UI.
Also we can the title to work out the file name for new recipes or imported recipe.
For the existing recipe, we use the recipe id to work out the existing recipe file path at the server side. It would not use the name or recipe title.
| .route("/recipes/scan", post(scan_recipe)) | ||
| .route("/recipes/list", get(list_recipes)) | ||
| .route("/recipes/delete", post(delete_recipe)) | ||
| .route("/recipes/save_to_file", post(save_recipe_to_file)) |
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.
let's call this just save and save_recipe. no need to tell people we save this to file
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.
Was thinking of save_to_disk but yeah I agree the extra info isn't needed
|
|
||
| pub fn save_recipe_to_file( | ||
| recipe: Recipe, | ||
| is_global: Option<bool>, |
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 we should drop this now. all writes should go to the global version - we can read from local for a while though
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 should also be reflected in the UI by removing the radio buttons
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 have a separate PR for this
| Ok(recipes_with_path) | ||
| } | ||
|
|
||
| pub fn list_all_recipes_from_library() -> Result<Vec<(PathBuf, Recipe)>> { |
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 prefer it if we would integrate this with how the CLI works; there we have a function that reads recipes from various places; can we not add this to that so we get a clear overview
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.
will address in the next PR
| .route("/recipes/list", get(list_recipes)) | ||
| .route("/recipes/delete", post(delete_recipe)) | ||
| .route("/recipes/save_to_file", post(save_recipe_to_file)) | ||
| .route("/recipes/parse", post(parse_recipe)) |
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.
should we replace /decode with this too? and really make parse so that it also takes base64 etc
| .route("/recipes/scan", post(scan_recipe)) | ||
| .route("/recipes/list", get(list_recipes)) | ||
| .route("/recipes/delete", post(delete_recipe)) | ||
| .route("/recipes/save_to_file", post(save_recipe_to_file)) |
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.
Was thinking of save_to_disk but yeah I agree the extra info isn't needed
| continue; | ||
| }; | ||
| let recipe_metadata = | ||
| RecipeManifestMetadata::from_yaml_file(&file_path).unwrap_or_else(|_| { |
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 current import UI also supports JSON so we may want to consider that too
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.
yaml is a superset of json so that should be alright
| if path.exists() { | ||
| for entry in fs::read_dir(path)? { | ||
| let path = entry?.path(); | ||
| if path.extension() == Some("yaml".as_ref()) { |
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.
ditto
|
|
||
| pub fn save_recipe_to_file( | ||
| recipe: Recipe, | ||
| is_global: Option<bool>, |
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 should also be reflected in the UI by removing the radio buttons
| if (titleValidationError) { | ||
| throw new Error(titleValidationError); | ||
| } | ||
| // const titleValidationError = await validateTitleUniqueness(value.recipeTitle.trim()); |
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.
Name and title are different attributes. Name is what's currently displayed in the UI, title is only used for the file name. We added uniqueness check because otherwise it'd overwrite existing recipes altogether, which is also a confusing UX
| // if (!validationResult.success) { | ||
| // const errorMessages = getValidationErrorMessages(validationResult.errors); | ||
| // throw new Error(`Recipe validation failed: ${errorMessages.join(', ')}`); | ||
| // } |
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.
Why are we commenting out this?
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.
As noted in the PR description, the ImportRecipeFrom.tsx change is only for testing. The UI code change will start after Zane's PR is merged
…se into zane/create-recipe-unification-feedback * 'zane/create-recipe-unification' of github.com:block/goose: Lifei/create save recipe to file (#4895)
* main: (24 commits) Lifei/create save recipe to file (#4895) feat(nightly): build nightlies from main shas (#4888) Add missing library for fedora/rhel/centos docs (#4819) feat(process): Add GOVERNANCE and MAINTAINERS documents (#4962) Pause test finder, have it run cargo fmt (#4958) Disable the issue comment trigger on pr-comment-bundle (#4961) fix(providers): update Claude Sonnet 4 model identifier (#4884) fix redirect to extensions page after deeplink install and show toast with success message (#4863) Remove wait-for-ready log (#4956) docs: add a new goose tip (#4940) Add PR template (#4934) Using --resume with --name should still accept session IDs (#4937) Fix auto scroll to bottom during chat (#4923) Fix Typo, Add Description to Hacktoberfest Content Issue Template (#4931) Don't set agent props twice (#4872) fix: conversation fixer merges assistant text blocks and drops empty text messages (#4898) Batch fetch remaining issues for documentation updates fix: session timestamps (#4913) feat: lazy infinite scroller for session history view (#4922) chore: properly identify when to try oauth (#4918) ...
Signed-off-by: HikaruEgashira <[email protected]>
* main: docs: Change community page sections (block#4984) docs: remove temporary Hacktoberfest issue templates (block#4982) Create multi-channel researcher prompt (block#4947) docs: Add Community Content section to Community Page (block#4964) Allow empty API Key when registering custom provider (block#4977) Feat: Add prompt injection detection settings UI + update logging (block#4651) Make create_session work concurrently (block#4954) Lifei/create save recipe to file (block#4895)
Pull Request Description
Created endpoints so that frontend can call the server side to import recipe and save recipe to files
/recipes/save_to_file/recipe/parseRemoved the is_global attribute from list_recipes endpoint payload
Created a general ErrorResponse to return error message from server side
Note
I only changed the ImportFileDialog a bit to just test whether these end points works. I did not make much changes/removal on Frontend to avoid merge conflicts before Zane's PR is merged