-
Notifications
You must be signed in to change notification settings - Fork 2.6k
refactor: Centralise deeplink encode and decode into server #3489
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
a9a787e to
bfc7118
Compare
bfc7118 to
3451eb3
Compare
| Router::new() | ||
| .route("/recipe/create", post(create_recipe)) | ||
| .route("/recipes/encode", post(encode_recipe)) | ||
| .route("/recipes/decode", post(decode_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 think we should move /recipe to /recipes in a future pr
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.
agree
DOsinga
left a comment
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.
thanks for jumping on this. my main concern is the way that we have done the API integration in the past and now follow up, sidestepping our open-api setup -
just generate-openapi
| const generateLink = async () => { | ||
| setIsGeneratingLink(true); | ||
| try { | ||
| const currentConfig = { |
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.
here and elsewhere, I'm not sure why we use the word config when we meant to say recipe or recipeToEncode or some such
| } | ||
|
|
||
| // When opening the app with a deeplink, the window is still initializing so we have to duplicate some window dependant logic here. | ||
| async function decodeRecipeMain(deeplink: string, port: number): Promise<Recipe | null> { |
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.
... and hopefully in the not so distant future we can just send the deeplink along when we initialize the backend
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 only soo much refactoring could be done at a time.
| error: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Debug, Deserialize)] |
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.
It looks like this entire recipe thing is not part of our open-api integration somehow. this should be integrated with openapi.rs and then we can autogenerate the ts code. can you check how much work it is to do this the right way? if too much, we can do in a follow up
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'm not familiar with the open-api pieces. I can rectifying that in a future pr.
…ntral-deeplinks * origin/main: (22 commits) feat: deprecate jetbrains extension in favor of public one (#2589) feat: Add LiteLLM provider with automatic prompt caching support (#3380) docs: update desktop instructions for managing sessions (#3522) docs: update desktop instructions for session recipes (#3521) Replace mcp_core::content types with rmcp::model types (#3500) docs: update desktop instructions for tool perms (#3518) docs: update desktop instructions for tool router (#3519) Alexhancock/reapply 3491 (#3515) docs: update mcp install instructions for desktop (#3504) Docs: Access settings in new UI (#3514) feat: switch from mcp_core::Role to rmcp::model::Role (#3488) Revert "fix the output not being visible issue (#3491)" (#3511) fix: Load and Use recipes in new window (#3501) fix: working dir was not being set correctly (#3477) Fix launching session in new window (#3497) Fix tool call allow still showing initial state in chat after navigating back (#3498) feat: add rmcp as a workspace dep (#3483) feat: consolidate subagent execution for dynamic tasks (#3444) fix token alert indicator/popovers hiding and showing (#3492) Fix llm errors not propagating to the ui and auto summarize not starting (#3490) ...
| use crate::recipe::Recipe; | ||
|
|
||
| #[derive(Error, Debug)] | ||
| pub enum DecodeError { |
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.
For future, it would be nice to distinguish the errors.
#[derive(Error, Debug)]
pub enum DecodeError {
#[error("Invalid base64 encoding")]
InvalidBase64,
#[error("Invalid JSON format: {0}")]
InvalidJson(String),
#[error("Invalid recipe format: missing required fields")]
InvalidRecipe,
#[error("Unsupported deeplink format")]
UnsupportedFormat,
}| Router::new() | ||
| .route("/recipe/create", post(create_recipe)) | ||
| .route("/recipes/encode", post(encode_recipe)) | ||
| .route("/recipes/decode", post(decode_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.
agree
* main: Extension Library Improvements (#3541) fix(ui): enable selection of zero-config providers in desktop GUI (#3378) refactor: Renames recipe route to recipes to be consistent (#3540) Blog: Orchestrating 6 Subagents to Build a Collaborative API Playground (#3528) Catch json errors a little better (#3437) Rust debug (#3510) refactor: Centralise deeplink encode and decode into server (#3489) feat: deprecate jetbrains extension in favor of public one (#2589) feat: Add LiteLLM provider with automatic prompt caching support (#3380) docs: update desktop instructions for managing sessions (#3522) docs: update desktop instructions for session recipes (#3521) Replace mcp_core::content types with rmcp::model types (#3500) docs: update desktop instructions for tool perms (#3518) docs: update desktop instructions for tool router (#3519) Alexhancock/reapply 3491 (#3515) docs: update mcp install instructions for desktop (#3504) Docs: Access settings in new UI (#3514) feat: switch from mcp_core::Role to rmcp::model::Role (#3488) Revert "fix the output not being visible issue (#3491)" (#3511) fix: Load and Use recipes in new window (#3501)
* main: (32 commits) fix: use sequential when sub recipe task is 1. (#3573) fix: track message id to keep like with like (#3572) Replace mcp_core::prompt with rmcp::model types (#3561) feat (ui): close recipe modals with esc key (#3568) feat: recipes can retry with success criteria (#3474) Env var to set Ollama request timeout (#3516) Updating docs to match new UI (#3552) Improve Claude Code provider error message for missing CLI (#3363) feat: Work around Gemini API tool call quirks (#3328) feat(ui): Source CashSans-Bold and improve overall text rendering (#3091) refactor: Use openapi for recipe endpoint types and in frontend (#3548) Fix Google Analytics error for local dev (#3544) Extension Library Improvements (#3541) fix(ui): enable selection of zero-config providers in desktop GUI (#3378) refactor: Renames recipe route to recipes to be consistent (#3540) Blog: Orchestrating 6 Subagents to Build a Collaborative API Playground (#3528) Catch json errors a little better (#3437) Rust debug (#3510) refactor: Centralise deeplink encode and decode into server (#3489) feat: deprecate jetbrains extension in favor of public one (#2589) ...
Signed-off-by: Adam Tarantino <tarantino.adam@hey.com>
Summary
Centralized deeplink recipe encoding/decoding logic in
goose-serverto ensure consistency and avoid incompatibility across different implementations, as discussed in PR #3446 review.Key Changes
Centralized Logic:
goose-server).Refactoring:
RecipeConfigstruct into the more completeRecipestruct—frontend now has a single config representation.Tested
Notes