-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: extensions read config #1637
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
3fb3e04 to
238871a
Compare
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 file is autogenerated
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 file is autogenerated
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 file is autogenerated
ui/desktop/src/components/settings_v2/providers/ProviderSettingsPage.tsx
Show resolved
Hide resolved
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.
- exported some structs via openapi so frontend uses same schema
- added a
key()method that creates the key underwhich the extension should be stored
users will interact / supply / create names of extensions only, the key is purely for config storage and lookup under the hood and is abstracted away
|
.bundle |
macOS ARM64 Desktop App (Apple Silicon)📱 Download macOS Desktop App (arm64, signed) Instructions: This link is provided by nightly.link and will work even if you're not logged into GitHub. |
baxen
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.
LGTM! A few comments below
| .required(false) | ||
| .items( | ||
| &extension_status | ||
| &disabled_extensions |
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 we are applying a similar filter twice below, should the existing iter/filter/map be removed or consolidated ?
| } | ||
|
|
||
| pub fn name_to_key(name: &str) -> String { | ||
| format!("_{}", 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.
curious why we add the leading underscore - does this make it backwards incompatible with existing config files? also seems like it could be a bit odd to see if you manually edit your config
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 the editing is sort of an issue -- i don't think the keys should be arbitrary names and i also don't think they should be something the user defines because it's sort of a detail... anyway! am i going to just make the key the same as the name
this is an issue in the UI for example, asking them to define key and name is super confusing, so i wanted a way to abstract the key part out
Sort of think if you bork your config file extensions from bad manual edits that's sort of on you, but idk
| ) | ||
| )] | ||
| pub async fn read_all_config( | ||
| pub async fn update_extension( |
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.
nit: this method is the same as add_extension except it raises an error if it already exists? i will look at the UX code below but usually in this case i'd suggest only having one method that creates or updates
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.
😂 wait that is actually kinda funny -- goose implemented a lot of this and tbh i had to do some moderate work to get it working end to end but i didn't notice this
| ) | ||
| )] | ||
| pub async fn update_extension( | ||
| pub async fn toggle_extension( |
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.
similar comment as above, can we just use a single set meth to also set enabled/dsiabled? it would make sense to have this if in some cases the UI doesn't have the other components of the extension readily available. but if it does i usually would prefer keeping the surface area smaller
| import React from "react"; | ||
| import {useConfig} from "../../ConfigContext"; | ||
|
|
||
| export function ConfigPage({ onClose }: { onClose: () => void }) { |
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.
nit: is this a debugging tool? could add 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.
thought i deleted this
|
i ran the UI locally with when i run |
@salman1993 yes -- this doesn't actually update the state or anything in the UI yet... i just set up the backend routes and some functions in |
|
@baxen -- i have already started implementing some stuff using these routes ( |
* main: (32 commits) ui: load builtins (#1679) chore(release): release version 1.0.14 (#1676) Revert "feat: handling larger more complex PDF docs (and fix) (#1663)" (#1675) fix: uvshim default to existing uv configuration (#1670) fix: handle interruptions during tool responses (#1651) feat: Copy error message button in toast (#1658) feat: handling larger more complex PDF docs (and fix) (#1663) Add Filesystem Tutorial (#1666) docs: figma blog post (#1647) docs: updating goose modes doc (#1665) docs: Add running tasks guide (#1626) docs: Add experimental features (#1644) feat(cli): add better error message, support stdin via -i - or just no args (#1660) feat: extensions read config (#1637) fix: trigger words for memory (#1654) fix: cleanup keyboard shortcut indication (#1642) Extensions load in background and show pending state (#1657) Extension error toast stays until dismissed, and error message cleanup (#1653) fix: remove other category in settings (#1641) fix: restore image outputs from tool calls (#1640) ...
UI changes
Goose core changes
key()method, which is used to generate / get the key under which an extension goeskeyfor lookup nownameof the extension,keyis purely for reading/writing the config fileeg